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

[Markus Armbruster]

malc <address@hidden> writes:

> On Mon, 7 Dec 2009, Markus Armbruster wrote:
>
>> 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.
>
> I bet you dollars to donuts that testing for zero before calling malloc
> is cheaper than the eventual test that is done inside it.

Testing for zero before calling malloc() doesn't make the eventual test
that is done inside it go away, so it carries an additional cost.  What
you save by it is the cost of actual zero allocation.  Weight the two by
frequency and compare.

But once again, I doubt the performance impact is relevant.

>> > 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.
>
> Yes, it's better than having _silent_ bugs, which, furthermore, have a
> good potential of mainfesting themselves a lot further from the "bug"
> site.
>
>> 
>> 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?
>
> I wont dispute that.

Good.

>> 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.
[...]

[On one specific example where malloc(0) is a sign of something weird
going on:]
>> My point isn't that permitting malloc(0) is the best way to handle it.
>> It's a better way than aborting, though.
>
> And this is precisely the gist of our disagreement.

Yup.  When a feature can be used in a safe manner, but its use can
sometimes also be a sign of a bug, then I prefer the program to take its
chances and continue, while you prefer to stop the program cold, so you
can eliminate these scary uses as they occur.  Either choice can
conceivably lead to grief.  We disagree on likelihood and severety of
the grief to expect.

[...]
>> > http://groups.google.com/group/comp.std.c/browse_thread/thread/4e9af8847613d71f/6f75ad22e0768a0b?q=realloc++group:comp.std.c#6f75ad22e0768a0b
>> 
>> --verbose ?
>
> Sigh...
>
> C90 - realloc(non-null, 0) =
>       free(non-null); return NULL;
>
> C99 - realloc(non-null, 0) =
>       free(non-null); return realloc(NULL, 0); 
>
> GLIBC 2.7 = C90

glibc frees since 1999.  From glibc/ChangeLog.10:

1999-04-28  Andreas Jaeger  <address@hidden>

        * malloc/malloc.c (REALLOC_ZERO_BYTES_FREES): Define it to follow
        ISO C9x and Unix98.

> How anyone can use this interface and it's implementations portably
> or "sanely" is beyond me.
>
> Do read the discussion the linked above.

Yes, malloc() & friends are tricky for portable programs.  And yes,
bugs do creep into international standards.

But since we wrap malloc() & friends anyway, we can pick a variant of
the semantics we like.  No need to punish ourselves with wrappers that
are hard to use, just because the wrappees are hard to use.

Anyway, I figure this thread has wandered off topic for this list.