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

[Max Reitz]

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?  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.  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.

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.

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);
>

signature.asc
Description: OpenPGP digital signature