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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 19/03/2014 14:56, Paolo Bonzini ha scritto:
> 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.

Doing this only fixes one false positive.  Given the results, okay to 
use the limited model where realloc never frees and malloc(0) returns 
non-NULL?

Paolo