| Message ID | m3zl5x1q5x.fsf@crossbow.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
Hello, probably a stupid remark, but I'll do it anyway :-) Let's assume there are some contexts where doing a memory allocation with a size of 0 is invalid while in some other contexts it is valid. Wouldn't it make sense in that case to have two functions that do memory allocation? Of course such a split would require full review of existing code and might introduce complexities for instance for realloc (was the original memory block allocated with a possible size of 0 or not?). Laurent
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
On 12/05/2009 07:08 PM, malc wrote: > >> 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 > > I don't see how this is relevant. Linux' read(2) may or may not be broken, but what does that have to do with breaking callers of qemu_malloc() for certain use cases?
On Sat, Dec 05, 2009 at 08:08:27PM +0300, malc wrote: > 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 Which simply means that the linux man page was based on SUSv2 which obviously was quite badly written. SUSv3 gets it right (proving along the way it is better to use the POSIX man pages, i.e. "man 3 read"). It might be good to suggest them to update the linux man page though.
malc <av1474@comtv.ru> writes: > 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 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.
On Sun, 6 Dec 2009, Markus Armbruster wrote: > malc <av1474@comtv.ru> writes: > > > 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. > >> >> > >> > [..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. > That's why i asked for somone to run it on OpenBSD: Quoting http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=3&manpath=OpenBSD+Current&arch=i386&format=html Allocation of a zero size object returns a pointer to a zero size object. This zero size object is access protected, so any access to it will gen- erate an exception (SIGSEGV). Many zero-sized objects can be placed con- secutively in shared protected pages. The minimum size of the protection on each object is suitably aligned and sized as previously stated, but the protection may extend further depending on where in a protected zone the object lands. > malloc() either returns a valid pointer or NULL, so what read() does for > other pointers doesn't matter. Replace "returns" with "should" and this still won't match the wording of the standard, besides as "read" behaviour on Linux shows following those are not high on the agenda. 7.20.3.3 The malloc function Synopsis [#1] #include <stdlib.h> void *malloc(size_t size); Description [#2] The malloc function allocates space for an object whose size is specified by size and whose value is indeterminate. Returns [#3] The malloc function returns either a null pointer or a pointer to the allocated space. I don't know what a "valid pointer" in this context represents. > qemu_malloc() always returns a valid pointer, but that's not even needed > in this case. > -- mailto:av1474@comtv.ru
malc <av1474@comtv.ru> writes: > On Sun, 6 Dec 2009, Markus Armbruster wrote: > >> malc <av1474@comtv.ru> writes: >> >> > 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. >> >> >> >> >> > > > [..snip..] > > >> > >> > P.S. It would be interesting to know how this code behaves under OpenBSD, with >> > p = malloc (0); [...] >> >> Replace "p = (void *)-1" by "p = NULL" and it works just fine. >> > > That's why i asked for somone to run it on OpenBSD: > > Quoting http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=3&manpath=OpenBSD+Current&arch=i386&format=html > > Allocation of a zero size object returns a pointer to a zero size object. > This zero size object is access protected, so any access to it will gen- > erate an exception (SIGSEGV). Many zero-sized objects can be placed con- > secutively in shared protected pages. The minimum size of the protection > on each object is suitably aligned and sized as previously stated, but > the protection may extend further depending on where in a protected zone > the object lands. read(fd, malloc(0), 0) is just fine, because read() doesn't touch the buffer when the size is zero. >> malloc() either returns a valid pointer or NULL, so what read() does for >> other pointers doesn't matter. > > Replace "returns" with "should" and this still won't match the wording > of the standard, besides as "read" behaviour on Linux shows following > those are not high on the agenda. > > 7.20.3.3 The malloc function > > Synopsis > > [#1] > > #include <stdlib.h> > void *malloc(size_t size); > > Description > > [#2] The malloc function allocates space for an object whose > size is specified by size and whose value is indeterminate. > > Returns > > [#3] The malloc function returns either a null pointer or a > pointer to the allocated space. > > I don't know what a "valid pointer" in this context represents. I can talk standardese, if you prefer :) malloc() either returns either a null pointer or a pointer to the allocated space. In either case, you must not dereference the pointer. OpenBSD chooses to return a pointer to the allocated space. It chooses to catch common ways to dereference the pointer. Your "p = (void *)-1" is neither a null pointer nor can it point to allocated space on your particular system. Hence, it cannot be a value of malloc() for any argument, and therefore what read() does with it on that particular system doesn't matter.
On Sun, Dec 6, 2009 at 10:39 AM, malc <av1474@comtv.ru> wrote: > On Sun, 6 Dec 2009, Markus Armbruster wrote: > >> malc <av1474@comtv.ru> writes: >> >> > 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. >> >> >> >> >> > > > [..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. >> > > That's why i asked for somone to run it on OpenBSD: $ cat mall.c #define _GNU_SOURCE #include <err.h> #include <unistd.h> #include <stdlib.h> #include <fcntl.h> #include <stdio.h> int main (void) { int fd = open ("/dev/zero", 0); int ret; #if 0 void *p = (void *) -1; #else void *p = malloc(0); #endif fprintf(stderr, "ptr %p\n", p); if (fd == -1) err (1, "open"); ret = read (fd, p, 0); if (ret != 0) err (1, "read"); return 0; } $ gcc mall.c $ ./a.out ptr 0x46974060 $ Changing read count to 1: $ ./a.out ptr 0x41ce0070 a.out: read: Bad address
On Sun, 6 Dec 2009, Blue Swirl wrote: > On Sun, Dec 6, 2009 at 10:39 AM, malc <av1474@comtv.ru> wrote: > > On Sun, 6 Dec 2009, Markus Armbruster wrote: [..snip..] > $ gcc mall.c > $ ./a.out > ptr 0x46974060 > $ > > Changing read count to 1: > $ ./a.out > ptr 0x41ce0070 > a.out: read: Bad address > Thanks.
On Sun, 6 Dec 2009, Markus Armbruster wrote: > malc <av1474@comtv.ru> writes: > [..snip..] > > read(fd, malloc(0), 0) is just fine, because read() doesn't touch the > buffer when the size is zero. > [..snip..] Yet under linux the address is checked even for zero case. > > > > I don't know what a "valid pointer" in this context represents. > > I can talk standardese, if you prefer :) > > malloc() either returns either a null pointer or a pointer to the > allocated space. In either case, you must not dereference the pointer. > > OpenBSD chooses to return a pointer to the allocated space. It chooses > to catch common ways to dereference the pointer. > > Your "p = (void *)-1" is neither a null pointer nor can it point to > allocated space on your particular system. Hence, it cannot be a value > of malloc() for any argument, and therefore what read() does with it on > that particular system doesn't matter. > Here, i believe, you are inventing artificial restrictions on how malloc behaves, i don't see anything that prevents the implementor from setting aside a range of addresses with 31st bit set as an indicator of "zero" allocations, and then happily giving it to the user of malloc and consumming it in free.
On 12/06/2009 12:22 PM, malc wrote: > Here, i believe, you are inventing artificial restrictions on how > malloc behaves, i don't see anything that prevents the implementor > from setting aside a range of addresses with 31st bit set as an > indicator of "zero" allocations, and then happily giving it to the > user of malloc and consumming it in free. > The implementation needs to track which addresses it handed out, since it is required that malloc(0) != malloc(0) (unless both are NULL).
malc <av1474@comtv.ru> writes: > On Sun, 6 Dec 2009, Markus Armbruster wrote: > >> malc <av1474@comtv.ru> writes: >> > > [..snip..] > >> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the >> buffer when the size is zero. >> > > [..snip..] > > Yet under linux the address is checked even for zero case. Any value you can obtain from malloc() passes that check. Why does the fact that you can construct pointers that don't pass this check matter for our discussion of malloc()? >> > I don't know what a "valid pointer" in this context represents. >> >> I can talk standardese, if you prefer :) >> >> malloc() either returns either a null pointer or a pointer to the >> allocated space. In either case, you must not dereference the pointer. >> >> OpenBSD chooses to return a pointer to the allocated space. It chooses >> to catch common ways to dereference the pointer. >> >> Your "p = (void *)-1" is neither a null pointer nor can it point to >> allocated space on your particular system. Hence, it cannot be a value >> of malloc() for any argument, and therefore what read() does with it on >> that particular system doesn't matter. >> > > Here, i believe, you are inventing artificial restrictions on how > malloc behaves, i don't see anything that prevents the implementor > from setting aside a range of addresses with 31st bit set as an > indicator of "zero" allocations, and then happily giving it to the > user of malloc and consumming it in free. Misunderstanding? Such behavior is indeed permissible, and I can't see where I restricted it away. An implementation that behaves as you describe returns "pointer to allocated space". That the pointer has some funny bit set doesn't matter. That it can't be dereferenced is just fine. I'm not sure what your point is. If it is that malloc(0) can return a value that cannot be passed to a zero-sized read(), then I fear you have not made your point.
On 12/06/2009 11:22 AM, malc wrote: > Here, i believe, you are inventing artificial restrictions on how > malloc behaves, i don't see anything that prevents the implementor > from setting aside a range of addresses with 31st bit set as an > indicator of "zero" allocations, and then happily giving it to the > user of malloc and consumming it in free. But it has to make it a valid address anyway. If a zero-sized read treats it as invalid (SIGSEGV, EFAULT, whatever), malloc has failed to return a valid address and is not obeying its specification. Paolo
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 12:22 PM, malc wrote: > > Here, i believe, you are inventing artificial restrictions on how > > malloc behaves, i don't see anything that prevents the implementor > > from setting aside a range of addresses with 31st bit set as an > > indicator of "zero" allocations, and then happily giving it to the > > user of malloc and consumming it in free. > > > > The implementation needs to track which addresses it handed out, since it is > required that malloc(0) != malloc(0) (unless both are NULL). You haven't read carefully, i said range.
On Sun, 6 Dec 2009, Markus Armbruster wrote: > malc <av1474@comtv.ru> writes: > > > On Sun, 6 Dec 2009, Markus Armbruster wrote: > > > >> malc <av1474@comtv.ru> writes: > >> > > > > [..snip..] > > > >> > >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the > >> buffer when the size is zero. > >> > > > > [..snip..] > > > > Yet under linux the address is checked even for zero case. > > Any value you can obtain from malloc() passes that check. > > Why does the fact that you can construct pointers that don't pass this > check matter for our discussion of malloc()? > > >> > I don't know what a "valid pointer" in this context represents. > >> > >> I can talk standardese, if you prefer :) > >> > >> malloc() either returns either a null pointer or a pointer to the > >> allocated space. In either case, you must not dereference the pointer. > >> > >> OpenBSD chooses to return a pointer to the allocated space. It chooses > >> to catch common ways to dereference the pointer. > >> > >> Your "p = (void *)-1" is neither a null pointer nor can it point to > >> allocated space on your particular system. Hence, it cannot be a value > >> of malloc() for any argument, and therefore what read() does with it on > >> that particular system doesn't matter. > >> > > > > Here, i believe, you are inventing artificial restrictions on how > > malloc behaves, i don't see anything that prevents the implementor > > from setting aside a range of addresses with 31st bit set as an > > indicator of "zero" allocations, and then happily giving it to the > > user of malloc and consumming it in free. > > Misunderstanding? Such behavior is indeed permissible, and I can't see > where I restricted it away. An implementation that behaves as you > describe returns "pointer to allocated space". That the pointer has > some funny bit set doesn't matter. That it can't be dereferenced is > just fine. > > I'm not sure what your point is. If it is that malloc(0) can return a > value that cannot be passed to a zero-sized read(), then I fear you have > not made your point. One more attempt to make it clearer. If you agree that this behaviour is permissible then the game is lost as things stand now under Linux, since replacing [1]: void *p = (void *) -1 with: void *p = (void *) 0x80000000 or anything else with said bit set will yield EFAULT. Consequently the code you cited as a well behaving malloc(0) call site will bomb. [1] Under 32bit Linux that is, with the usual split.
On Sun, 6 Dec 2009, Paolo Bonzini wrote: > On 12/06/2009 11:22 AM, malc wrote: > > Here, i believe, you are inventing artificial restrictions on how > > malloc behaves, i don't see anything that prevents the implementor > > from setting aside a range of addresses with 31st bit set as an > > indicator of "zero" allocations, and then happily giving it to the > > user of malloc and consumming it in free. > > But it has to make it a valid address anyway. If a zero-sized read treats it > as invalid (SIGSEGV, EFAULT, whatever), malloc has failed to return a valid > address and is not obeying its specification. Once again - standard doesn't speak about "valid addresses".
On 12/06/2009 01:53 PM, malc wrote: > On Sun, 6 Dec 2009, Avi Kivity wrote: > > >> On 12/06/2009 12:22 PM, malc wrote: >> >>> Here, i believe, you are inventing artificial restrictions on how >>> malloc behaves, i don't see anything that prevents the implementor >>> from setting aside a range of addresses with 31st bit set as an >>> indicator of "zero" allocations, and then happily giving it to the >>> user of malloc and consumming it in free. >>> >>> >> The implementation needs to track which addresses it handed out, since it is >> required that malloc(0) != malloc(0) (unless both are NULL). >> > You haven't read carefully, i said range. > I did in fact. Consider a loop malloc(0); p = malloc(0); while (1) { n = malloc(0); free(p); p = n; } without some form of tracking, you won't be able to return unique addresses eventually.
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 01:53 PM, malc wrote: > > On Sun, 6 Dec 2009, Avi Kivity wrote: > > > > > > > On 12/06/2009 12:22 PM, malc wrote: > > > > > > > Here, i believe, you are inventing artificial restrictions on how > > > > malloc behaves, i don't see anything that prevents the implementor > > > > from setting aside a range of addresses with 31st bit set as an > > > > indicator of "zero" allocations, and then happily giving it to the > > > > user of malloc and consumming it in free. > > > > > > > > > > > The implementation needs to track which addresses it handed out, since it > > > is > > > required that malloc(0) != malloc(0) (unless both are NULL). > > > > > You haven't read carefully, i said range. > > > > I did in fact. Consider a loop > > malloc(0); > p = malloc(0); > while (1) { > n = malloc(0); > free(p); > p = n; > } > > without some form of tracking, you won't be able to return unique addresses > eventually. So? There will be tracking, it's the same as with OpenBSD where allocations of zero and greater than zero are handled differently.
On 12/06/2009 02:11 PM, malc wrote: >>>> The implementation needs to track which addresses it handed out, since it >>>> is >>>> required that malloc(0) != malloc(0) (unless both are NULL). >>>> >>>> >>> You haven't read carefully, i said range. >>> >>> >> > So? There will be tracking, it's the same as with OpenBSD where > allocations of zero and greater than zero are handled differently. > That's exactly what I said. Some tracking is needed.
On 12/06/2009 01:02 PM, malc wrote: > On Sun, 6 Dec 2009, Paolo Bonzini wrote: > >> On 12/06/2009 11:22 AM, malc wrote: >>> Here, i believe, you are inventing artificial restrictions on how >>> malloc behaves, i don't see anything that prevents the implementor >>> from setting aside a range of addresses with 31st bit set as an >>> indicator of "zero" allocations, and then happily giving it to the >>> user of malloc and consumming it in free. >> >> But it has to make it a valid address anyway. If a zero-sized read treats it >> as invalid (SIGSEGV, EFAULT, whatever), malloc has failed to return a valid >> address and is not obeying its specification. > > Once again - standard doesn't speak about "valid addresses". For that matter, POSIX doesn't mention EFAULT at all, and doesn't include detecting "valid addresses" among the things that read can do before returning 0. So if an OS extends POSIX with EFAULT, it had better provide a malloc that is consistent with whatever definition of "valid address" EFAULT uses. While if it doesn't provide EFAULT, read should return 0 for the OS to be conforming to POSIX, and the whole discussion is moot. Paolo
On 12/06/2009 01:00 PM, malc wrote: > or anything else with said bit set will yield EFAULT. Consequently the > code you cited as a well behaving malloc(0) call site will bomb. It is not well-behaving unless the definition of EFAULT is changed consistently. Paolo
Am 06.12.2009 13:00, schrieb malc: > On Sun, 6 Dec 2009, Markus Armbruster wrote: > >> malc <av1474@comtv.ru> writes: >> >>> On Sun, 6 Dec 2009, Markus Armbruster wrote: >>> >>>> malc <av1474@comtv.ru> writes: >>>> >>> >>> [..snip..] >>> >>>> >>>> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the >>>> buffer when the size is zero. >>>> >>> >>> [..snip..] >>> >>> Yet under linux the address is checked even for zero case. >> >> Any value you can obtain from malloc() passes that check. >> >> Why does the fact that you can construct pointers that don't pass this >> check matter for our discussion of malloc()? >> >>>>> I don't know what a "valid pointer" in this context represents. >>>> >>>> I can talk standardese, if you prefer :) >>>> >>>> malloc() either returns either a null pointer or a pointer to the >>>> allocated space. In either case, you must not dereference the pointer. >>>> >>>> OpenBSD chooses to return a pointer to the allocated space. It chooses >>>> to catch common ways to dereference the pointer. >>>> >>>> Your "p = (void *)-1" is neither a null pointer nor can it point to >>>> allocated space on your particular system. Hence, it cannot be a value >>>> of malloc() for any argument, and therefore what read() does with it on >>>> that particular system doesn't matter. >>>> >>> >>> Here, i believe, you are inventing artificial restrictions on how >>> malloc behaves, i don't see anything that prevents the implementor >>> from setting aside a range of addresses with 31st bit set as an >>> indicator of "zero" allocations, and then happily giving it to the >>> user of malloc and consumming it in free. >> >> Misunderstanding? Such behavior is indeed permissible, and I can't see >> where I restricted it away. An implementation that behaves as you >> describe returns "pointer to allocated space". That the pointer has >> some funny bit set doesn't matter. That it can't be dereferenced is >> just fine. >> >> I'm not sure what your point is. If it is that malloc(0) can return a >> value that cannot be passed to a zero-sized read(), then I fear you have >> not made your point. > > One more attempt to make it clearer. If you agree that this behaviour > is permissible then the game is lost as things stand now under Linux, > since replacing [1]: > > void *p = (void *) -1 > with: > void *p = (void *) 0x80000000 Then this implementation of malloc(0) isn't suitable for a libc running on Linux - despite its general permissibility. You have a point if, and only if, you can reproduce such misbehaviour with a real malloc of a real libc that isn't constructed to work against the kernel but to fit it. So far I can't see that you have found any. By the way, this whole discussion about theoretical malloc(0) return values doesn't even matter here. The patch replaces it by malloc(1), with which we both are very confident that you can pass its return value to read for reading 0 bytes, right? Kevin
malc <av1474@comtv.ru> writes: > On Sun, 6 Dec 2009, Markus Armbruster wrote: > >> malc <av1474@comtv.ru> writes: >> >> > On Sun, 6 Dec 2009, Markus Armbruster wrote: >> > >> >> malc <av1474@comtv.ru> writes: >> >> >> > >> > [..snip..] >> > >> >> >> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the >> >> buffer when the size is zero. >> >> >> > >> > [..snip..] >> > >> > Yet under linux the address is checked even for zero case. >> >> Any value you can obtain from malloc() passes that check. >> >> Why does the fact that you can construct pointers that don't pass this >> check matter for our discussion of malloc()? >> >> >> > I don't know what a "valid pointer" in this context represents. >> >> >> >> I can talk standardese, if you prefer :) >> >> >> >> malloc() either returns either a null pointer or a pointer to the >> >> allocated space. In either case, you must not dereference the pointer. >> >> >> >> OpenBSD chooses to return a pointer to the allocated space. It chooses >> >> to catch common ways to dereference the pointer. >> >> >> >> Your "p = (void *)-1" is neither a null pointer nor can it point to >> >> allocated space on your particular system. Hence, it cannot be a value >> >> of malloc() for any argument, and therefore what read() does with it on >> >> that particular system doesn't matter. >> >> >> > >> > Here, i believe, you are inventing artificial restrictions on how >> > malloc behaves, i don't see anything that prevents the implementor >> > from setting aside a range of addresses with 31st bit set as an >> > indicator of "zero" allocations, and then happily giving it to the >> > user of malloc and consumming it in free. >> >> Misunderstanding? Such behavior is indeed permissible, and I can't see >> where I restricted it away. An implementation that behaves as you >> describe returns "pointer to allocated space". That the pointer has >> some funny bit set doesn't matter. That it can't be dereferenced is >> just fine. >> >> I'm not sure what your point is. If it is that malloc(0) can return a >> value that cannot be passed to a zero-sized read(), then I fear you have >> not made your point. > > One more attempt to make it clearer. If you agree that this behaviour > is permissible then the game is lost as things stand now under Linux, > since replacing [1]: > > void *p = (void *) -1 > with: > void *p = (void *) 0x80000000 > > or anything else with said bit set will yield EFAULT. Consequently the > code you cited as a well behaving malloc(0) call site will bomb. > > [1] Under 32bit Linux that is, with the usual split. You can't just pull pointers out of your ear and expect stuff to work. malloc() is free to return a pointer to allocated space that is set up in a way that catches access beyond the allocated size. OpenBSD does that for size zero; it allocates one byte then, from pages that are used only for zero-sized allocations, and takes care to disable access to these pages with mprotect(..., PROT_NONE)[*]. Since read(..., 0) does not access beyond the allocated size, it still works just fine. If you replace glibc's malloc() to get OpenBSD-like behavior, you can't just make up some pointer to a memory area you believe to be unused, you have to do it right, like OpenBSD does. [*] Check out omalloc_make_chunks() at http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/malloc.c?rev=1.121;content-type=text%2Fplain
On Mon, 7 Dec 2009, Markus Armbruster wrote: > malc <av1474@comtv.ru> writes: > > > On Sun, 6 Dec 2009, Markus Armbruster wrote: > > > >> malc <av1474@comtv.ru> writes: > >> > >> > On Sun, 6 Dec 2009, Markus Armbruster wrote: > >> > > >> >> malc <av1474@comtv.ru> writes: > >> >> > >> > > >> > [..snip..] > >> > > >> >> > >> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the > >> >> buffer when the size is zero. > >> >> > >> > > >> > [..snip..] > >> > > >> > Yet under linux the address is checked even for zero case. > >> > >> Any value you can obtain from malloc() passes that check. > >> > >> Why does the fact that you can construct pointers that don't pass this > >> check matter for our discussion of malloc()? > >> > >> >> > I don't know what a "valid pointer" in this context represents. > >> >> > >> >> I can talk standardese, if you prefer :) > >> >> > >> >> malloc() either returns either a null pointer or a pointer to the > >> >> allocated space. In either case, you must not dereference the pointer. > >> >> > >> >> OpenBSD chooses to return a pointer to the allocated space. It chooses > >> >> to catch common ways to dereference the pointer. > >> >> > >> >> Your "p = (void *)-1" is neither a null pointer nor can it point to > >> >> allocated space on your particular system. Hence, it cannot be a value > >> >> of malloc() for any argument, and therefore what read() does with it on > >> >> that particular system doesn't matter. > >> >> > >> > > >> > Here, i believe, you are inventing artificial restrictions on how > >> > malloc behaves, i don't see anything that prevents the implementor > >> > from setting aside a range of addresses with 31st bit set as an > >> > indicator of "zero" allocations, and then happily giving it to the > >> > user of malloc and consumming it in free. > >> > >> Misunderstanding? Such behavior is indeed permissible, and I can't see > >> where I restricted it away. An implementation that behaves as you > >> describe returns "pointer to allocated space". That the pointer has > >> some funny bit set doesn't matter. That it can't be dereferenced is > >> just fine. > >> Here you agree that it's permissible. > >> I'm not sure what your point is. If it is that malloc(0) can return a > >> value that cannot be passed to a zero-sized read(), then I fear you have > >> not made your point. > > > > One more attempt to make it clearer. If you agree that this behaviour > > is permissible then the game is lost as things stand now under Linux, > > since replacing [1]: > > > > void *p = (void *) -1 > > with: > > void *p = (void *) 0x80000000 > > > > or anything else with said bit set will yield EFAULT. Consequently the > > code you cited as a well behaving malloc(0) call site will bomb. > > > > [1] Under 32bit Linux that is, with the usual split. > > You can't just pull pointers out of your ear and expect stuff to work. And here you don't. Which renders whole discussion rather pointless. > > malloc() is free to return a pointer to allocated space that is set up > in a way that catches access beyond the allocated size. OpenBSD does > that for size zero; it allocates one byte then, from pages that are used > only for zero-sized allocations, and takes care to disable access to > these pages with mprotect(..., PROT_NONE)[*]. Since read(..., 0) does > not access beyond the allocated size, it still works just fine. > > If you replace glibc's malloc() to get OpenBSD-like behavior, you can't > just make up some pointer to a memory area you believe to be unused, you > have to do it right, like OpenBSD does. > > > [*] Check out omalloc_make_chunks() at > http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/malloc.c?rev=1.121;content-type=text%2Fplain >
Am 07.12.2009 11:00, schrieb malc: >>>> Misunderstanding? Such behavior is indeed permissible, and I can't see >>>> where I restricted it away. An implementation that behaves as you >>>> describe returns "pointer to allocated space". That the pointer has >>>> some funny bit set doesn't matter. That it can't be dereferenced is >>>> just fine. >>>> > > Here you agree that it's permissible. He's just differentiating between values that are generally permissible in terms of the C standard... >> You can't just pull pointers out of your ear and expect stuff to work. > > And here you don't. Which renders whole discussion rather pointless. ...and values that can be used by a working implementation on Linux. That's quite a difference. It is obvious that a libc implementation must match the kernel, or nothing will work. Kevin
malc <av1474@comtv.ru> writes: > On Mon, 7 Dec 2009, Markus Armbruster wrote: > >> malc <av1474@comtv.ru> writes: >> >> > On Sun, 6 Dec 2009, Markus Armbruster wrote: >> > >> >> malc <av1474@comtv.ru> writes: >> >> >> >> > On Sun, 6 Dec 2009, Markus Armbruster wrote: >> >> > >> >> >> malc <av1474@comtv.ru> writes: >> >> >> >> >> > >> >> > [..snip..] >> >> > >> >> >> >> >> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the >> >> >> buffer when the size is zero. >> >> >> >> >> > >> >> > [..snip..] >> >> > >> >> > Yet under linux the address is checked even for zero case. >> >> >> >> Any value you can obtain from malloc() passes that check. >> >> >> >> Why does the fact that you can construct pointers that don't pass this >> >> check matter for our discussion of malloc()? >> >> >> >> >> > I don't know what a "valid pointer" in this context represents. >> >> >> >> >> >> I can talk standardese, if you prefer :) >> >> >> >> >> >> malloc() either returns either a null pointer or a pointer to the >> >> >> allocated space. In either case, you must not dereference the pointer. >> >> >> >> >> >> OpenBSD chooses to return a pointer to the allocated space. It chooses >> >> >> to catch common ways to dereference the pointer. >> >> >> >> >> >> Your "p = (void *)-1" is neither a null pointer nor can it point to >> >> >> allocated space on your particular system. Hence, it cannot be a value >> >> >> of malloc() for any argument, and therefore what read() does with it on >> >> >> that particular system doesn't matter. >> >> >> >> >> > >> >> > Here, i believe, you are inventing artificial restrictions on how >> >> > malloc behaves, i don't see anything that prevents the implementor >> >> > from setting aside a range of addresses with 31st bit set as an >> >> > indicator of "zero" allocations, and then happily giving it to the >> >> > user of malloc and consumming it in free. >> >> >> >> Misunderstanding? Such behavior is indeed permissible, and I can't see >> >> where I restricted it away. An implementation that behaves as you >> >> describe returns "pointer to allocated space". That the pointer has >> >> some funny bit set doesn't matter. That it can't be dereferenced is >> >> just fine. >> >> > > Here you agree that it's permissible. We were talking about ISO C, so by "implementation" I meant an implementation of ISO C, not an application program using it. Sorry if I didn't make that sufficiently clear. >> >> I'm not sure what your point is. If it is that malloc(0) can return a >> >> value that cannot be passed to a zero-sized read(), then I fear you have >> >> not made your point. >> > >> > One more attempt to make it clearer. If you agree that this behaviour >> > is permissible then the game is lost as things stand now under Linux, >> > since replacing [1]: >> > >> > void *p = (void *) -1 >> > with: >> > void *p = (void *) 0x80000000 >> > >> > or anything else with said bit set will yield EFAULT. Consequently the >> > code you cited as a well behaving malloc(0) call site will bomb. >> > >> > [1] Under 32bit Linux that is, with the usual split. >> >> You can't just pull pointers out of your ear and expect stuff to work. > > And here you don't. Which renders whole discussion rather pointless. And here we're talking about making up pointers in a portable application program. Which QEMU is. > Which renders whole discussion rather pointless. It's only tangentially related to the question whether qemu_malloc() should accept zero arguments anyway. >> malloc() is free to return a pointer to allocated space that is set up >> in a way that catches access beyond the allocated size. OpenBSD does >> that for size zero; it allocates one byte then, from pages that are used >> only for zero-sized allocations, and takes care to disable access to >> these pages with mprotect(..., PROT_NONE)[*]. Since read(..., 0) does >> not access beyond the allocated size, it still works just fine. >> >> If you replace glibc's malloc() to get OpenBSD-like behavior, you can't >> just make up some pointer to a memory area you believe to be unused, you >> have to do it right, like OpenBSD does. >> >> >> [*] Check out omalloc_make_chunks() at >> http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/malloc.c?rev=1.121;content-type=text%2Fplain
diff --git a/hw/elf_ops.h b/hw/elf_ops.h index 6093dea..003d2ef 100644 --- a/hw/elf_ops.h +++ b/hw/elf_ops.h @@ -219,51 +219,53 @@ static int glue(load_elf, SZ)(const char *name, int fd, int64_t address_offset, glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb); - size = ehdr.e_phnum * sizeof(phdr[0]); - lseek(fd, ehdr.e_phoff, SEEK_SET); - phdr = qemu_mallocz(size); - if (!phdr) - goto fail; - if (read(fd, phdr, size) != size) - goto fail; - if (must_swab) { - for(i = 0; i < ehdr.e_phnum; i++) { - ph = &phdr[i]; - glue(bswap_phdr, SZ)(ph); - } - } - - total_size = 0; - for(i = 0; i < ehdr.e_phnum; i++) { - ph = &phdr[i]; - if (ph->p_type == PT_LOAD) { - mem_size = ph->p_memsz; - /* XXX: avoid allocating */ - data = qemu_mallocz(mem_size); - if (ph->p_filesz > 0) { - if (lseek(fd, ph->p_offset, SEEK_SET) < 0) - goto fail; - if (read(fd, data, ph->p_filesz) != ph->p_filesz) - goto fail; + if (ehdr.e_phnum) { + size = ehdr.e_phnum * sizeof(phdr[0]); + lseek(fd, ehdr.e_phoff, SEEK_SET); + phdr = qemu_mallocz(size); + if (!phdr) + goto fail; + if (read(fd, phdr, size) != size) + goto fail; + if (must_swab) { + for(i = 0; i < ehdr.e_phnum; i++) { + ph = &phdr[i]; + glue(bswap_phdr, SZ)(ph); } - /* address_offset is hack for kernel images that are - linked at the wrong physical address. */ - addr = ph->p_paddr + address_offset; + } - snprintf(label, sizeof(label), "phdr #%d: %s", i, name); - rom_add_blob_fixed(label, data, mem_size, addr); + total_size = 0; + for(i = 0; i < ehdr.e_phnum; i++) { + ph = &phdr[i]; + if (ph->p_type == PT_LOAD) { + mem_size = ph->p_memsz; + if (memsize) { + data = qemu_mallocz(mem_size); + if (ph->p_filesz > 0) { + if (lseek(fd, ph->p_offset, SEEK_SET) < 0) + goto fail; + if (read(fd, data, ph->p_filesz) != ph->p_filesz) + goto fail; + } + /* address_offset is hack for kernel images that are + linked at the wrong physical address. */ + addr = ph->p_paddr + address_offset; - total_size += mem_size; - if (addr < low) - low = addr; - if ((addr + mem_size) > high) - high = addr + mem_size; + snprintf(label, sizeof(label), "phdr #%d: %s", i, name); + rom_add_blob_fixed(label, data, mem_size, addr); - qemu_free(data); - data = NULL; + total_size += mem_size; + qemu_free(data); + data = NULL; + } + if (addr < low) + low = addr; + if ((addr + mem_size) > high) + high = addr + mem_size; + } } + qemu_free(phdr); } - qemu_free(phdr); if (lowaddr) *lowaddr = (uint64_t)(elf_sword)low; if (highaddr)