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

[Markus Armbruster]

malc <address@hidden> writes:

> On Sat, 5 Dec 2009, Markus Armbruster wrote:
>
>> 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:
>
> Excellent example, now consider this:
>
>   read$ cat << eof | gcc -x c -
>   #define _GNU_SOURCE
>   #include <err.h>
>   #include <unistd.h>
>   #include <stdlib.h>
>   #include <fcntl.h>
>
>   int main (void)
>   {
>       int fd = open ("/dev/zero", 0);
>       int ret;
>       void *p = (void *) -1;
>
>       if (fd == -1) err (1, "open");
>       ret = read (fd, p, 0);
>       if (ret != 0) err (1, "read");
>       return 0;
>   }
>   eof
>   read$ ./a.out                
>   a.out: read: Bad address
>
> Even though that linux's read(2) man page claims [1]:
>
> DESCRIPTION
>        read()  attempts to read up to count bytes from file descriptor fd into
>        the buffer starting at buf.
>
>        If count is zero, read() returns zero and has  no  other  results.   If
>        count is greater than SSIZE_MAX, the result is unspecified.
>
> [..snip..]
>
> P.S. It would be interesting to know how this code behaves under OpenBSD, with
>      p = malloc (0);
>
> [1] As does, in essence, 
> http://www.opengroup.org/onlinepubs/7990989775/xsh/read.html

Replace "p = (void *)-1" by "p = NULL" and it works just fine.

malloc() either returns a valid pointer or NULL, so what read() does for
other pointers doesn't matter.

qemu_malloc() always returns a valid pointer, but that's not even needed
in this case.