From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M6PVt-00082f-90 for qemu-devel@nongnu.org; Tue, 19 May 2009 09:37:49 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M6PVm-00081R-7w for qemu-devel@nongnu.org; Tue, 19 May 2009 09:37:46 -0400 Received: from [199.232.76.173] (port=39446 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M6PVk-00081F-W1 for qemu-devel@nongnu.org; Tue, 19 May 2009 09:37:41 -0400 Received: from mx2.redhat.com ([66.187.237.31]:39889) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M6PVk-0007gN-8N for qemu-devel@nongnu.org; Tue, 19 May 2009 09:37:40 -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> <8763fxvbfk.fsf@pike.pond.sub.org> From: Markus Armbruster Date: Tue, 19 May 2009 15:37:20 +0200 In-Reply-To: (malc's message of "Tue\, 19 May 2009 17\:00\:27 +0400 \(MSD\)") Message-ID: <87octpnrhr.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 Tue, 19 May 2009, Markus Armbruster wrote: > >> 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. > > Pretty good sign that something is wrong, no? That's why I'm asking :) >> 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; > > Uh, no. > > $ git cat-file -p 63c75dcd669d011f438421980b4379827da4bb1c^:block-qcow2.c | sed -n 1811,1815p > offset = s->snapshots_offset; > s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot)); > if (!s->snapshots) > goto fail; > > So before the patch qemu_mallocz(0) call would return NULL, goto fail > would be executed and `qcow_read_snapshots' will return -1 and QEMU > will fail to start with very helpful message: > qemu: could not open disk image The test went away in commit 3ec88e80. The hunk became pointless only then. >> 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. > > Wish i had your confidence and analysis skills, after skimming over > 2666 lines of C in block-qcow2.c i can safely assert that this is > indeed the only condition under which s->snapshots can be NULL, hence > this hunk. > >> I'm sure you didn't patch this for nothing, so I must miss something. >> Could you enlighten me? >> > > I hope that the above explanation is good enough, please do tell if it's not. Thanks. > Also please notice how throwing second implemenation-defined behaviour of > malloc in to the mix led to the hunk #2, in which you see no point. > >> > 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. > > Made by a very skilled programmer i might add. I've fallen into this trap myself. >> > 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. > > And mine: > 3. Abort the program if somebody tries it. Because so far history thought > me that nobody does 1. Tries what? Passing zero to qemu_malloc()? That's legitimate. And with allocation functions that cannot return failure, it's hardly dangerous, isn't it? >> 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(). >> > > qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part > about qemu_malloc escapes me. qemu_malloc() & friends never fail. Checking their value for failure is pointless. Therefore, 1. is practical. 2. is certainly practical as well. 3. is like 2, with the (size ? size : 1) pushed into callers. I find that mildly annoying. I don't care whether we pick 1., 2. or 3., but I'd prefer we pick the same for qemu_malloc() and qemu_realloc(). We currently use 1. for qemu_realloc(). My point is: if you pick something else for qemu_malloc(), please consider changing qemu_realloc(). That's all.