Re: [PATCH] revision.c: fix possible null pointer access

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2015, 15:39 UTC
Message-ID: <xmqq610ete8x.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <46311B14CC814F54AC34764F2520947A@PhilipOakley>

"Philip Oakley" <philipoakley@iee.org> writes:

Show 15 quoted lines

> From: "Junio C Hamano" <gitster@pobox.com>
>> Stefan Naewe <stefan.naewe@gmail.com> writes:
>>
>>> Two functions dereference a tree pointer before checking
>>
>> Reading them a bit carefully, a reader would notice that they
>> actually do not dereference the pointer at all.  It just computes
>> another pointer and that is done by adding the offset of object
>> member in the tree struct.
>
> But you can't do that computation (in the error case under
> consideration). Null can't be added to anything (as far as the
> implications of the standards go). These are horrid gotchas because
> they go against the grain of all that binary arithmetic and
> simplifications we learnt long ago.

Yeah, but in that hunk that does check !tree, because the function can be fed a NULL, the computed result assigned to object, which is undefined, is never used ;-)

Of course, there used to be exotic platforms that are still standard compliant that triggered a trap when such a pointer computation was made (rather, such a bogus pointer was assigned to a pointer variable). I do not think anybody attempted to port Git to such a platform, but I agree that it is better to "fix" such a codepath, if only to stop wasting time dealing with them discussing with language lawyers ;-)

So as I said in my review, the first hunk is a reject, the second one is OK.

Thanks.