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.