From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49752) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WQcbE-0002CF-VE for qemu-devel@nongnu.org; Thu, 20 Mar 2014 09:01:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WQcb7-0000ZQ-1D for qemu-devel@nongnu.org; Thu, 20 Mar 2014 09:01:32 -0400 Message-ID: <532AE69E.4070502@redhat.com> Date: Thu, 20 Mar 2014 14:01:18 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1395247965-13889-1-git-send-email-pbonzini@redhat.com> <5329D4B7.4090602@redhat.com> <5329F406.7040606@redhat.com> <874n2tgw9h.fsf@blackfin.pond.sub.org> In-Reply-To: <874n2tgw9h.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] 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 20/03/2014 08:32, Markus Armbruster ha scritto: >>>> +static void __write(uint8_t *buf, int len) >>> >>> Will the fact that you used 'int len' instead of 'size_t' bite us on 32- >>> vs. 64-bit? Same for __read. >> >> Yeah, I copied this from address_space_rw. I'll change to ssize_t to >> catch negative values. > > Change the real address_space_rw(), or the model's __write()? __read and __write for now (hard freeze etc. etc.). >> + if (is_write) __write(buf, len); else __read(buf, len); >> + >> + return result; >> +} > > I'm curious: could you give me a rough idea on how modelling > address_space_rw() affects results? Sure! The problematic code is this one: if (!memory_access_is_direct(mr, is_write)) { l = memory_access_size(mr, l, addr1); /* XXX: could force current_cpu to NULL to avoid potential bugs */ switch (l) { case 8: /* 64 bit write access */ val = ldq_p(buf); error |= io_mem_write(mr, addr1, val, 8); break; Coverity doesn't understand that memory_access_size return a value that is less than l, and thus thinks that address_space_rw can do an 8-byte access. So it flags cases where we use it to read into an int or a similarly small char[]. It's actually fairly common, it occurs ~20 times. >> +static int get_keysym(const name2keysym_t *table, >> + const char *name) > > Curious again: is this just insurance, or did you observe scanning > improvements? It fixes exactly one error. All of the "tainted value" can be considered false positives, but I wanted to have an example on how to shut them up. > This claims g_malloc(0) returns a non-null pointer to a block of size 1. > Could we say it returns a non-null pointer to a block of size 0? Not sure of the semantics of __coverity_alloc__(0). Leave it to further future improvements? > if (success) { > void* tmp = __coverity_alloc__(size); > if (tmp) __coverity_mark_as_uninitialized_buffer__(tmp); > __coverity_mark_as_afm_allocated__(tmp, AFM_free); > return tmp; > } else { > __coverity_panic__ (); > } Is the "if" needed at all? The "else" path is killed altogether by __coverity_panic__(). Paolo