[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [PATCH] scripts: add sample model file for Coverity S

From: Paolo Bonzini
Subject: Re: [Qemu-trivial] [PATCH] scripts: add sample model file for Coverity Scan
Date: Wed, 19 Mar 2014 14:56:25 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0

Il 19/03/2014 13:46, Paolo Bonzini ha scritto:
Il 19/03/2014 10:08, Markus Armbruster ha scritto:
It probably would make static analysis a bit less powerful or will
return more false positives.  The NULL return for realloc (in the
"free" case) already causes some.  So I'm undecided between a more
correct model and a more selective one (with a fat comment).

I can't see how lying to the analyzer could make it more powerful :)
It can, however, suppress false positives.  Scan and find out how many?

Full model (g_malloc returns NULL for 0 argument) => 750 defects

Posted model (g_malloc never returns NULL)        => 702 defects
        -59 NULL_RETURNS defects
         -1 REVERSE_INULL defects
        +12 TAINTED_SCALAR defects

Reduced model (g_realloc never frees)             => 690 defects
        -12 NULL_RETURNS defects

Of course, silly me, I threw away the results of the analysis for the
full model.  I'll now rerun it and look for false negatives caused by
the reduced model.

For the REVERSE_INULL and TAINTED_SCALAR defects, I don't see why the model should make any difference. The missing REVERSE_INULL becomes a false-negative. The new TAINTED_SCALAR were false negatives.

I checked ~10 of the NULL_RETURNS and they are all false positives. Either the argument really cannot be zero, or it is asserted that it is not zero before accessing the array, or the array access is within a for loop that will never roll if the size was zero.


1) gencb_alloc (and a few others have the same idiom) gets a length, allocates a block of the given length, and fills in the beginning of that block. It's arguably missing an assertion that the length is good-enough. No reason for this to be tied to NULL_RETURNS, but in practice it is.

2) This only gets zero if there is an overflow, since dma->memmap_size is initialized to zero. But Coverity flags it as a possible NULL return:

316         dma->memmap = g_realloc(dma->memmap, sizeof(*entry) *
317                         (dma->memmap_size + 1));

3) vnc_dpy_cursor_define calls g_malloc0(vd->cursor_msize), which returns NULL if the array has size 0. Coverity complains because cursor_get_mono_mask calls memset on the result, but we already rely elsewhere on that not happening for len == 0.

I think we're well into diminishing returns, which justifies using the less-precise model.

I'm now adding new models for memset/memcpy/memmove/memcmp that check for non-zero argument, and see what that improves with respect to the full and reduced models.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]