Re: [PATCH] tag: avoid NULL pointer arithmetic

["René Scharfe" <l.s.r@web.de>]

Am 03.10.2017 um 12:22 schrieb SZEDER Gábor:
>> 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é