malc <av1...@comtv.ru> writes: > On Sat, 5 Dec 2009, Markus Armbruster wrote: > >> Anthony Liguori <anth...@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
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.