From: Johannes Sixt Date: Mon, 07 Dec 2015 21:54:23 GMT Subject: Re: [PATCH v2] revision.c: fix possible null pointer access Message-ID: <5666000F.8050306@kdbg.org> In-Reply-To: Am 07.12.2015 um 21:31 schrieb Junio C Hamano: > Stefan Naewe 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 >> --- > > 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;