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

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

[Stefan Naewe]

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(-)

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

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

[Junio C Hamano]

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.

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

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

[Stefan Naewe]

On Thu, Dec 3, 2015 at 9:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

>
> >
> >       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')"

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

[Philip Oakley]

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.

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/

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

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

[Stefan Beller]

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

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

[Junio C Hamano]

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

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

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

[Jeff King]

On Fri, Dec 04, 2015 at 07:39:10AM -0800, Junio C Hamano wrote:

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

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

[Stefan Naewe]

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(-)

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

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

[Junio C Hamano]

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.

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;

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

[Johannes Sixt]

Am 07.12.2015 um 21:31 schrieb Junio C Hamano:
> 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

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