Re: [PATCH] revision.c: fix possible null pointer access
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:07:22
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH] revision.c: fix possible null pointer access · Jeff King <hidden>
- 2016-06-15 · [PATCH] revision.c: fix possible null pointer access · Stefan Naewe <hidden>
- 2016-06-15 · Re: [PATCH] revision.c: fix possible null pointer access · Junio C Hamano <hidden>
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).
quoted hunk
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 hunk
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.