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

[Liam Merwick]

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 <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > ---
> >   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);