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

[Johannes Sixt <j6t@kdbg.org>]

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;