From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50638) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQHwp-0007Le-D2 for qemu-devel@nongnu.org; Wed, 19 Mar 2014 10:58:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQHwh-0004Zu-0P for qemu-devel@nongnu.org; Wed, 19 Mar 2014 10:58:27 -0400 Sender: Paolo Bonzini Message-ID: <5329B073.9030907@redhat.com> Date: Wed, 19 Mar 2014 15:57:55 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1395162223-28733-1-git-send-email-pbonzini@redhat.com> <874n2vcpu9.fsf@blackfin.pond.sub.org> <53294155.8040403@redhat.com> <87d2hi4ktz.fsf@blackfin.pond.sub.org> <5329918C.8090403@redhat.com> <5329A209.5000308@redhat.com> In-Reply-To: <5329A209.5000308@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] scripts: add sample model file for Coverity Scan List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-trivial@nongnu.org, qemu-devel@nongnu.org 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