From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52634) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjk2i-0001n7-Vl for qemu-devel@nongnu.org; Tue, 13 Sep 2016 05:30:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjk2f-0006qL-26 for qemu-devel@nongnu.org; Tue, 13 Sep 2016 05:30:15 -0400 Date: Tue, 13 Sep 2016 11:30:02 +0200 From: Kevin Wolf Message-ID: <20160913093002.GA6021@noname.redhat.com> References: <1473758138-19260-1-git-send-email-stefanha@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473758138-19260-1-git-send-email-stefanha@redhat.com> Subject: Re: [Qemu-devel] [PATCH] qcow2: avoid memcpy(dst, NULL, len) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org Am 13.09.2016 um 11:15 hat Stefan Hajnoczi geschrieben: > Section "7.1.4 Use of library functions" in the C99 standard says: > > If an argument to a function has an invalid value (such as [...] > a null pointer [...]) [...] the behavior is undefined. > > Additionally the "searching and sorting" functions are specified as > requiring valid pointer values as described in 7.1.4. > > This patch fixes the following static analyzer errors: "sanitizer", not "static analyzer" > block/qcow2.c:1807:41: runtime error: null pointer passed as argument 2, which is declared to never be null > block/qcow2-cluster.c:86:26: runtime error: null pointer passed as argument 2, which is declared to never be null > > Reported-by: Peter Maydell > Signed-off-by: Stefan Hajnoczi > --- > block/qcow2-cluster.c | 4 +++- > block/qcow2.c | 5 ++++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index f941835..ab0dcdc 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -83,7 +83,9 @@ int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size, > } > memset(new_l1_table, 0, align_offset(new_l1_size2, 512)); > > - memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t)); > + if (s->l1_table) { > + memcpy(new_l1_table, s->l1_table, s->l1_size * sizeof(uint64_t)); > + } We could add 'else assert(s->l1_size == 0)' to make the intentions clearer. Or actually, maybe the simpler way would be to do 'if (s->l1_size)', then it's obvious that the skipped memcpy() wouldn't do anything anyway. > /* write new table (align to cluster) */ > BLKDBG_EVENT(bs->file, BLKDBG_L1_GROW_ALLOC_TABLE); > diff --git a/block/qcow2.c b/block/qcow2.c > index c079aa8..758a997 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1804,7 +1804,10 @@ static size_t header_ext_add(char *buf, uint32_t magic, const void *s, > .magic = cpu_to_be32(magic), > .len = cpu_to_be32(len), > }; > - memcpy(buf + sizeof(QCowExtension), s, len); > + > + if (s) { > + memcpy(buf + sizeof(QCowExtension), s, len); > + } Same thing here. With or without the change: Reviewed-by: Kevin Wolf