Re: [Qemu-devel] [PATCH v4 6/8] block: dump_qlist() may dereference a Null pointer

[Liam Merwick <liam.merwick@oracle.com>]

On 05/11/18 00:07, Max Reitz wrote:
> On 19.10.18 22:39, Liam Merwick wrote:
>> A NULL 'list' passed into function dump_qlist() isn't correctly
>> validated and can be passed to qlist_first() where it is dereferenced.
>>
>> Given that dump_qlist() is static, and callers already do the right
>> thing, just add an assert to catch future potential bugs (plus the
>> added benefit of suppressing a warning from a static analysis tool
>> and removing this noise will help us better find real issues).
> 
> But can't you fix the tool? 

I don't have access to the tool source but have been filing bugs against 
it as I run it on the QEMU codebase and discover false positives.

> My opinion is still that large parts of our
> code do not assert that some parameter is not NULL, and I think it isn't
> a good idea to make them assert that.  

Yeah, that can be a slippery slope....

> I don't know what makes this
> function special, and I wonder why it is special to your tool -- as I've
> said in the last version, dump_qdict() is basically the same in this
> regard.  I wonder why your tool doesn't mind that.
> 

I had gone though the code paths to try to see how the tool was happy 
with one and not the other - the implementation differed slightly w.r.t 
macro usage but I couldn't see any obvious reason.

> Can you not whitelist something as false positives?  I know we have a
> lot of those in Coverity, and we just mark them as such, and that's it.

Yeah, I can flag this as a FP and have it fall off my list.

I'll will drop this patch in v5

Regards,
Liam

> 
> Finally, one could argue that the nonnull GCC function attribute would
> be a better fit, actually.
> 
> But overall, I just don't think it's a good idea to start changing the
> code to accommodate for false positives in static analyzers, because in
> my experience the number of false positives only rises with time.
> 
> Max
> 
>> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block/qapi.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index c66f949db839..e81be604217c 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -740,6 +740,8 @@ static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
>>       const QListEntry *entry;
>>       int i = 0;
>>   
>> +    assert(list);
>> +
>>       for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
>>           QType type = qobject_type(entry->value);
>>           bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
>>
> 
>