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

On Thu, Dec 3, 2015 at 1:34 PM, Philip Oakley [off-list ref] wrote:

From: "Junio C Hamano" <redacted>

quoted

Stefan Naewe [off-list ref] writes:

quoted

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. :)