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

From: Philip Oakley <philipoakley@iee.org>
Date: Dec 3, 2015, 21:15 UTC
Message-ID: <46311B14CC814F54AC34764F2520947A@PhilipOakley>
In-Reply-To: <xmqqlh9bthyb.fsf@gitster.mtv.corp.google.com>

From: "Junio C Hamano" <gitster@pobox.com>

Show 8 quoted lines

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

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)

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

Show 64 quoted lines

>
>> if the pointer is valid. Fix that by doing the check first.
>>
>> Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
>> ---
>> This has been reported through the CppHints newsletter 
>> (http://cpphints.com/hints/40)
>> but doesn't seem to have made its way to the ones who care (the git list
>> that is...)
>
> Nobody would be surprised, unless the newsletter was sent to this
> list, which I do not think it was (but if it was sent while I was
> away, then it is very possible that I didn't see it).
>
>>  revision.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 0fbb684..bb40179 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -104,7 +104,12 @@ static void mark_tree_contents_uninteresting(struct 
>> tree *tree)
>>  {
>>  struct tree_desc desc;
>>  struct name_entry entry;
>> - struct object *obj = &tree->object;
>> + struct object *obj;
>> +
>> + if (!tree)
>> + return;
>> +
>> + obj = &tree->object;
>
> This is questionable; if you check all the callers of this function
> (there are two of them, I think), you would notice that they both
> know that tree cannot be NULL here.
>
>>
>>  if (!has_sha1_file(obj->sha1))
>>  return;
>> @@ -135,10 +140,13 @@ static void mark_tree_contents_uninteresting(struct 
>> tree *tree)
>>
>>  void mark_tree_uninteresting(struct tree *tree)
>>  {
>> - struct object *obj = &tree->object;
>> + struct object *obj;
>>
>>  if (!tree)
>>  return;
>> +
>> + obj = &tree->object;
>> +
>>  if (obj->flags & UNINTERESTING)
>>  return;
>
> This one is not wrong per-se, but an unnecessary change, because no
> deferencing is involved.  At least, please lose the blank line after
> the new assignment.
>
>>  obj->flags |= UNINTERESTING;
>
> Thanks.

-- Philip