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

[Markus Armbruster]

malc <address@hidden> writes:

> On Mon, 30 Nov 2009, 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.
>> 
>> 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:
>
> Bottom line: your point is that there are benign uses of zero allocation.
> There are, at the expense of memory consumption/fragmentation and useless
> code which, as your investigation shows, involve syscalls and what not.

I doubt the performance impact is relevant, but since you insist on
discussing it...

First and foremost, any performance argument not backed by measurements
is speculative.

Potential zero allocation is common in code.  Actual zero allocation is
rare at runtime.  The amount of memory consumed is therefore utterly
trivial compared to total dynamic memory use.  Likewise, time spent in
allocating is dwarved by time spent in other invocations of malloc()
several times over.

Adding a special case for avoiding zero allocation is not free.  Unless
zero allocations are sufficiently frequent, that cost exceeds the cost
of the rare zero allocation.

> Outlook from my side of the fence: no one audited the code, no one
> knows that all of the uses are benign, abort gives the best automatic
> way to know for sure one way or the other.
>
> Rationale for keeping it as is: zero-sized allocations - overwhelming
> majority of the times, are not anticipated and not well understood,
> aborting gives us the ability to eliminate them in an automatic
> fashion.

Keeping it as is releasing with known crash bugs, and a known pattern
for unknown crash bugs.  Is that what you want us to do?  I doubt it.

I grant you that there may be allocations we expect never to be empty,
and where things break when they are.  Would you concede that there are
allocations that work just fine when empty?

If you do, we end up with three kinds of allocations: known empty bad,
known empty fine, unknown.  Aborting on known empty bad is okay with me.
Aborting on unknown in developement builds is okay with me, too.  I
don't expect it to be a successful way to find bugs, because empty
allocations are rare.

Initially, all allocations should be treated as "unknown".  I want a way
to mark an allocation as "known empty fine".  I figure you want a way to
mark "known empty bad".

>> * 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.
>
> If there's a malformed ELF files that must be supported (and by supported
> - user notification is meant) then things should be checked, because having
> gargantuan size will force qemu_mallocz to abort via OOM check (even
> with Linux and overcommit, since malloc will fallback to mmap which
> will fail) and this argument falls apart.

ELF files with empty PHT are *not* malformed.  Empty PHT is explicitly
permitted by ELF TIS.  Likewise for empty segments.  I already gave you
chapter & verse.

>> * 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.
>
> What is? Let's suppose height is zero, without explicit checks, user
> will never know why his screen doesn't update, with abort he will at
> least know that something is very wrong.

My point isn't that permitting malloc(0) is the best way to handle it.
It's a better way than aborting, though.

I'm not arguing against treating case 0 specially ever.

>>   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.
>
> Based on this sample i'm confident, we can eventually fix them should those
> become the problem.

You broke working code.  I'm glad you're confident we can fix it
"eventually".  What about 0.12?  Ship it broken?

>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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));
>>  }
>
> http://groups.google.com/group/comp.std.c/browse_thread/thread/4e9af8847613d71f/6f75ad22e0768a0b?q=realloc++group:comp.std.c#6f75ad22e0768a0b

--verbose ?