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

Stefan Naewe [off-list ref] 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 <redacted>
---
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).

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.

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.

On Thu, Dec 3, 2015 at 9:06 PM, Junio C Hamano [off-list ref] wrote:

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.

quoted

if the pointer is valid. Fix that by doing the check first.

Signed-off-by: Stefan Naewe <redacted>
---
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).

quoted

 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.

quoted

      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.

quoted

      obj->flags |= UNINTERESTING;

Thanks.

Thanks,
  Stefan
-- 
----------------------------------------------------------------
python -c "print '73746566616e2e6e6165776540676d61696c2e636f6d'.decode('hex')"

From: "Junio C Hamano" <redacted>

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.

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/

quoted

if the pointer is valid. Fix that by doing the check first.

Signed-off-by: Stefan Naewe <redacted>
---
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).

quoted

 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.

quoted

 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.

quoted

 obj->flags |= UNINTERESTING;

Thanks.

--
Philip 

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

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