Re: [PATCH] tag: avoid NULL pointer arithmetic

From: René Scharfe <l.s.r@web.de>
Date: Oct 3, 2017, 12:51 UTC
Message-ID: <19a08fb1-2fb3-f368-772b-36646a179975@web.de>
In-Reply-To: <20171003102215.9952-1-szeder.dev@gmail.com>

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

Show 16 quoted lines

>> 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é