Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:

quoted

lookup_blob() etc. can return NULL if the referenced object isn't of the
expected type.  In theory it's wrong to reference the object member in
that case.  In practice it's OK because it's located at offset 0 for all
types, so the pointer arithmetic (NULL + 0) is optimized out by the
compiler.  The issue is reported by Clang's AddressSanitizer, though.

Avoid the ASan error by casting the results of the lookup functions to
struct object pointers.  That works fine with NULL pointers as well.  We
already rely on the object member being first in all object types in
other places in the code.

This sounds like the main goal of the patch is to avoid an ASan error,
but I think it's more important to avoid (and to be more explicit
about avoiding) the undefined behavior.  I.e. along the lines of
s/In theory it's wrong/It's undefined behavior/ and
s/ASan error/undefined behavior/

You are probably right, but I struggle to pin down the difference.  Another
try: Adding zero to a NULL pointer is undefined, but is either optimized
out or has no effect.  That's why the code doesn't crash without ASan and
UBSan.  Relying on undefined behavior is wrong nevertheless, so let's stop
doing it.

Furthermore, fsck.c:fsck_walk_tree() does the same "immediately
reference the object member in lookup_blob()'s and lookup_tree()'s
return value" thing.  I think those should receive the same treatment
as well.

Hmm, are put_object_name() and all the walk() implementations ready for
a NULL object handed to them?  Or would we rather need to error out
right there?  Other suspicious places in this regard are (at least):
builtin/merge-tree.c, cache-tree.c, http-push.c, list-objects.c,
merge-recursive.c, and shallow.c. :-/

René