From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M6J4X-0003wb-QF for qemu-devel@nongnu.org; Tue, 19 May 2009 02:45:09 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M6J4S-0003vV-0n for qemu-devel@nongnu.org; Tue, 19 May 2009 02:45:08 -0400 Received: from [199.232.76.173] (port=55265 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M6J4R-0003vR-Ou for qemu-devel@nongnu.org; Tue, 19 May 2009 02:45:03 -0400 Received: from mx20.gnu.org ([199.232.41.8]:49952) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1M6J4R-0007eF-6h for qemu-devel@nongnu.org; Tue, 19 May 2009 02:45:03 -0400 Received: from mx2.redhat.com ([66.187.237.31]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M6J4Q-0002ct-1k for qemu-devel@nongnu.org; Tue, 19 May 2009 02:45:02 -0400 Subject: Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0 References: <1242678676-19439-1-git-send-email-ehabkost@redhat.com> <20090518221705.GO2079@blackpad> From: Markus Armbruster Date: Tue, 19 May 2009 08:44:47 +0200 In-Reply-To: (malc's message of "Tue\, 19 May 2009 04\:17\:55 +0400 \(MSD\)") Message-ID: <8763fxvbfk.fsf@pike.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: malc Cc: Eduardo Habkost , qemu-devel@nongnu.org malc writes: > On Mon, 18 May 2009, Eduardo Habkost wrote: > >> On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote: >> > On Mon, 18 May 2009, Eduardo Habkost wrote: >> > >> > > This patch is similar to a previous qemu_realloc() fix >> > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc(). >> > > >> > > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on >> > > this case. >> > [...] >> > Standard (in 7.20.3) says that malloc's behaviour in case of size being >> > zero is implementation defined. >> > >> > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'. >> > >> > The best(only?) thing to do is to check size passed to qemu_malloc[z] >> > and abort the program if this situation is encountered. >> >> Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL >> or a pointer, and on any case the value can be safely passed to free() >> later. > > I believe you haven't examined the commit that i referenced. Thing is Well, I just did, but I don't get it. diff --git a/block-qcow2.c b/block-qcow2.c index 9aa7261..d4556ef 100644 --- a/block-qcow2.c +++ b/block-qcow2.c @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs) int64_t offset; uint32_t extra_data_size; + if (!s->nb_snapshots) { + s->snapshots = NULL; + s->snapshots_size = 0; + return 0; + } + offset = s->snapshots_offset; s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot)); if (!s->snapshots) Can't see what this hunk accomplishes. If we remove it, the loop rejects, and we thus execute: offset = s->snapshots_offset; s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot)); s->snapshots_size = offset - s->snapshots_offset; return 0; Next hunk: @@ -2023,8 +2029,10 @@ static int qcow_snapshot_create(BlockDriverState *bs, snapshots1 = qemu_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot)); if (!snapshots1) goto fail; - memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot)); - qemu_free(s->snapshots); + if (s->snapshots) { + memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot) ); + qemu_free(s->snapshots); + } s->snapshots = snapshots1; s->snapshots[s->nb_snapshots++] = *sn; Again, I can't see the point. if !s->snapshots, then s->nb_snapshots == 0, doesn't it? memcpy(snapshots1, NULL, 0) does nothing. free(NULL) does nothing. I'm sure you didn't patch this for nothing, so I must miss something. Could you enlighten me? > existing code used to, i'd venture a guess accidentaly, rely on the > behaviour that current GLIBC provides and consequently failed to > operate on AIX where malloc(0) returns NULL, Common error. > IOW making qemu_malloc[z] > return whatever the underlying system returns is just hiding the bugs, > the code becomes unportable. Matter of taste. 1. Deal with the implementation-definedness. Every caller that could pass zero needs to take care not to confuse empty allocation with an out of memory condition. This is easier than it sounds when you check for out of memory in just one place, like we do. 2. Remove the implementation-definedness. Easiest way is to detect zero size in a wrapper (for us: qemu_malloc()) and bump it to one. qemu_realloc() currently uses 1. realloc(NULL, sz) is specified to be equivalent to malloc(sz). It would be kind of nice to keep that for qemu_realloc() and qemu_malloc().