qemu-trivial
[Top][All Lists]
Advanced

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

Re: [Qemu-trivial] [Qemu-devel] [PATCH] scripts: add sample model file f


From: Markus Armbruster
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan
Date: Wed, 19 Mar 2014 16:56:58 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux)

Paolo Bonzini <address@hidden> writes:

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

REVERSE_INULL is a null check after a dereference.  Pretending
g_malloc() never returns a null pointer can conceivably create false
negatives: Coverity flags a null check as REVERSE_INULL even though it's
necessary.  This should be rare, because checking for null return value
instead of zero size argument is unnatural.

I can't explain offhand how the TAINTED_SCALAR get unmasked.

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

I've seen a few like these myself.  Coverity isn't quite smart enough to
prove non-zero size, and then non-null return value.

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

g_renew(struct memmap_entry_s, dma->memmap, dma->memmap_size + 1)

One overflow less to worry about.  I'd give sich transformations a try
if getting tree-wide cleanups committed wasn't such an impossible pain.
Oh well, we can fix 'em one CVE at a time.

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

Coverity comes with models for all four (see its
library/generic/libc/all/all.c).  I'm surprised you adding your own
models has any positive effect.

>                                            Given the results, okay to
> use the limited model where realloc never frees and malloc(0) returns
> non-NULL?

I'd describe realloc() as "always frees the old block, returns a new
block, which is never null".  We might mean the same.

Go ahead with the lying^Wlimited model, with a big, fat comment
explaining our reasons.



reply via email to

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