From patchwork Mon Nov 30 13:55:34 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 39803 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 74F1AB7082 for ; Tue, 1 Dec 2009 00:56:25 +1100 (EST) Received: from localhost ([127.0.0.1]:49360 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NF6jm-0004RJ-Eq for incoming@patchwork.ozlabs.org; Mon, 30 Nov 2009 08:56:22 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NF6jA-0004QQ-DL for qemu-devel@nongnu.org; Mon, 30 Nov 2009 08:55:44 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NF6j5-0004O3-N8 for qemu-devel@nongnu.org; Mon, 30 Nov 2009 08:55:44 -0500 Received: from [199.232.76.173] (port=49618 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NF6j5-0004Ni-Gt for qemu-devel@nongnu.org; Mon, 30 Nov 2009 08:55:39 -0500 Received: from mx1.redhat.com ([209.132.183.28]:14813) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NF6j4-0002ME-Ui for qemu-devel@nongnu.org; Mon, 30 Nov 2009 08:55:39 -0500 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id nAUDtasC012497 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 30 Nov 2009 08:55:36 -0500 Received: from crossbow.pond.sub.org (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id nAUDtZVP004910 for ; Mon, 30 Nov 2009 08:55:36 -0500 Received: by crossbow.pond.sub.org (Postfix, from userid 500) id 84B8F203B4; Mon, 30 Nov 2009 14:55:34 +0100 (CET) From: Markus Armbruster To: qemu-devel@nongnu.org Date: Mon, 30 Nov 2009 14:55:34 +0100 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.18 X-detected-operating-system: by monty-python.gnu.org: Genre and OS details not recognized. Subject: [Qemu-devel] [PATCH] Permit zero-sized qemu_malloc() & friends X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Commit a7d27b53 made zero-sized allocations a fatal error, deviating from ISO C's malloc() & friends. Revert that, but take care never to return a null pointer, like malloc() & friends may do (it's implementation defined), because that's another source of bugs. Rationale: while zero-sized allocations might occasionally be a sign of something going wrong, they can also be perfectly legitimate. The change broke such legitimate uses. We've found and "fixed" at least one of them already (commit eb0b64f7, also reverted by this patch), and another one just popped up: the change broke qcow2 images with virtual disk size zero, i.e. images that don't hold real data but only VM state of snapshots. If a change breaks two uses, it probably breaks more. As a quick check, I reviewed the first six of more than 200 uses of qemu_mallocz(), qemu_malloc() and qemu_realloc() that don't have an argument of the form sizeof(...) or similar: * load_elf32(), load_elf64() in hw/elf_ops.h: size = ehdr.e_phnum * sizeof(phdr[0]); lseek(fd, ehdr.e_phoff, SEEK_SET); phdr = qemu_mallocz(size); This aborts when the ELF file has no program header table, because then e_phnum is zero (TIS ELF 1.2 page 1-6). Even if we require the ELF file to have a program header table, aborting is not an acceptable way to reject an unsuitable or malformed ELF file. * load_elf32(), load_elf64() in hw/elf_ops.h: mem_size = ph->p_memsz; /* XXX: avoid allocating */ data = qemu_mallocz(mem_size); This aborts when the segment occupies zero bytes in the image file (TIS ELF 1.2 page 2-2). * bdrv_open2() in block.c: bs->opaque = qemu_mallocz(drv->instance_size); However, vvfat_write_target.instance_size is zero. Not sure whether this actually bites or is "only" a time bomb. * qemu_aio_get() in block.c: acb = qemu_mallocz(pool->aiocb_size); No existing instance of BlockDriverAIOCB has a zero aiocb_size. Probably okay. * defaultallocator_create_displaysurface() in console.c has surface->data = (uint8_t*) qemu_mallocz(surface->linesize * surface->height); With Xen, surface->linesize and surface->height come out of xenfb_configure_fb(), which set xenfb->width and xenfb->height to values obtained from the guest. If a buggy guest sets one to zero, we abort. Not an good way to handle that. Non-Xen uses aren't obviously correct either, but I didn't dig deeper. * load_device_tree() in device_tree.c: *sizep = 0; dt_size = get_image_size(filename_path); if (dt_size < 0) { printf("Unable to get size of device tree file '%s'\n", filename_path); goto fail; } /* Expand to 2x size to give enough room for manipulation. */ dt_size *= 2; /* First allocate space in qemu for device tree */ fdt = qemu_mallocz(dt_size); We abort if the image is empty. Not an acceptable way to handle that. Based on this sample, I'm confident there are many more uses broken by the change. Signed-off-by: Markus Armbruster Acked-by: Kevin Wolf Acked-by: Eduardo Habkost --- block/qcow2-snapshot.c | 5 ----- qemu-malloc.c | 14 ++------------ 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 94cb838..e3b208c 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) QCowSnapshot *sn; int i; - if (!s->nb_snapshots) { - *psn_tab = NULL; - return s->nb_snapshots; - } - sn_tab = qemu_mallocz(s->nb_snapshots * sizeof(QEMUSnapshotInfo)); for(i = 0; i < s->nb_snapshots; i++) { sn_info = sn_tab + i; diff --git a/qemu-malloc.c b/qemu-malloc.c index 295d185..aeeb78b 100644 --- a/qemu-malloc.c +++ b/qemu-malloc.c @@ -44,22 +44,12 @@ void qemu_free(void *ptr) void *qemu_malloc(size_t size) { - if (!size) { - abort(); - } - return oom_check(malloc(size)); + return oom_check(malloc(size ? size : 1)); } void *qemu_realloc(void *ptr, size_t size) { - if (size) { - return oom_check(realloc(ptr, size)); - } else { - if (ptr) { - return realloc(ptr, size); - } - } - abort(); + return oom_check(realloc(ptr, size ? size : 1)); } void *qemu_mallocz(size_t size)