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

[Stefan Beller <sbeller@google.com>]

On Thu, Dec 3, 2015 at 1:34 PM, Philip Oakley <philipoakley@iee.org> wrote:
> 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.

Well compiler people want their compiler to produce the best output,
meaning the compiled code goes fast.

So if you ask a compiler-writer, this may qualify enough for
being a dereference, because it looks like a dereference.

Assuming this is a dereference, you can further reason about
upcoming

  if (pointer)

As the pointer was already dereferenced, it can be assumed not NULL.
(the pointer being NULL would be undefined behavior, in which the
compiler can do whatever it wants, i.e. that case can be ignored)

So with the strong assumption of the pointer being not NULL, you
can optimize away an

  if (pointer)

as that is "always" false.

In case the pointer is NULL, we have had undefined behavior, so
the compiler is allowed to generate wrong code.

Which is why the if(pointer) is removed from the compiled binary,
as less instructions make the code go faster.

>
> 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.
>
> That said, the fact that we know it can't be null does save the day, until
> that is, the compiler [via some coding of an interpretation] decides that it
> could be null and thus undefined etc etc (which one would argue as poor
> logic, but standards have no truck with such arguments;-).
>
> There were some discussion on undefined behaviour way back (2013-08-08) when
> Stephan Beller looked at STACK's checking of the Git code, see for example
> http://article.gmane.org/gmane.comp.version-control.git/231945/
> "3 issues have been discovered using the STACK tool
> The paper regarding that tool can be found at
> https://pdos.csail.mit.edu/papers/stack:sosp13.pdf" (link updated)


Yeah that tool would detect such a bug. I can see
if I can get it to run frequently and post results somewhere.
IIRC it was quite a pain to get it working correctly on Git and
then reasoning for the resulting patch.

>
> All their source code is publicly available at
> http://css.csail.mit.edu/stack/

Thanks for pointing to that tool again. :)