Re: [Qemu-devel] [PATCH] Permit zero-sized qemu_malloc() & friends

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> Markus Armbruster wrote:
>> 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.
>>   
>
> I still believe that it is poor practice to pass size==0 to *malloc().
> I think actively discouraging this in qemu is a good thing because
> it's a broken idiom.

What's the impact of such usage?  What would improve for users if it
were eradicated?  For developers?

I believe the answer the first two questions is "nothing in particular",
and the answer to the last one is "hassle".  But I'd be happy to see
*specific* examples (code, not hand-waving) for where I'm wrong.

I'm opposed to "fixing" something without a real gain, not just because
it's work, but also because like any change it's going to introduce new
bugs.

I'm particularly critical of outlawing zero size for uses like
malloc(N*sizeof(T).  These are fairly common in our code.  Having to add
special treatment for N==0 is a trap for the unwary.  Which means we'll
never be done chasing this stuff.  Moreover, the special treatment
clutters the code, and requiring it without a clear benefit is silly.
Show me the benefit, and I'll happily reconsider.

For a specific example, let's look at the first example from my commit
message again: load_elf32(), load_elf64() in hw/elf_ops.h.  "Fix" for
its "broken" usage of qemu_mallocz(), shot from the hip, entirely
untested:

diff --git a/hw/elf_ops.h b/hw/elf_ops.h
index 6093dea..003d2ef 100644
--- a/hw/elf_ops.h
+++ b/hw/elf_ops.h
@@ -219,51 +219,53 @@ static int glue(load_elf, SZ)(const char *name, int fd, 
int64_t address_offset,
 
     glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
 
-    size = ehdr.e_phnum * sizeof(phdr[0]);
-    lseek(fd, ehdr.e_phoff, SEEK_SET);
-    phdr = qemu_mallocz(size);
-    if (!phdr)
-        goto fail;
-    if (read(fd, phdr, size) != size)
-        goto fail;
-    if (must_swab) {
-        for(i = 0; i < ehdr.e_phnum; i++) {
-            ph = &phdr[i];
-            glue(bswap_phdr, SZ)(ph);
-        }
-    }
-
-    total_size = 0;
-    for(i = 0; i < ehdr.e_phnum; i++) {
-        ph = &phdr[i];
-        if (ph->p_type == PT_LOAD) {
-            mem_size = ph->p_memsz;
-            /* XXX: avoid allocating */
-            data = qemu_mallocz(mem_size);
-            if (ph->p_filesz > 0) {
-                if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
-                    goto fail;
-                if (read(fd, data, ph->p_filesz) != ph->p_filesz)
-                    goto fail;
+    if (ehdr.e_phnum) {
+        size = ehdr.e_phnum * sizeof(phdr[0]);
+        lseek(fd, ehdr.e_phoff, SEEK_SET);
+        phdr = qemu_mallocz(size);
+        if (!phdr)
+            goto fail;
+        if (read(fd, phdr, size) != size)
+            goto fail;
+        if (must_swab) {
+            for(i = 0; i < ehdr.e_phnum; i++) {
+                ph = &phdr[i];
+                glue(bswap_phdr, SZ)(ph);
             }
-            /* address_offset is hack for kernel images that are
-               linked at the wrong physical address.  */
-            addr = ph->p_paddr + address_offset;
+        }
 
-            snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
-            rom_add_blob_fixed(label, data, mem_size, addr);
+        total_size = 0;
+        for(i = 0; i < ehdr.e_phnum; i++) {
+            ph = &phdr[i];
+            if (ph->p_type == PT_LOAD) {
+                mem_size = ph->p_memsz;
+                if (memsize) {
+                    data = qemu_mallocz(mem_size);
+                    if (ph->p_filesz > 0) {
+                        if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
+                            goto fail;
+                        if (read(fd, data, ph->p_filesz) != ph->p_filesz)
+                            goto fail;
+                    }
+                    /* address_offset is hack for kernel images that are
+                       linked at the wrong physical address.  */
+                    addr = ph->p_paddr + address_offset;
 
-            total_size += mem_size;
-            if (addr < low)
-                low = addr;
-            if ((addr + mem_size) > high)
-                high = addr + mem_size;
+                    snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
+                    rom_add_blob_fixed(label, data, mem_size, addr);
 
-            qemu_free(data);
-            data = NULL;
+                    total_size += mem_size;
+                    qemu_free(data);
+                    data = NULL;
+                }
+                if (addr < low)
+                    low = addr;
+                if ((addr + mem_size) > high)
+                    high = addr + mem_size;
+            }
         }
+        qemu_free(phdr);
     }
-    qemu_free(phdr);
     if (lowaddr)
         *lowaddr = (uint64_t)(elf_sword)low;
     if (highaddr)

Anyone fancy to review a hundred or two of these?

And what would it buy us?  Let's see what the old code does:

* It mallocs a buffer for the PHT.

* It seeks to the PHT, and reads it.

* It maybe byte-swaps the PHT.

* It iterates over the PHT to load segments.

Works just fine for empty PHTs and empty segments.

The new code does slightly less work for empty PHTs and segments (rare
case), and very slightly more work for non-empty ones (common case).
Neither of it matters.  What matters is that it is nested two levels
deeper.

Benefit?

> That said, we've had this change in for a while and it's painfully
> obviously we haven't eliminated all of these instances in qemu.
> Knowing that we still have occurrences of this and actively exit()'ing
> when they happen is pretty much suicidal in a production environment.

Agreed.

> It's not a bug, it's just a poor practice.
>
> Here's what I propose.  I think we should introduce a
> CONFIG_DEBUG_ZERO_MALLOC.  When this is set, we should
> assert(size!=0).  Otherwise, we should return malloc(1) as Markus'
> patch does below.
>
> Using the same rules as we follow for -Werror, we should enable
> CONFIG_DEBUG_ZERO_MALLOC for the development tree and disable it for
> any releases.
>
> This helps us make qemu better during development while not
> unnecessarily causing us harm in a production environment.  What
> happens here long term I think remains to be seen.  But for right now,
> I think this is an important change to make for 0.12.

We'll clutter the code, and we'll forever chase the uses of malloc()
that haven't received their clutter, yet.  CONFIG_DEBUG_ZERO_MALLOC will
exist forever, and be universally off in production builds.

I'd like to see some real benefit for that, please.