From: "Junio C Hamano" <gitster@xxxxxxxxx>
Stefan Naewe <stefan.naewe@xxxxxxxxx> 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@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html