Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan

[Paolo Bonzini <pbonzini@redhat.com>]

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.

Examples:

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.

Paolo