Paolo Bonzini <pbonz...@redhat.com> 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.
