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

[malc <av1474@comtv.ru>]

On Sat, 5 Dec 2009, Markus Armbruster wrote:

> Anthony Liguori <anthony@codemonkey.ws> 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

-- 
mailto:av1474@comtv.ru