From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1M6QJY-0007za-CN for qemu-devel@nongnu.org; Tue, 19 May 2009 10:29:08 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1M6QJT-0007sa-Js for qemu-devel@nongnu.org; Tue, 19 May 2009 10:29:07 -0400 Received: from [199.232.76.173] (port=39549 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1M6QJT-0007sM-Ew for qemu-devel@nongnu.org; Tue, 19 May 2009 10:29:03 -0400 Received: from mx2.redhat.com ([66.187.237.31]:33151) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1M6QJS-0008HO-T0 for qemu-devel@nongnu.org; Tue, 19 May 2009 10:29:03 -0400 Date: Tue, 19 May 2009 11:28:47 -0300 From: Eduardo Habkost Subject: Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0 Message-ID: <20090519142847.GC4254@blackpad> References: <1242678676-19439-1-git-send-email-ehabkost@redhat.com> <20090518221705.GO2079@blackpad> <8763fxvbfk.fsf@pike.pond.sub.org> <87octpnrhr.fsf@pike.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: malc Cc: Markus Armbruster , qemu-devel@nongnu.org On Tue, May 19, 2009 at 06:06:56PM +0400, malc wrote: > On Tue, 19 May 2009, Markus Armbruster wrote: > > > malc writes: > > > > [..snip..] > > > >> 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: > > >> > > Once again, on Linux/GLIBC it will, on AIX it wont. Why not? It will. If nb_snapshots is 0, it won't enter the loop. The problem with that code was the "if (!s->snapshots)" check, not the qemu_mallocz(0) call. > > And FWIW despite behaviour of malloc(0) being marked as implementation > defined i have sa far was unable to find any documentaiton (Linux man > pages, GLIBC info files) witht the actual definition, unlike on AIX > where man pages make it crystal clear what happens. You don't need to have the exact behavior defined, as long as: 1) You call free(p) later 2) You don't dereference the returned pointer (just like you can't dereference p[n] on a malloc(n) block) 3) You don't assume anything about the returned value when size==0 My point is that this is valid malloc() usage, and there may be existing qemu code relying on that, and I don't see any reason to put a trap for code that would be valid malloc()/free() usage. > > > > > > 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? > > That's legitimate only if one writes unportable code targeting single > system and knowing how it was defined. No, that's legitimate and portable. You just can't assume anything about the returned value. > As for being dangerous, yes it > is: dereferencing the returned pointer, while UB, doesn't trigger a > SEGFAULT on, at least, this machine with Linux. > > > >> qemu_realloc() currently uses 1. > > void *qemu_realloc(void *ptr, size_t size) > { > if (size) > return oom_check(realloc(ptr, size)); > else > return realloc(ptr, size); > } > > There is nothing implementation defined about realloc(whatever, 0), it > has a defined meaning in POSIX: > http://opengroup.org/onlinepubs/007908775/xsh/realloc.html > > So it doesn't use 1. > realloc() return value is specified exactly the same way malloc() is: "If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() is returned." > > >> > > >> 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. > > Huh, that's not at all what i proposed. What i had in mind is: > > void *qemu_malloc(size_t size) > { > if (!size) abort(); > return oom_check(malloc(size)); > } Understood. And that's exactly what I think we should not do. -- Eduardo