[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Sca
From: |
Markus Armbruster |
Subject: |
Re: [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.
- [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Paolo Bonzini, 2014/03/18
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Markus Armbruster, 2014/03/18
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Paolo Bonzini, 2014/03/19
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Markus Armbruster, 2014/03/19
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Paolo Bonzini, 2014/03/19
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Paolo Bonzini, 2014/03/19
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Paolo Bonzini, 2014/03/19
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Paolo Bonzini, 2014/03/19
Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan, Kevin Wolf, 2014/03/19