threads / patch / 40928

patchrevision.c: fix possible null pointer access

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

## tl;dr

10 messages between Dec 3, 2015 and Dec 7, 2015. Diffs are folded; open one to read it.

replies: 9people: 6as markdown or json

Stefan Naewe· Dec 3, 2015, 19:32 UTC · lore

Two functions dereference a tree pointer before checking 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...)
 revision.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
Show changes to revision.c +10 −2
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;
 
 	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;
 	obj->flags |= UNINTERESTING;

-- 
2.6.3
Junio C Hamano· Dec 3, 2015, 20:06 UTC · re: Stefan Naewe · lore

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

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.

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

Show 18 quoted lines
>  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.

Show 17 quoted lines
>  
>  	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.
Stefan Naewe· Dec 3, 2015, 21:15 UTC · re: Junio C Hamano · lore

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

On Thu, Dec 3, 2015 at 9:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 44 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.
>
> > 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.
OK.
Show 21 quoted lines
>
> >
> >       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.
But 'tree->object' is dereferencing tree, isn't it ? Like '(*tree).object'.
??
> At least, please lose the blank line after
> the new assignment.
Will do, if you want this patch at all.
> >       obj->flags |= UNINTERESTING;
>
> Thanks.
Thanks,
  Stefan
-- 
----------------------------------------------------------------
python -c "print '73746566616e2e6e6165776540676d61696c2e636f6d'.decode('hex')"
Philip Oakley· Dec 3, 2015, 21:15 UTC · re: Junio C Hamano · lore

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

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

Stefan Beller· Dec 3, 2015, 22:17 UTC · re: Philip Oakley · lore

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

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

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.

Show 17 quoted lines
>
> 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. :)
Junio C Hamano· Dec 4, 2015, 15:39 UTC · re: Philip Oakley · lore

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

"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.
Jeff King· Dec 4, 2015, 23:32 UTC · re: Junio C Hamano · lore

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

On Fri, Dec 04, 2015 at 07:39:10AM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> > 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 ;-)

FWIW, I'd worry much more about compilers which do aggressive optimizations based on language-lawyering (e.g., removing the null-check as dead code, which is legal according to the standard because after you computed the pointer based on it, it's all undefined behavior).

I don't think that changes your conclusion, though:
> So as I said in my review, the first hunk is a reject, the second
> one is OK.
-Peff
Stefan Naewe· Dec 5, 2015, 15:27 UTC · re: Junio C Hamano · lore

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

mark_tree_uninteresting dereferences a tree pointer before checking if the pointer is valid. Fix that by doing the check first.

Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
---
 revision.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to revision.c +3 −1
diff --git a/revision.c b/revision.c
index 0fbb684..8c569cc 100644
--- a/revision.c
+++ b/revision.c
@@ -135,10 +135,12 @@ 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;
 	obj->flags |= UNINTERESTING;

-- 
2.6.3
Junio C Hamano· Dec 7, 2015, 20:31 UTC · re: Stefan Naewe · lore

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

Stefan Naewe <stefan.naewe@gmail.com> writes:
Show 5 quoted lines
> mark_tree_uninteresting dereferences a tree pointer before checking
> if the pointer is valid. Fix that by doing the check first.
>
> Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
> ---

I still have a problem with "dereferences", as "dereference" is about computing an address and accessing memory based on the result, and only the first half is happening here. I can live with "The function does a pointer arithmetic on 'tree' before it makes sure that 'tree' is not NULL", but in any case, let's queue this as-is for now and wait for a while to see if others can come up with a more appropriate phrases.

Thanks.
Show 21 quoted lines
>  revision.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index 0fbb684..8c569cc 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -135,10 +135,12 @@ 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;
>  	obj->flags |= UNINTERESTING;
Johannes Sixt· Dec 7, 2015, 21:54 UTC · re: Junio C Hamano · lore

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

Am 07.12.2015 um 21:31 schrieb Junio C Hamano:
Show 15 quoted lines
> Stefan Naewe <stefan.naewe@gmail.com> writes:
>
>> mark_tree_uninteresting dereferences a tree pointer before checking
>> if the pointer is valid. Fix that by doing the check first.
>>
>> Signed-off-by: Stefan Naewe <stefan.naewe@gmail.com>
>> ---
>
> I still have a problem with "dereferences", as "dereference" is
> about computing an address and accessing memory based on the result,
> and only the first half is happening here.  I can live with "The
> function does a pointer arithmetic on 'tree' before it makes sure
> that 'tree' is not NULL", but in any case, let's queue this as-is
> for now and wait for a while to see if others can come up with a
> more appropriate phrases.

Don't shoo away language lawyers, because this is a pure C language rule patch. If this were only about pointer arithmetic, a change would not be necessary. But it isn't. The patch corrects a case where the compiler can remove a NULL pointer check that we actually want to remain. The language rule that gives sufficient room for interpretation to the compiler is about dereferencing a pointer. It is irrelevant that an address of an object is taken after the dereference and then only pointer arithmetic remains---the dereference has already taken place, and that cannot occur for a NULL pointer in a valid program. So, the phrase "dereference" is precise and correct here.

-- Hannes
Show 24 quoted lines
>
> Thanks.
>
>>   revision.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 0fbb684..8c569cc 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -135,10 +135,12 @@ 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;
>>   	obj->flags |= UNINTERESTING;

← back to recent threads