| Message ID | m3skbwxgkp.fsf@crossbow.pond.sub.org |
|---|---|
| State | New |
| Headers | show |
On 11/30/2009 03:55 PM, 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 strongly agree with this patch. A consistent API improves quality, especially when it is also consistent with people's expectations.
Am 30.11.2009 14:55, schrieb Markus Armbruster: > 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. > > [...] > > Based on this sample, I'm confident there are many more uses broken by > the change. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Acked-by: Kevin Wolf <kwolf@redhat.com> We really should fix the root cause once and for all instead of working around unexpected qemu_malloc behaviour each time when one of the time bombs has blown up another (possibly production) VM.
> diff --git a/qemu-malloc.c b/qemu-malloc.c > index 295d185..aeeb78b 100644 > --- a/qemu-malloc.c > +++ b/qemu-malloc.c > @@ -44,22 +44,12 @@ void qemu_free(void *ptr) > > void *qemu_malloc(size_t size) > { > - if (!size) { > - abort(); > - } > - return oom_check(malloc(size)); > + return oom_check(malloc(size ? size : 1)); > } You might want to have a 'static uint8_t zero_length_malloc[0]' and return that instead of the magic cookie '1'. Makes the code more readable IMHO and you'll also have symbol in gdb when debugging qemu. Even more advanced: Make zero_length_malloc page-sized and page-aligned, then munmap int, so dereferencing it actually traps. > void *qemu_realloc(void *ptr, size_t size) > { > + return oom_check(realloc(ptr, size ? size : 1)); qemu_realloc(qemu_malloc(0), 42); should better work correctly ... Likewise qemu_free(qemu_malloc(0)); cheers, Gerd
> You might want to have a 'static uint8_t zero_length_malloc[0]' and > return that instead of the magic cookie '1'. Makes the code more > readable IMHO and you'll also have symbol in gdb when debugging qemu. Having multiple malloc return the same pointer sounds like a really bad idea. Paul
On 12/01/09 13:40, Gerd Hoffmann wrote: >> + return oom_check(malloc(size ? size : 1)); >> void *qemu_realloc(void *ptr, size_t size) >> { >> + return oom_check(realloc(ptr, size ? size : 1)); > > qemu_realloc(qemu_malloc(0), 42); > > should better work correctly ... Oops, scratch that. "return malloc(size ? size : 1)" is very different from "return size ? malloc(size) : 1" which my mind somehow made out of the code line above ... sorry, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: >> diff --git a/qemu-malloc.c b/qemu-malloc.c >> index 295d185..aeeb78b 100644 >> --- a/qemu-malloc.c >> +++ b/qemu-malloc.c >> @@ -44,22 +44,12 @@ void qemu_free(void *ptr) >> >> void *qemu_malloc(size_t size) >> { >> - if (!size) { >> - abort(); >> - } >> - return oom_check(malloc(size)); >> + return oom_check(malloc(size ? size : 1)); >> } > > You might want to have a 'static uint8_t zero_length_malloc[0]' and > return that instead of the magic cookie '1'. Makes the code more > readable IMHO and you'll also have symbol in gdb when debugging qemu. Complicates qemu_realloc() and qemu_free() somewhat, and that makes me think we better do it as a separate commit. Agree? > Even more advanced: Make zero_length_malloc page-sized and > page-aligned, then munmap int, so dereferencing it actually traps. Overrunning a malloc'ed buffer very rarely traps, not sure catching this special case is worth the portability headaches. If you really want to catch overruns, you need special tools like valgrind or electric fence anyway. >> void *qemu_realloc(void *ptr, size_t size) >> { >> + return oom_check(realloc(ptr, size ? size : 1)); > > qemu_realloc(qemu_malloc(0), 42); > > should better work correctly ... > > Likewise qemu_free(qemu_malloc(0));
On Tue, Dec 01, 2009 at 12:57:27PM +0000, Paul Brook wrote: > > You might want to have a 'static uint8_t zero_length_malloc[0]' and > > return that instead of the magic cookie '1'. Makes the code more > > readable IMHO and you'll also have symbol in gdb when debugging qemu. > > Having multiple malloc return the same pointer sounds like a really bad idea. And why's that? Keep in mind that *any* dereference over that address is a bug. Actually, I very much like Gerd's idea to unmap that address, so the bug won't hide from us in any circumnstances.
Glauber Costa <glommer@redhat.com> writes: > On Tue, Dec 01, 2009 at 12:57:27PM +0000, Paul Brook wrote: >> > You might want to have a 'static uint8_t zero_length_malloc[0]' and >> > return that instead of the magic cookie '1'. Makes the code more >> > readable IMHO and you'll also have symbol in gdb when debugging qemu. >> >> Having multiple malloc return the same pointer sounds like a really bad idea. > And why's that? > > Keep in mind that *any* dereference over that address is a bug. > > Actually, I very much like Gerd's idea to unmap that address, so the bug > won't hide from us in any circumnstances. For what it's worth, it violates the spec for malloc(). For zero-sized allocations, we may either return a null pointer (but we already decided we don't want to), or an object different from any other object alive. Thus, we can't return the same non-null pointer for all zero-sized allocations. Chapter and verse: ISO/IEC 9899:1999 7.20.3 Memory management functions The order and contiguity of storage allocated by successive calls to the calloc, malloc, and realloc functions is unspecified. The pointer returned if the allocation succeeds is suitably aligned so that it may be assigned to a pointer to any type of object and then used to access such an object or an array of such objects in the space allocated (until the space is explicitly deallocated). The lifetime of an allocated object extends from the allocation until the deallocation. Each such allocation shall yield a pointer to an object disjoint from any other object. The pointer returned points to the start (lowest byte address) of the allocated space. If the space cannot be allocated, a null pointer is returned. If the size of the space requested is zero, the behavior is implementation- defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object.
On Tuesday 01 December 2009, Glauber Costa wrote: > On Tue, Dec 01, 2009 at 12:57:27PM +0000, Paul Brook wrote: > > > You might want to have a 'static uint8_t zero_length_malloc[0]' and > > > return that instead of the magic cookie '1'. Makes the code more > > > readable IMHO and you'll also have symbol in gdb when debugging qemu. > > > > Having multiple malloc return the same pointer sounds like a really bad > > idea. > > And why's that? > > Keep in mind that *any* dereference over that address is a bug. Dereferencing the address is a bug. However testing the addresses themselves for equality is valid. This is much the same reason I think returning NULL would be a bad idea. Paul
On 12/01/2009 02:40 PM, Gerd Hoffmann wrote: > > Even more advanced: Make zero_length_malloc page-sized and > page-aligned, then munmap int, so dereferencing it actually traps. That will cause vma fragmentation. Since we don't trap malloc(n)[n] for n > 0, we may as well not trap it for n = 0.
On 12/01/09 15:08, Markus Armbruster wrote: > For what it's worth, it violates the spec for malloc(). For zero-sized > allocations, we may either return a null pointer (but we already decided > we don't want to), or an object different from any other object alive. Which clearly rules out the fixed memory location for malloc(0). "malloc(size ? size : 1)" is indeed the easiest way to make sure we conform to the spec and also don't return NULL. cheers, Gerd
On 12/01/09 15:34, Avi Kivity wrote: > On 12/01/2009 02:40 PM, Gerd Hoffmann wrote: >> >> Even more advanced: Make zero_length_malloc page-sized and >> page-aligned, then munmap int, so dereferencing it actually traps. > > That will cause vma fragmentation. I was thinking about a single unmapped page, which wouldn't cause fragmentation. But that is a bad idea too for other reasons ... cheers, Gerd
Excerpts from Markus Armbruster's message of Mon Nov 30 11:55:34 -0200 2009: > 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: Acked-by: Eduardo Habkost <ehabkost@redhat.com> This also makes qemu_realloc(NULL, size) completely equivalent to qemu_malloc(size), and that's a good thing.
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. That said, we've had this change in for a while and it's painfully obviously we haven't eliminated all of these instances in qemu. Knowing that we still have occurrences of this and actively exit()'ing when they happen is pretty much suicidal in a production environment. It's not a bug, it's just a poor practice. Here's what I propose. I think we should introduce a CONFIG_DEBUG_ZERO_MALLOC. When this is set, we should assert(size!=0). Otherwise, we should return malloc(1) as Markus' patch does below. Using the same rules as we follow for -Werror, we should enable CONFIG_DEBUG_ZERO_MALLOC for the development tree and disable it for any releases. This helps us make qemu better during development while not unnecessarily causing us harm in a production environment. What happens here long term I think remains to be seen. But for right now, I think this is an important change to make for 0.12. > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/qcow2-snapshot.c | 5 ----- > qemu-malloc.c | 14 ++------------ > 2 files changed, 2 insertions(+), 17 deletions(-) > > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c > index 94cb838..e3b208c 100644 > --- a/block/qcow2-snapshot.c > +++ b/block/qcow2-snapshot.c > @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) > QCowSnapshot *sn; > int i; > > - if (!s->nb_snapshots) { > - *psn_tab = NULL; > - return s->nb_snapshots; > - } > - > This does not belong here. Regards, Anthony Liguori
On 12/04/2009 06:49 PM, Anthony Liguori wrote: > > 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. Why? Unless we have a separate array allocator (like C++'s new and new[]), we need to support zero-element arrays without pushing the burden to callers (in the same way that for () supports zero iteration loops without a separate if ()).
Avi Kivity wrote: > On 12/04/2009 06:49 PM, Anthony Liguori wrote: >> >> 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. > > Why? Unless we have a separate array allocator (like C++'s new and > new[]), we need to support zero-element arrays without pushing the > burden to callers (in the same way that for () supports zero iteration > loops without a separate if ()). If you call malloc(size=0), then it's impossible to differentiate between a failed malloc return and a valid result on certain platform (like AIX). So the only correct way would be to write: array = malloc(size); if (array == NULL && size != 0) { return -ENOMEM; } If you were writing portable code. But that's not what people write. You can argue that qemu_malloc() can have any semantics we want and while that's true, it doesn't change my above statement. I think the main argument for this behavior in qemu is that people are used to using this idiom with malloc() but it's a non-portable practice. If qemu_malloc() didn't carry the name "malloc()" then semantics with size=0 would be a different discussion. But so far, all qemu_* functions tend to behave almost exactly like their C counterparts. Relying on the result of size=0 with malloc() is broken. Regards, Anthony Liguori
On Sat, Dec 5, 2009 at 5:07 PM, Avi Kivity <avi@redhat.com> wrote: > On 12/04/2009 06:49 PM, Anthony Liguori wrote: >> >> 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. > > Why? Unless we have a separate array allocator (like C++'s new and new[]), > we need to support zero-element arrays without pushing the burden to callers > (in the same way that for () supports zero iteration loops without a > separate if ()). Running a loop zero or nonzero number of times always has a very clear and precise meaning. A pointer returned from allocating zero or nonzero number of items may be completely unusable or usable, respectively. I think Laurent's proposal would work. We even could go so far as rename the current function as qemu_malloc_possibly_broken (and adjust callers mechanically) and introduce two new versions, which handle the zero case in clearly advertised ways. Patches would fix the callers to use the correct one.
On 12/05/2009 07:27 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 12/04/2009 06:49 PM, Anthony Liguori wrote: >>> >>> 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. >> >> Why? Unless we have a separate array allocator (like C++'s new and >> new[]), we need to support zero-element arrays without pushing the >> burden to callers (in the same way that for () supports zero >> iteration loops without a separate if ()). > > If you call malloc(size=0), then it's impossible to differentiate > between a failed malloc return and a valid result on certain platform > (like AIX). > > So the only correct way would be to write: > > array = malloc(size); > if (array == NULL && size != 0) { > return -ENOMEM; > } > > If you were writing portable code. But that's not what people write. > You can argue that qemu_malloc() can have any semantics we want and > while that's true, it doesn't change my above statement. I think the > main argument for this behavior in qemu is that people are used to > using this idiom with malloc() but it's a non-portable practice. > > If qemu_malloc() didn't carry the name "malloc()" then semantics with > size=0 would be a different discussion. But so far, all qemu_* > functions tend to behave almost exactly like their C counterparts. > Relying on the result of size=0 with malloc() is broken. A zero-supporting qemu_malloc() is fully compatible with malloc(), we're only restricting the possible returns. So we're not misleading any caller. In fact, taking your argument to the extreme, a malloc implementation would need to if (size == 0) { if (rand() % 2) { return NULL; } else { size = 1; } } Note that qemu_malloc() already restricts return values by not returning NULL on oom, according to your logic we shouldn't do this (and have two tests accompany any variable-size qemu_malloc()).
On 12/05/2009 07:28 PM, Blue Swirl wrote: > On Sat, Dec 5, 2009 at 5:07 PM, Avi Kivity<avi@redhat.com> wrote: > >> On 12/04/2009 06:49 PM, Anthony Liguori wrote: >> >>> 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. >>> >> Why? Unless we have a separate array allocator (like C++'s new and new[]), >> we need to support zero-element arrays without pushing the burden to callers >> (in the same way that for () supports zero iteration loops without a >> separate if ()). >> > Running a loop zero or nonzero number of times always has a very clear > and precise meaning. A pointer returned from allocating zero or > nonzero number of items may be completely unusable or usable, > respectively. > Only if you allocate using POSIX malloc(). If you allocate using a function that is defined to return a valid pointer for zero length allocations, you're happy. > I think Laurent's proposal would work. We even could go so far as > rename the current function as qemu_malloc_possibly_broken (and adjust > callers mechanically) and introduce two new versions, which handle the > zero case in clearly advertised ways. Patches would fix the callers to > use the correct one Good idea. Let's name the function that returns a valid pointer qemu_malloc() (since that's what many callers expect anyway, and it's fully backwards compatible), and see who calls qemu_malloc_dont_call_me_with_zero(). Realistically, do we need two variants of malloc/realloc/free, or can we stick with one that works for callers with a minimum of fuss?
Avi Kivity wrote: > A zero-supporting qemu_malloc() is fully compatible with malloc(), > we're only restricting the possible returns. So we're not misleading > any caller. In fact, taking your argument to the extreme, a malloc > implementation would need to This is really the crux of the whole argument. You're arguing that while most people rely on incorrect idioms with malloc(), the problem is not the idioms themselves but the definition of malloc(). The opposing argument is that instead of providing a "fixed" version of malloc(), we should encourage people to use a proper idiom. I dislike the entire notion of qemu_malloc(). I've always disliked the fact that it abort()s on OOM. I'd rather see us use a normal malloc() and code to that malloc currently which means avoiding size=0 and checking NULL results. However, this is all personal preference and I'd rather focus my energy on things that have true functional impact. Markus raised a valid functional problem with the current implementation and I proposed a solution that would address that functional problem. I'd rather see the discussion focus on the merits of that solution than revisiting whether ANSI got the semantics of malloc() correct in the standards definition. Regards, Anthony Liguori
On 12/05/2009 07:54 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> A zero-supporting qemu_malloc() is fully compatible with malloc(), >> we're only restricting the possible returns. So we're not misleading >> any caller. In fact, taking your argument to the extreme, a malloc >> implementation would need to > > This is really the crux of the whole argument. You're arguing that > while most people rely on incorrect idioms with malloc(), the problem > is not the idioms themselves but the definition of malloc(). The > opposing argument is that instead of providing a "fixed" version of > malloc(), we should encourage people to use a proper idiom. When we see a lengthy and error prone idiom we usually provide a wrapper. That wrapper is qemu_malloc(). If you like, don't see it as a fixed malloc(), but as qemu's way of allocating memory which is totally independent from malloc(). > > I dislike the entire notion of qemu_malloc(). I've always disliked > the fact that it abort()s on OOM. I'd rather see us use a normal > malloc() and code to that malloc currently which means avoiding size=0 > and checking NULL results. I believe that's impossible. Many times, when the guest writes to a register, you have no way to indicate failure. Sometimes you can indicate failure (say, time out on IDE write requests) but will likely cause much damage to the guest. Preallocating memory is likely to be very difficult and also very wasteful (since you'll have to account for the worst case). Furthermore, Linux under its default configuration will never fail a small allocation, instead oom-killing a semi-random process. So all that work will not help on that platform. > > However, this is all personal preference and I'd rather focus my > energy on things that have true functional impact. Markus raised a > valid functional problem with the current implementation and I > proposed a solution that would address that functional problem. I'd > rather see the discussion focus on the merits of that solution than > revisiting whether ANSI got the semantics of malloc() correct in the > standards definition. > Unless ANSI has a say on qemu_malloc(), I think it's worthwhile to get that right rather than wrapping every array caller with useless tests.
On Sat, Dec 5, 2009 at 6:44 PM, Avi Kivity <avi@redhat.com> wrote: [...] > >> I think Laurent's proposal would work. We even could go so far as >> rename the current function as qemu_malloc_possibly_broken (and adjust >> callers mechanically) and introduce two new versions, which handle the >> zero case in clearly advertised ways. Patches would fix the callers to >> use the correct one > > Good idea. Let's name the function that returns a valid pointer > qemu_malloc() (since that's what many callers expect anyway, and it's fully > backwards compatible), and see who calls > qemu_malloc_dont_call_me_with_zero(). Yes, you're always free to follow poor coding rules by breaking strict API. Laurent
Avi Kivity wrote: > When we see a lengthy and error prone idiom we usually provide a > wrapper. That wrapper is qemu_malloc(). If you like, don't see it as > a fixed malloc(), but as qemu's way of allocating memory which is > totally independent from malloc(). We constantly get patches with qemu_malloc() with a NULL check. Then we tell people to remove the NULL check. It feels very weird to ask people to remove error handling. I can understand the argument that getting OOM right is very difficult but it's not impossible. >> >> However, this is all personal preference and I'd rather focus my >> energy on things that have true functional impact. Markus raised a >> valid functional problem with the current implementation and I >> proposed a solution that would address that functional problem. I'd >> rather see the discussion focus on the merits of that solution than >> revisiting whether ANSI got the semantics of malloc() correct in the >> standards definition. >> > > Unless ANSI has a say on qemu_malloc(), I think it's worthwhile to get > that right rather than wrapping every array caller with useless tests. If you're concerned about array allocation, introduce an array allocation function. Honestly, there's very little reason to open code array allocation/manipulation at all. We should either be using a list type or if we really need to, we should introduce a vector type. Regards, Anthony Liguori
On 12/05/2009 10:58 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> When we see a lengthy and error prone idiom we usually provide a >> wrapper. That wrapper is qemu_malloc(). If you like, don't see it >> as a fixed malloc(), but as qemu's way of allocating memory which is >> totally independent from malloc(). > > We constantly get patches with qemu_malloc() with a NULL check. Then > we tell people to remove the NULL check. It feels very weird to ask > people to remove error handling. You prefer to explain to them how to do error handling correctly? > > I can understand the argument that getting OOM right is very difficult > but it's not impossible. There are 755 calls to malloc in the code. And practically every syscall can return ENOMEM, including the innocuous KVM_RUN ioctl(). It's going to be pretty close to impossible to recover from malloc() failure, and impossible to recover from KVM_RUN failure (except by retrying, which you can assume the kernel already has). All for something which never happens. I propose that fixing OOM handling is going to introduce some errors into the non-error paths, and many errors into the error return paths, for zero benefit. > >>> >>> However, this is all personal preference and I'd rather focus my >>> energy on things that have true functional impact. Markus raised a >>> valid functional problem with the current implementation and I >>> proposed a solution that would address that functional problem. I'd >>> rather see the discussion focus on the merits of that solution than >>> revisiting whether ANSI got the semantics of malloc() correct in the >>> standards definition. >>> >> >> Unless ANSI has a say on qemu_malloc(), I think it's worthwhile to >> get that right rather than wrapping every array caller with useless >> tests. > > If you're concerned about array allocation, introduce an array > allocation function. Honestly, there's very little reason to open > code array allocation/manipulation at all. We should either be using > a list type or if we really need to, we should introduce a vector type. A NEW(type) and ARRAY_NEW(type, count) marcros would improve type safety and plug a dormant buffer overflow due to multiplication overflow, yes. Even qemu_calloc() would be an improvement. But having qemu_malloc() not fix the zero length array case which we know we have is irresponsible, IMO.
Avi Kivity wrote: > Only if you allocate using POSIX malloc(). If you allocate using a > function that is defined to return a valid pointer for zero length > allocations, you're happy. Wouldnt it be better to, rather than use a qemu_malloc() that is utterly counterintuitive in that it has no way to report failure, and behaves in ways people dont expect, to use normal malloc() and never pass it 0 ? Seriously, who does that anyway? why call malloc when you dont want the space? so you can use realloc? 99.99% of the time realloc() is the Wrong Solution(tm). stick to what people know, and LART them for misuse of it if necessary. (Just my 2p) -Ian
On 12/06/2009 01:08 AM, Ian Molton wrote: > Avi Kivity wrote: > > >> Only if you allocate using POSIX malloc(). If you allocate using a >> function that is defined to return a valid pointer for zero length >> allocations, you're happy. >> > Wouldnt it be better to, rather than use a qemu_malloc() that is utterly > counterintuitive in that it has no way to report failure, and behaves in > ways people dont expect, to use normal malloc() and never pass it 0 ? > It's not that it doesn't have a way to report failure, it's that it doesn't fail. Do you prefer functions that fail and report it to functions that don't fail? > Seriously, who does that anyway? why call malloc when you dont want the > space? so you can use realloc? 99.99% of the time realloc() is the Wrong > Solution(tm). > Read the beginning of the thread. Basically it's for arrays, malloc(n * sizeof(x)). > stick to what people know, and LART them for misuse of it if necessary. > The LART is a crash, great.
Avi Kivity wrote: > It's not that it doesn't have a way to report failure, it's that it > doesn't fail. Do you prefer functions that fail and report it to > functions that don't fail? You have a way of allocating memory that will _never_ fail? >> Seriously, who does that anyway? why call malloc when you dont want the >> space? so you can use realloc? 99.99% of the time realloc() is the Wrong >> Solution(tm). >> > Read the beginning of the thread. Basically it's for arrays, malloc(n * > sizeof(x)). well, make sure n is not 0. Its not that hard. I dont think I've *ever* had a situation where I wanted to pass 0 to malloc. >> stick to what people know, and LART them for misuse of it if necessary. > > The LART is a crash, great. No, the LART would be a 'your patch does this wrong, try this:' -Ian
Anthony Liguori <anthony@codemonkey.ws> writes: > Avi Kivity wrote: >> On 12/04/2009 06:49 PM, Anthony Liguori wrote: >>> >>> 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. >> >> Why? Unless we have a separate array allocator (like C++'s new and >> new[]), we need to support zero-element arrays without pushing the >> burden to callers (in the same way that for () supports zero >> iteration loops without a separate if ()). > > If you call malloc(size=0), then it's impossible to differentiate > between a failed malloc return and a valid result on certain platform > (like AIX). > > So the only correct way would be to write: > > array = malloc(size); > if (array == NULL && size != 0) { > return -ENOMEM; > } > > If you were writing portable code. But that's not what people write. Yup. But if (n == 0) p = malloc(n * sizeof(struct foo)) else p = NULL; isn't what people write either. > You can argue that qemu_malloc() can have any semantics we want and > while that's true, it doesn't change my above statement. I think the > main argument for this behavior in qemu is that people are used to > using this idiom with malloc() but it's a non-portable practice. We've decided long ago to disable the trap you quoted by not returning NULL for empty allocations. That way it doesn't kill us when people fail to call qemu_malloc() perfectly every time. > If qemu_malloc() didn't carry the name "malloc()" then semantics with > size=0 would be a different discussion. But so far, all qemu_* > functions tend to behave almost exactly like their C counterparts. That's a reason for making qemu_malloc() work for zero arguments, because its C counterpart does. > Relying on the result of size=0 with malloc() is broken. No. Relying on non-null-ness is broken. You can write perfectly portable, working code without doing that. p = malloc(n * sizeof(struct foo); if (n && !p) exit_no_mem(); for (i = 0; i < n; i++) compute_one(p, i); With qemu_malloc(), the error handling moves into qemu_malloc(): p = qemu_malloc(n * sizeof(struct foo); for (i = 0; i < n; i++) compute_one(p, i);
Avi Kivity <avi@redhat.com> writes: > A NEW(type) and ARRAY_NEW(type, count) marcros would improve type > safety and plug a dormant buffer overflow due to multiplication > overflow, yes. Even qemu_calloc() would be an improvement. But > having qemu_malloc() not fix the zero length array case which we know > we have is irresponsible, IMO. Agree on all counts.
On 12/06/2009 01:25 AM, Ian Molton wrote: > Avi Kivity wrote: > > >> It's not that it doesn't have a way to report failure, it's that it >> doesn't fail. Do you prefer functions that fail and report it to >> functions that don't fail? >> > You have a way of allocating memory that will _never_ fail? > > Sort of. Did you look at the code? >>> Seriously, who does that anyway? why call malloc when you dont want the >>> space? so you can use realloc? 99.99% of the time realloc() is the Wrong >>> Solution(tm). >>> >>> >> Read the beginning of the thread. Basically it's for arrays, malloc(n * >> sizeof(x)). >> > well, make sure n is not 0. Its not that hard. I dont think I've *ever* > had a situation where I wanted to pass 0 to malloc. > There are multiple such cases in the code. >>> stick to what people know, and LART them for misuse of it if necessary. >>> >> The LART is a crash, great. >> > No, the LART would be a 'your patch does this wrong, try this:' > What about existing usage? Will you audit all the existing calls?
Markus Armbruster wrote: > p = malloc(n * sizeof(struct foo); > if (n && !p) > exit_no_mem(); > for (i = 0; i < n; i++) > compute_one(p, i); > > With qemu_malloc(), the error handling moves into qemu_malloc(): > > p = qemu_malloc(n * sizeof(struct foo); > for (i = 0; i < n; i++) > compute_one(p, i); And you lose the ability to fail gracefully...
Avi Kivity wrote: > On 12/06/2009 01:25 AM, Ian Molton wrote: >> Avi Kivity wrote: >> >> >>> It's not that it doesn't have a way to report failure, it's that it >>> doesn't fail. Do you prefer functions that fail and report it to >>> functions that don't fail? >>> >> You have a way of allocating memory that will _never_ fail? > > Sort of. 'sort of' never ? > Did you look at the code? Yes. Its hardly infallible. >> well, make sure n is not 0. Its not that hard. I dont think I've *ever* >> had a situation where I wanted to pass 0 to malloc. > > There are multiple such cases in the code. > >>>> stick to what people know, and LART them for misuse of it if necessary. >>>> >>> The LART is a crash, great. >>> >> No, the LART would be a 'your patch does this wrong, try this:' > > What about existing usage? Will you audit all the existing calls? mark qemu_malloc as deprecated. don't include new patches that use it. Plenty of time to fix the broken uses... -Ian
On 12/06/2009 06:58 PM, Ian Molton wrote: > Avi Kivity wrote: > >> On 12/06/2009 01:25 AM, Ian Molton wrote: >> >>> Avi Kivity wrote: >>> >>> >>> >>>> It's not that it doesn't have a way to report failure, it's that it >>>> doesn't fail. Do you prefer functions that fail and report it to >>>> functions that don't fail? >>>> >>>> >>> You have a way of allocating memory that will _never_ fail? >>> >> Sort of. >> > 'sort of' never ? > > >> Did you look at the code? >> > Yes. Its hardly infallible. > It will never fail on Linux. On other hosts it prevents a broken oom handler from taking the guest down a death spiral. >> What about existing usage? Will you audit all the existing calls? >> > mark qemu_malloc as deprecated. don't include new patches that use it. > Plenty of time to fix the broken uses... > Send patches. I don't think it's realistic to handle OOM in qemu (handling n=0 is easy, but a lot of work for no real gain).
On 12/06/2009 06:52 PM, Ian Molton wrote: > Markus Armbruster wrote: > > >> p = malloc(n * sizeof(struct foo); >> if (n&& !p) >> exit_no_mem(); >> for (i = 0; i< n; i++) >> compute_one(p, i); >> >> With qemu_malloc(), the error handling moves into qemu_malloc(): >> >> p = qemu_malloc(n * sizeof(struct foo); >> for (i = 0; i< n; i++) >> compute_one(p, i); >> > And you lose the ability to fail gracefully... > We never had it. Suppose p is allocated in response to an SCSI register write, and we allocate a scatter-gather list. What do you do if you OOM? 1) Introduce an error path that works synchronously off the stack and so does not need to allocate memory 2) Don't allocate in the first place, always read guest memory and transform it for sglists, preallocate everything else 3) Have a per-thread emergency pool, stall the vcpu until all memory is returned to the emergency pool all of these options are pretty horrible, either to the code or to the guest, for something that never happens.
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 06:52 PM, Ian Molton wrote: > > Markus Armbruster wrote: > > > > > > > p = malloc(n * sizeof(struct foo); > > > if (n&& !p) > > > exit_no_mem(); > > > for (i = 0; i< n; i++) > > > compute_one(p, i); > > > > > > With qemu_malloc(), the error handling moves into qemu_malloc(): > > > > > > p = qemu_malloc(n * sizeof(struct foo); > > > for (i = 0; i< n; i++) > > > compute_one(p, i); > > > > > And you lose the ability to fail gracefully... > > > > We never had it. Suppose p is allocated in response to an SCSI register > write, and we allocate a scatter-gather list. What do you do if you OOM? Uh, please do not generalize. See for instance 29ddf27b72960d6e6b115cd69812c9c57b2a7b13 the incident was logged and debugged, no OOM, no abort. Not all scenarios admit this, but claiming that there are none that do is incorrect. > > 1) Introduce an error path that works synchronously off the stack and so does > not need to allocate memory > 2) Don't allocate in the first place, always read guest memory and transform > it for sglists, preallocate everything else > 3) Have a per-thread emergency pool, stall the vcpu until all memory is > returned to the emergency pool > > all of these options are pretty horrible, either to the code or to the guest, > for something that never happens. > >
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 06:58 PM, Ian Molton wrote: > > Avi Kivity wrote: > > > > > On 12/06/2009 01:25 AM, Ian Molton wrote: > > > > > > > Avi Kivity wrote: > > > > > > > > > > > > > > > > > It's not that it doesn't have a way to report failure, it's that it > > > > > doesn't fail. Do you prefer functions that fail and report it to > > > > > functions that don't fail? > > > > > > > > > > > > > > You have a way of allocating memory that will _never_ fail? > > > > > > > Sort of. > > > > > 'sort of' never ? > > > > > > > Did you look at the code? > > > > > Yes. Its hardly infallible. > > > > It will never fail on Linux. On other hosts it prevents a broken oom handler > from taking the guest down a death spiral. It fails here all the time i'm sorry to say, i have overcommit disabled (mostly because kpdf when doing a text search tends to overwhelm the VM subsystem and Linux happily picks X11 as it's OOM kill target) > > > > What about existing usage? Will you audit all the existing calls? > > > > > mark qemu_malloc as deprecated. don't include new patches that use it. > > Plenty of time to fix the broken uses... > > > > Send patches. I don't think it's realistic to handle OOM in qemu (handling > n=0 is easy, but a lot of work for no real gain). >
On 12/06/2009 07:47 PM, malc wrote: > >> It will never fail on Linux. On other hosts it prevents a broken oom handler >> from taking the guest down a death spiral. >> > It fails here all the time i'm sorry to say, i have overcommit disabled > (mostly because kpdf when doing a text search tends to overwhelm the VM > subsystem and Linux happily picks X11 as it's OOM kill target) > Right, I meant under the default configuration. Unfortunately there is no good configuration for Linux - without strict overcommit you'll invoke the oom killer, with strict overcommit you'll need ridiculous amounts of swap because fork() and MAP_PRIVATE require tons of backing store. On my home machine I have $ grep Commit /proc/meminfo CommitLimit: 7235160 kB Committed_AS: 4386172 kB So, 4GB of virtual memory needed just to run a normal desktop with thunderbird+firefox. Actual anonymous memory is less than 2GB, so you could easily run this workload on a 2GB machine without swap, but with strict overcommit 2GB RAM + 2GB swap will see failures even though you have free RAM.
On 12/06/2009 07:45 PM, malc wrote: > > >>> And you lose the ability to fail gracefully... >>> >>> >> We never had it. Suppose p is allocated in response to an SCSI register >> write, and we allocate a scatter-gather list. What do you do if you OOM? >> > Uh, please do not generalize. > > Sorry. > See for instance 29ddf27b72960d6e6b115cd69812c9c57b2a7b13 the incident was > logged and debugged, no OOM, no abort. Not all scenarios admit this, but > claiming that there are none that do is incorrect. > Init is pretty easy to handle. I'm worried about runtime where you can't report an error to the guest. Real hardware doesn't oom.
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 07:47 PM, malc wrote: > > > > > It will never fail on Linux. On other hosts it prevents a broken oom > > > handler > > > from taking the guest down a death spiral. > > > > > It fails here all the time i'm sorry to say, i have overcommit disabled > > (mostly because kpdf when doing a text search tends to overwhelm the VM > > subsystem and Linux happily picks X11 as it's OOM kill target) > > > > Right, I meant under the default configuration. Unfortunately there is no > good configuration for Linux - without strict overcommit you'll invoke the oom > killer, with strict overcommit you'll need ridiculous amounts of swap because > fork() and MAP_PRIVATE require tons of backing store. > > On my home machine I have > > $ grep Commit /proc/meminfo > CommitLimit: 7235160 kB > Committed_AS: 4386172 kB > > So, 4GB of virtual memory needed just to run a normal desktop with > thunderbird+firefox. Actual anonymous memory is less than 2GB, so you could > easily run this workload on a 2GB machine without swap, but with strict > overcommit 2GB RAM + 2GB swap will see failures even though you have free RAM. > Well, i don't have a swap... ~$ cat /proc/meminfo MemTotal: 515708 kB MemFree: 3932 kB Buffers: 10120 kB Cached: 365320 kB SwapCached: 0 kB Active: 238120 kB Inactive: 222396 kB SwapTotal: 0 kB SwapFree: 0 kB ... CommitLimit: 464136 kB Committed_AS: 178920 kB ...
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 07:45 PM, malc wrote: > > > > > > > > And you lose the ability to fail gracefully... > > > > > > > > > > > We never had it. Suppose p is allocated in response to an SCSI register > > > write, and we allocate a scatter-gather list. What do you do if you OOM? > > > > > Uh, please do not generalize. > > > > > > Sorry. > > > See for instance 29ddf27b72960d6e6b115cd69812c9c57b2a7b13 the incident was > > logged and debugged, no OOM, no abort. Not all scenarios admit this, but > > claiming that there are none that do is incorrect. > > > > Init is pretty easy to handle. I'm worried about runtime where you can't > report an error to the guest. Real hardware doesn't oom. Here, i do agree, but mostly because most of the users of allocation functions just themselves returned NULL or -1 or whatever and never really bothered to report anything, so the addition of OOM check that you've added made the code less cluttered.
On 12/06/2009 08:09 PM, malc wrote: > > Well, i don't have a swap... > > ~$ cat /proc/meminfo > MemTotal: 515708 kB > MemFree: 3932 kB > Buffers: 10120 kB > Cached: 365320 kB > SwapCached: 0 kB > Active: 238120 kB > Inactive: 222396 kB > SwapTotal: 0 kB > SwapFree: 0 kB > ... > CommitLimit: 464136 kB > Committed_AS: 178920 kB > ... > Out of curiosity, what desktop and apps are you running? Here firefox takes 1.3GB virt size and 377MB RSS, that alone could overwhelm your system. [avi@firebolt qemu-kvm (master)]$ cat /proc/2698/status Name: firefox State: S (sleeping) Tgid: 2698 Pid: 2698 PPid: 2686 TracerPid: 0 Uid: 500 500 500 500 Gid: 500 500 500 500 Utrace: 0 FDSize: 256 Groups: 500 502 512 VmPeak: 1478804 kB VmSize: 1288100 kB VmLck: 0 kB VmHWM: 434788 kB VmRSS: 386296 kB VmData: 676384 kB VmStk: 248 kB VmExe: 88 kB VmLib: 62504 kB VmPTE: 2312 kB
On 12/06/2009 08:12 PM, malc wrote: > >> Init is pretty easy to handle. I'm worried about runtime where you can't >> report an error to the guest. Real hardware doesn't oom. >> > Here, i do agree, but mostly because most of the users of allocation > functions just themselves returned NULL or -1 or whatever and never > really bothered to report anything, so the addition of OOM check that > you've added made the code less cluttered. > My point is that it would take a major rework, and would probably involve removing the allocations instead of handling any errors. For example, a scsi device would tell the block device the upper bound of aiocbs it could possibly issue, and the maximum number of sg elements in a request, and qcow2 (or any other backing format driver) would preallocate enough resources to satisfy the worst case. And we still can't handle a syscall returning ENOMEM.
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 08:09 PM, malc wrote: > > > > Well, i don't have a swap... > > > > ~$ cat /proc/meminfo > > MemTotal: 515708 kB > > MemFree: 3932 kB > > Buffers: 10120 kB > > Cached: 365320 kB > > SwapCached: 0 kB > > Active: 238120 kB > > Inactive: 222396 kB > > SwapTotal: 0 kB > > SwapFree: 0 kB > > ... > > CommitLimit: 464136 kB > > Committed_AS: 178920 kB > > ... > > > > Out of curiosity, what desktop and apps are you running? Here firefox takes > 1.3GB virt size and 377MB RSS, that alone could overwhelm your system. Most of the time my setup is: IceWM - 4 virtual desktops XEmacs Seamonkey 1.1.16 IceDock A bunch of rxvts > > [avi@firebolt qemu-kvm (master)]$ cat /proc/2698/status > Name: firefox > State: S (sleeping) > Tgid: 2698 > Pid: 2698 > PPid: 2686 > TracerPid: 0 > Uid: 500 500 500 500 > Gid: 500 500 500 500 > Utrace: 0 > FDSize: 256 > Groups: 500 502 512 > VmPeak: 1478804 kB > VmSize: 1288100 kB > VmLck: 0 kB > VmHWM: 434788 kB > VmRSS: 386296 kB > VmData: 676384 kB > VmStk: 248 kB > VmExe: 88 kB > VmLib: 62504 kB > VmPTE: 2312 kB > ~$ cat /proc/$(pidof seamonkey-bin)/status Name: seamonkey-bin State: S (sleeping) Tgid: 2332 Pid: 2332 PPid: 1 TracerPid: 0 Uid: 1000 1000 1000 1000 Gid: 100 100 100 100 FDSize: 256 Groups: 10 11 17 18 19 100 VmPeak: 151848 kB VmSize: 150976 kB VmLck: 0 kB VmHWM: 57032 kB VmRSS: 56260 kB VmData: 105740 kB VmStk: 84 kB VmExe: 296 kB VmLib: 34472 kB VmPTE: 136 kB Threads: 5 [...]
Ian Molton wrote: > > Read the beginning of the thread. Basically it's for arrays, malloc(n * > > sizeof(x)). > > well, make sure n is not 0. Its not that hard. I dont think I've *ever* > had a situation where I wanted to pass 0 to malloc. I would like to remind everyone that sizeof(x) can be 0 too. For example, on Linux sizeof(spinlock_t) == 0 on UP. Anything where you have a bunch of structure fields which depend on compile time configuration, or where a type might be replaced by a stub empty structure, is a possible sizeof(x) == 0. > Its not that hard. The fact is there are a number of bugs in qemu where n == 0 is not checked prior to calling qemu_malloc() at the moment. None of them are "hard" to fix - they are rare cases that nobody noticed when writing them. Until we have code analysis tools checking for that, bugs of that kind will probably keep arising. -- Jamie
Avi Kivity wrote: > A NEW(type) and ARRAY_NEW(type, count) marcros would improve type safety > and plug a dormant buffer overflow due to multiplication overflow, yes. > Even qemu_calloc() would be an improvement. In my code I regularly use type_alloc(type) and type_free(type, ptr), giving type safety at both ends (and possibility to optimise allocations, but that's separate). If you have ARRAY_NEW(type, count) which permits count to be zero and returns a non-NULL result, I wonder, why is it ok to convert zero count to a guaranteed non-NULL unique result, but not do that for sizeof(type) (or just size)? -- Jamie
On Sun, 6 Dec 2009, Avi Kivity wrote: > On 12/06/2009 08:12 PM, malc wrote: > > > > > Init is pretty easy to handle. I'm worried about runtime where you can't > > > report an error to the guest. Real hardware doesn't oom. > > > > > Here, i do agree, but mostly because most of the users of allocation > > functions just themselves returned NULL or -1 or whatever and never > > really bothered to report anything, so the addition of OOM check that > > you've added made the code less cluttered. > > > > My point is that it would take a major rework, and would probably involve > removing the allocations instead of handling any errors. For example, a scsi > device would tell the block device the upper bound of aiocbs it could possibly > issue, and the maximum number of sg elements in a request, and qcow2 (or any > other backing format driver) would preallocate enough resources to satisfy the > worst case. And we still can't handle a syscall returning ENOMEM. > Sure. My point is that sometimes failure to allocate is due to bugs, invalid input etc, and conversion to OOM aborts en masse, might have not been the best possible route to take, but most likely it was better than doing nothing.
Avi Kivity wrote: > Init is pretty easy to handle. I'm worried about runtime where you > can't report an error to the guest. Real hardware doesn't oom. In the case of the socket reconnect code I posted recently, if the allocation failed, it would give up trying to reconnect and inform the user of that chardev that it had closed. Ok, this doesnt help the guest, but it allows other code to clean up nicely, and we can report the failure to the host. IMHO thats better than leaving a sysadmin scratching their head wondering why it suddenly just stopped feeding the guest entropy and isnt trying to reconnect anymore... (IMO) -Ian
malc wrote:
> Well, i don't have a swap...
Indeed, nor do I (machine has an NFS root. swap on NFS is... not good.)
Ian Molton wrote: > Avi Kivity wrote: > > > Init is pretty easy to handle. I'm worried about runtime where you > > can't report an error to the guest. Real hardware doesn't oom. > > In the case of the socket reconnect code I posted recently, if the > allocation failed, it would give up trying to reconnect and inform the > user of that chardev that it had closed. Ok, this doesnt help the guest, > but it allows other code to clean up nicely, and we can report the > failure to the host. IMHO thats better than leaving a sysadmin > scratching their head wondering why it suddenly just stopped feeding the > guest entropy and isnt trying to reconnect anymore... If the system as a whole runs out of memory so that no-overcommit malloc() fails on a small alloc, there's a good chance that you won't be able to send a message to the host (how do you format the QMP message without malloc?), and if you do manage that, there's a good chance the host won't be able to receive it (it can't malloc either), and if it does manage to receive the message, you can be almost certain that it won't be able to run any GUI operations, send mail, etc. to inform the admin. The chances of the path "qemu small alloc -> chardev error -> send QMP message -> receive QMP message -> parse QMG message -> do something useful (log/email/UI)" having fully preallocated buffers for every step, including a preallocated emergency pool for the buffers used by QMG formatting and parsing, so that it gets all the way past the last step are very slim indeed. There's no point writing the code for the first steps, if it's intractable to make the later steps do something useful. Btw, as an admin I would really rather the socket reconnection code keeps trying in that circumstance, if qemu does not simply fall over due to alloc failing for something else soon after. The most likely scenario, imho in a server like that, is to notice it is running out of memory and kill the real cause (e.g. another runaway process), then restart all daemons which have died. I'm not going to notice a non-fatal message (in the unlikely event it is propagated all the way up) because there are plenty of other non-fatal messages in normal use, multiplied by hundreds of guests (across a cluster). Or, if you mean the chardev closing causes qemu to terminate - what's the difference from the current qemu_malloc() behaviour? I'd rather it behaves like a broken HWRNG if it can't get host entropy: Don't provide data, and let the guest decide what to do, just like it does for a broken HWRNG. Except virtio-rng can report unavailability rather than simply being broken :-) -- Jamie
Am 05.12.2009 18:27, schrieb Anthony Liguori: > If qemu_malloc() didn't carry the name "malloc()" then semantics with > size=0 would be a different discussion. But so far, all qemu_* > functions tend to behave almost exactly like their C counterparts. > Relying on the result of size=0 with malloc() is broken. Then let's rename it as qemu_getmem, do a sed run over the whole code and have sane semantics. :-) Kevin
Jamie Lokier wrote: > If the system as a whole runs out of memory so that no-overcommit > malloc() fails on a small alloc, there's a good chance that you won't > be able to send a message to the host Send what message to the host? If the malloc in the socet reconnect code fails, its the code on the host thats going to flag up that malloc failed. > and if it does manage to receive the message, you can be almost > certain that it won't be able to run any GUI operations, send mail, > etc. to inform the admin. OTOH, If all it does it log it to a file, theres a fair chance it might succeed. > There's no point writing the code for the first steps, if it's > intractable to make the later steps do something useful. OTOH, a simple printed warning, and closing the socket are fairly likely to work. > Btw, as an admin I would really rather the socket reconnection code > keeps trying in that circumstance, if qemu does not simply fall over > due to alloc failing for something else soon after. Surely better to keep running by dropping nonessential services so that the guest might get a chance to shut down or the host might recover. > I'd rather it behaves like a broken HWRNG if it can't get host > entropy: Don't provide data, and let the guest decide what to do, just > like it does for a broken HWRNG. It does. > Except virtio-rng can report unavailability rather than simply being broken :-) It could, in theory. -Ian
Ian Molton <ian.molton@collabora.co.uk> writes: > Markus Armbruster wrote: > >> p = malloc(n * sizeof(struct foo); >> if (n && !p) >> exit_no_mem(); >> for (i = 0; i < n; i++) >> compute_one(p, i); >> >> With qemu_malloc(), the error handling moves into qemu_malloc(): >> >> p = qemu_malloc(n * sizeof(struct foo); >> for (i = 0; i < n; i++) >> compute_one(p, i); > > And you lose the ability to fail gracefully... That's a deliberate choice. It has its drawbacks, it has its advantages. And it's not related to the question at hand: permitting zero arguments.
On 12/06/2009 08:41 PM, malc wrote: > Sure. My point is that sometimes failure to allocate is due to bugs, > invalid input etc, and conversion to OOM aborts en masse, might have > not been the best possible route to take, but most likely it was better > than doing nothing. > I agree. Early oom handling does limit opportunities for recovery in the cases where it is possible/easy. We can have an alternative API that doesn't do oom handling for those cases where it is desirable.
On 12/07/2009 10:39 AM, Ian Molton wrote: > Send what message to the host? If the malloc in the socet reconnect code > fails, its the code on the host thats going to flag up that malloc failed. He means to management code. >> > and if it does manage to receive the message, you can be almost >> > certain that it won't be able to run any GUI operations, send mail, >> > etc. to inform the admin. > > OTOH, If all it does it log it to a file, theres a fair chance it might > succeed. Not necessarily, for example fprintf may fail. There _may_ be a fair chance to succeed if you use write(2) directly, but that's it basically, and ENOMEM is always there waiting for you... Paolo
Jamie Lokier <jamie@shareable.org> writes: > Ian Molton wrote: >> > Read the beginning of the thread. Basically it's for arrays, malloc(n * >> > sizeof(x)). >> >> well, make sure n is not 0. Its not that hard. I dont think I've *ever* >> had a situation where I wanted to pass 0 to malloc. > > I would like to remind everyone that sizeof(x) can be 0 too. For > example, on Linux sizeof(spinlock_t) == 0 on UP. Anything where you > have a bunch of structure fields which depend on compile time > configuration, or where a type might be replaced by a stub empty > structure, is a possible sizeof(x) == 0. Therefore, outlawing qemu_malloc(0) impacts abstract data types: given some abstract type T, about which you're not supposed to assume anything, you can't do qemu_malloc(sizeof(T)). If anybody here is into outlawing scary zeroes, types with size zero looks like another one to outlaw. >> Its not that hard. > > The fact is there are a number of bugs in qemu where n == 0 is not > checked prior to calling qemu_malloc() at the moment. None of them > are "hard" to fix - they are rare cases that nobody noticed when > writing them. Hard or not so hard, what would "fixing" them buy us? I keep asking this question, no takers so far. > Until we have code analysis tools checking for that, bugs of that kind > will probably keep arising. Correct.
Am 07.12.2009 10:47, schrieb Avi Kivity: > On 12/06/2009 08:41 PM, malc wrote: >> Sure. My point is that sometimes failure to allocate is due to bugs, >> invalid input etc, and conversion to OOM aborts en masse, might have >> not been the best possible route to take, but most likely it was better >> than doing nothing. >> > > I agree. Early oom handling does limit opportunities for recovery in > the cases where it is possible/easy. We can have an alternative API > that doesn't do oom handling for those cases where it is desirable. You could simply use normal malloc as an alternative API there. After all, the only thing that qemu_malloc adds is the OOM check (and currently also the zero check). Kevin
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. 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. > > * load_elf32(), load_elf64() in hw/elf_ops.h: > > size = ehdr.e_phnum * sizeof(phdr[0]); > lseek(fd, ehdr.e_phoff, SEEK_SET); > phdr = qemu_mallocz(size); > > This aborts when the ELF file has no program header table, because > then e_phnum is zero (TIS ELF 1.2 page 1-6). Even if we require the > ELF file to have a program header table, aborting is not an acceptable > way to reject an unsuitable or malformed ELF file. If there's a malformed ELF files that must be supported (and by supported - user notification is meant) then things should be checked, because having gargantuan size will force qemu_mallocz to abort via OOM check (even with Linux and overcommit, since malloc will fallback to mmap which will fail) and this argument falls apart. > * load_elf32(), load_elf64() in hw/elf_ops.h: > > mem_size = ph->p_memsz; > /* XXX: avoid allocating */ > data = qemu_mallocz(mem_size); > > This aborts when the segment occupies zero bytes in the image file > (TIS ELF 1.2 page 2-2). > > * bdrv_open2() in block.c: > > bs->opaque = qemu_mallocz(drv->instance_size); > > However, vvfat_write_target.instance_size is zero. Not sure whether > this actually bites or is "only" a time bomb. > > * qemu_aio_get() in block.c: > > acb = qemu_mallocz(pool->aiocb_size); > > No existing instance of BlockDriverAIOCB has a zero aiocb_size. > Probably okay. > > * defaultallocator_create_displaysurface() in console.c has > > surface->data = (uint8_t*) qemu_mallocz(surface->linesize * surface->height); > > With Xen, surface->linesize and surface->height come out of > xenfb_configure_fb(), which set xenfb->width and xenfb->height to > values obtained from the guest. If a buggy guest sets one to zero, we > abort. Not an good way to handle that. What is? Let's suppose height is zero, without explicit checks, user will never know why his screen doesn't update, with abort he will at least know that something is very wrong. > Non-Xen uses aren't obviously correct either, but I didn't dig deeper. > > * load_device_tree() in device_tree.c: > > *sizep = 0; > dt_size = get_image_size(filename_path); > if (dt_size < 0) { > printf("Unable to get size of device tree file '%s'\n", > filename_path); > goto fail; > } > > /* Expand to 2x size to give enough room for manipulation. */ > dt_size *= 2; > /* First allocate space in qemu for device tree */ > fdt = qemu_mallocz(dt_size); > > We abort if the image is empty. Not an acceptable way to handle that. > > Based on this sample, I'm confident there are many more uses broken by > the change. Based on this sample i'm confident, we can eventually fix them should those become the problem. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/qcow2-snapshot.c | 5 ----- > qemu-malloc.c | 14 ++------------ > 2 files changed, 2 insertions(+), 17 deletions(-) > > diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c > index 94cb838..e3b208c 100644 > --- a/block/qcow2-snapshot.c > +++ b/block/qcow2-snapshot.c > @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) > QCowSnapshot *sn; > int i; > > - if (!s->nb_snapshots) { > - *psn_tab = NULL; > - return s->nb_snapshots; > - } > - > sn_tab = qemu_mallocz(s->nb_snapshots * sizeof(QEMUSnapshotInfo)); > for(i = 0; i < s->nb_snapshots; i++) { > sn_info = sn_tab + i; > diff --git a/qemu-malloc.c b/qemu-malloc.c > index 295d185..aeeb78b 100644 > --- a/qemu-malloc.c > +++ b/qemu-malloc.c > @@ -44,22 +44,12 @@ void qemu_free(void *ptr) > > void *qemu_malloc(size_t size) > { > - if (!size) { > - abort(); > - } > - return oom_check(malloc(size)); > + return oom_check(malloc(size ? size : 1)); > } > > void *qemu_realloc(void *ptr, size_t size) > { > - if (size) { > - return oom_check(realloc(ptr, size)); > - } else { > - if (ptr) { > - return realloc(ptr, size); > - } > - } > - abort(); > + return oom_check(realloc(ptr, size ? size : 1)); > } http://groups.google.com/group/comp.std.c/browse_thread/thread/4e9af8847613d71f/6f75ad22e0768a0b?q=realloc++group:comp.std.c#6f75ad22e0768a0b
On 12/07/2009 11:55 AM, Paolo Bonzini wrote: >> OTOH, If all it does it log it to a file, theres a fair chance it might >> succeed. > > > Not necessarily, for example fprintf may fail. There _may_ be a fair > chance to succeed if you use write(2) directly, but that's it > basically, and ENOMEM is always there waiting for you... Right, if malloc() failed write(2) is likely to fail as well. More likely, in fact, since malloc() may have unused process memory to draw upon whereas write(2) can only allocate kernel memory.
malc <av1474@comtv.ru> 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. > 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. 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? 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. Initially, all allocations should be treated as "unknown". I want a way to mark an allocation as "known empty fine". I figure you want a way to mark "known empty bad". >> * load_elf32(), load_elf64() in hw/elf_ops.h: >> >> size = ehdr.e_phnum * sizeof(phdr[0]); >> lseek(fd, ehdr.e_phoff, SEEK_SET); >> phdr = qemu_mallocz(size); >> >> This aborts when the ELF file has no program header table, because >> then e_phnum is zero (TIS ELF 1.2 page 1-6). Even if we require the >> ELF file to have a program header table, aborting is not an acceptable >> way to reject an unsuitable or malformed ELF file. > > If there's a malformed ELF files that must be supported (and by supported > - user notification is meant) then things should be checked, because having > gargantuan size will force qemu_mallocz to abort via OOM check (even > with Linux and overcommit, since malloc will fallback to mmap which > will fail) and this argument falls apart. ELF files with empty PHT are *not* malformed. Empty PHT is explicitly permitted by ELF TIS. Likewise for empty segments. I already gave you chapter & verse. >> * load_elf32(), load_elf64() in hw/elf_ops.h: >> >> mem_size = ph->p_memsz; >> /* XXX: avoid allocating */ >> data = qemu_mallocz(mem_size); >> >> This aborts when the segment occupies zero bytes in the image file >> (TIS ELF 1.2 page 2-2). >> >> * bdrv_open2() in block.c: >> >> bs->opaque = qemu_mallocz(drv->instance_size); >> >> However, vvfat_write_target.instance_size is zero. Not sure whether >> this actually bites or is "only" a time bomb. >> >> * qemu_aio_get() in block.c: >> >> acb = qemu_mallocz(pool->aiocb_size); >> >> No existing instance of BlockDriverAIOCB has a zero aiocb_size. >> Probably okay. >> >> * defaultallocator_create_displaysurface() in console.c has >> >> surface->data = (uint8_t*) qemu_mallocz(surface->linesize * surface->height); >> >> With Xen, surface->linesize and surface->height come out of >> xenfb_configure_fb(), which set xenfb->width and xenfb->height to >> values obtained from the guest. If a buggy guest sets one to zero, we >> abort. Not an good way to handle that. > > What is? Let's suppose height is zero, without explicit checks, user > will never know why his screen doesn't update, with abort he will at > least know that something is very wrong. My point isn't that permitting malloc(0) is the best way to handle it. It's a better way than aborting, though. I'm not arguing against treating case 0 specially ever. >> Non-Xen uses aren't obviously correct either, but I didn't dig deeper. >> >> * load_device_tree() in device_tree.c: >> >> *sizep = 0; >> dt_size = get_image_size(filename_path); >> if (dt_size < 0) { >> printf("Unable to get size of device tree file '%s'\n", >> filename_path); >> goto fail; >> } >> >> /* Expand to 2x size to give enough room for manipulation. */ >> dt_size *= 2; >> /* First allocate space in qemu for device tree */ >> fdt = qemu_mallocz(dt_size); >> >> We abort if the image is empty. Not an acceptable way to handle that. >> >> Based on this sample, I'm confident there are many more uses broken by >> the change. > > Based on this sample i'm confident, we can eventually fix them should those > become the problem. You broke working code. I'm glad you're confident we can fix it "eventually". What about 0.12? Ship it broken? >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/qcow2-snapshot.c | 5 ----- >> qemu-malloc.c | 14 ++------------ >> 2 files changed, 2 insertions(+), 17 deletions(-) >> >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 94cb838..e3b208c 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) >> QCowSnapshot *sn; >> int i; >> >> - if (!s->nb_snapshots) { >> - *psn_tab = NULL; >> - return s->nb_snapshots; >> - } >> - >> sn_tab = qemu_mallocz(s->nb_snapshots * sizeof(QEMUSnapshotInfo)); >> for(i = 0; i < s->nb_snapshots; i++) { >> sn_info = sn_tab + i; >> diff --git a/qemu-malloc.c b/qemu-malloc.c >> index 295d185..aeeb78b 100644 >> --- a/qemu-malloc.c >> +++ b/qemu-malloc.c >> @@ -44,22 +44,12 @@ void qemu_free(void *ptr) >> >> void *qemu_malloc(size_t size) >> { >> - if (!size) { >> - abort(); >> - } >> - return oom_check(malloc(size)); >> + return oom_check(malloc(size ? size : 1)); >> } >> >> void *qemu_realloc(void *ptr, size_t size) >> { >> - if (size) { >> - return oom_check(realloc(ptr, size)); >> - } else { >> - if (ptr) { >> - return realloc(ptr, size); >> - } >> - } >> - abort(); >> + return oom_check(realloc(ptr, size ? size : 1)); >> } > > http://groups.google.com/group/comp.std.c/browse_thread/thread/4e9af8847613d71f/6f75ad22e0768a0b?q=realloc++group:comp.std.c#6f75ad22e0768a0b --verbose ?
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. > While it's always fun to argue about standards interpretation, I wanted to capture some action items from the discussion that I think there is agreement about. Since I want to make changes for 0.12, I think it would be best to try and settle these now so we can do this before -rc2. For 0.12.0-rc2: I will send out a patch tonight or tomorrow changing qemu_malloc() to return malloc(1) when size=0 only for production builds (via --enable-zero-mallocs). Development trees will maintain their current behavior. For 0.13: Someone (Marcus?) will introduce four new allocation functions. type *qemu_new(type, n_types); type *qemu_new0(type, n_types); type *qemu_renew(type, mem, n_types); type *qemu_renew0(type, mem, n_types); NB: The names are borrowed from glib. I'm not tied to them. Will do our best to convert old code to use these functions where ever possible. New code will be required to use these functions unless not possible. n_types==0 is valid. sizeof(type)==0 is valid. Both circumstances return a unique non-NULL pointer. If memory allocation fails, execution will abort. The existing qemu_malloc() will maintain it's current behavior (with the exception of the relaxed size==0 assertion for production releases). Does anyone object to this moving forward? Regards, Anthony Liguori
On 12/07/2009 05:50 PM, Anthony Liguori wrote: > > While it's always fun to argue about standards interpretation, I > wanted to capture some action items from the discussion that I think > there is agreement about. Since I want to make changes for 0.12, I > think it would be best to try and settle these now so we can do this > before -rc2. > > For 0.12.0-rc2: > > I will send out a patch tonight or tomorrow changing qemu_malloc() to > return malloc(1) when size=0 only for production builds (via > --enable-zero-mallocs). Development trees will maintain their current > behavior. > Since active development is ceasing on 0.12, I'd suggest not having separate behaviour for devel and production. Do we want patches for n==0 array allocations at this time? I'd really like to see Markus' patch applied. > For 0.13: > > Someone (Marcus?) will introduce four new allocation functions. > > type *qemu_new(type, n_types); > type *qemu_new0(type, n_types); > > type *qemu_renew(type, mem, n_types); > type *qemu_renew0(type, mem, n_types); > I'd like to see separate functions for arrays and single objects, to avoid ", 1)" everywhere. qemu_new() qemu_new0() qemu_new_array() qemu_new_array0() qemu_renew_array() qemu_renew_array0() In addition, Markus' patch should be applied to master to avoid regressions while the code is converted.
Avi Kivity wrote: > On 12/07/2009 05:50 PM, Anthony Liguori wrote: >> >> While it's always fun to argue about standards interpretation, I >> wanted to capture some action items from the discussion that I think >> there is agreement about. Since I want to make changes for 0.12, I >> think it would be best to try and settle these now so we can do this >> before -rc2. >> >> For 0.12.0-rc2: >> >> I will send out a patch tonight or tomorrow changing qemu_malloc() to >> return malloc(1) when size=0 only for production builds (via >> --enable-zero-mallocs). Development trees will maintain their >> current behavior. >> > > Since active development is ceasing on 0.12, I'd suggest not having > separate behaviour for devel and production. Do we want patches for > n==0 array allocations at this time? Covering every qemu_malloc instance this close to the GA is too risky. I agree that having separate behavior is less than ideal but I think it's the only sane way forward. > I'd really like to see Markus' patch applied. For 0.12, that doesn't seem like a possibility. >> For 0.13: >> >> Someone (Marcus?) will introduce four new allocation functions. >> >> type *qemu_new(type, n_types); >> type *qemu_new0(type, n_types); >> >> type *qemu_renew(type, mem, n_types); >> type *qemu_renew0(type, mem, n_types); >> > > I'd like to see separate functions for arrays and single objects, to > avoid ", 1)" everywhere. > > qemu_new() > qemu_new0() > > qemu_new_array() > qemu_new_array0() > qemu_renew_array() > qemu_renew_array0() Like I said, I'm not tied to naming. I'll defer this to whoever contributes the patch and signs up for the conversion work. > In addition, Markus' patch should be applied to master to avoid > regressions while the code is converted. Let's separate that discussion as it's an independent consideration. Regards, Anthony Liguori
On 12/07/2009 06:06 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 12/07/2009 05:50 PM, Anthony Liguori wrote: >>> >>> While it's always fun to argue about standards interpretation, I >>> wanted to capture some action items from the discussion that I think >>> there is agreement about. Since I want to make changes for 0.12, I >>> think it would be best to try and settle these now so we can do this >>> before -rc2. >>> >>> For 0.12.0-rc2: >>> >>> I will send out a patch tonight or tomorrow changing qemu_malloc() >>> to return malloc(1) when size=0 only for production builds (via >>> --enable-zero-mallocs). Development trees will maintain their >>> current behavior. >>> >> >> Since active development is ceasing on 0.12, I'd suggest not having >> separate behaviour for devel and production. Do we want patches for >> n==0 array allocations at this time? > > Covering every qemu_malloc instance this close to the GA is too > risky. I agree that having separate behavior is less than ideal but I > think it's the only sane way forward. > I don't understand why. What's so insane about Markus' patch? Allowing size=0 for both developer and production builds? It seems like the least risky, least change approach to me. Exactly what we want for 0.12. >> I'd really like to see Markus' patch applied. > > For 0.12, that doesn't seem like a possibility. Please explain why. > >> In addition, Markus' patch should be applied to master to avoid >> regressions while the code is converted. > > Let's separate that discussion as it's an independent consideration. > I've asked for qemu-malloc-discuss@vger.kernel.org to be created for this purpose.
Avi Kivity wrote: >> Covering every qemu_malloc instance this close to the GA is too >> risky. I agree that having separate behavior is less than ideal but >> I think it's the only sane way forward. >> > > I don't understand why. What's so insane about Markus' patch? > Allowing size=0 for both developer and production builds? There is a bug here. Callers are calling qemu_malloc incorrectly. There is an open discussion about how to address it--fix all callers of qemu_malloc() or allow size=0. Since there isn't agreement, a compromise of sticking to the current behavior for the development tree, and using the later for production since we can't guarantee the former seems reasonable. > It seems like the least risky, least change approach to me. Exactly > what we want for 0.12. The risk is that everyone will agree to this approach in the next two weeks. I'm fairly certain no amount of discussion on qemu-devel is going to lead to that. >>> In addition, Markus' patch should be applied to master to avoid >>> regressions while the code is converted. >> >> Let's separate that discussion as it's an independent consideration. >> > > I've asked for qemu-malloc-discuss@vger.kernel.org to be created for > this purpose. :-) Regards, Anthony Liguori
> type *qemu_new(type, n_types); > type *qemu_new0(type, n_types); > > type *qemu_renew(type, mem, n_types); > type *qemu_renew0(type, mem, n_types); It always annoys me having to specify element count for things that aren't arrays. I suggestion a single object allocation function, and an array allocation function that allows zero length arrays. Possibly also a must-not-be-zero array form if we think it's important. Note that conversion to object/type based allocation is not always straightforward because inheritance means we don't have the final object type when doing the allocation. Paul
On 12/07/2009 06:20 PM, Anthony Liguori wrote: > Avi Kivity wrote: >>> Covering every qemu_malloc instance this close to the GA is too >>> risky. I agree that having separate behavior is less than ideal but >>> I think it's the only sane way forward. >>> >> >> I don't understand why. What's so insane about Markus' patch? >> Allowing size=0 for both developer and production builds? > > There is a bug here. Callers are calling qemu_malloc incorrectly. > > There is an open discussion about how to address it--fix all callers > of qemu_malloc() or allow size=0. Since there isn't agreement, a > compromise of sticking to the current behavior for the development > tree, and using the later for production since we can't guarantee the > former seems reasonable. If we apply the patch, the callers are no longer incorrect. Since we're winding down development on that tree, I see no reason for the production build to be correct and the development tree to be incorrect. >> It seems like the least risky, least change approach to me. Exactly >> what we want for 0.12. > > The risk is that everyone will agree to this approach in the next two > weeks. I'm fairly certain no amount of discussion on qemu-devel is > going to lead to that. You've already agreed to it for the production build. There's no gain in having separate behaviour for development and production, when a tree is headed towards production.
Paul Brook wrote: >> type *qemu_new(type, n_types); >> type *qemu_new0(type, n_types); >> >> type *qemu_renew(type, mem, n_types); >> type *qemu_renew0(type, mem, n_types); >> > > It always annoys me having to specify element count for things that aren't > arrays. > > I suggestion a single object allocation function, and an array allocation > function that allows zero length arrays. Possibly also a must-not-be-zero > array form if we think it's important. > Sure. > Note that conversion to object/type based allocation is not always > straightforward because inheritance means we don't have the final object type > when doing the allocation. > That's if you have the base object do the allocation, yup. > Paul >
On 12/07/2009 06:24 PM, Paul Brook wrote: > Note that conversion to object/type based allocation is not always > straightforward because inheritance means we don't have the final object type > when doing the allocation. > Instead of specifying the size, we can specify a constructor function (we usually do have an .init). It's still dissatisfying in that it has to duplicate the call to whatever it ends up being called, though.
Avi Kivity wrote: > On 12/07/2009 06:20 PM, Anthony Liguori wrote: >> Avi Kivity wrote: >>>> Covering every qemu_malloc instance this close to the GA is too >>>> risky. I agree that having separate behavior is less than ideal >>>> but I think it's the only sane way forward. >>>> >>> >>> I don't understand why. What's so insane about Markus' patch? >>> Allowing size=0 for both developer and production builds? >> >> There is a bug here. Callers are calling qemu_malloc incorrectly. >> >> There is an open discussion about how to address it--fix all callers >> of qemu_malloc() or allow size=0. Since there isn't agreement, a >> compromise of sticking to the current behavior for the development >> tree, and using the later for production since we can't guarantee the >> former seems reasonable. > > If we apply the patch, the callers are no longer incorrect. Since > we're winding down development on that tree, I see no reason for the > production build to be correct and the development tree to be incorrect. The problem with this whole discussion is taking the position that one form is "correct" while the other form is "incorrect". It's not so black and white. I don't think a reasonable person can claim that either form of qemu_malloc() is absolutely correct while the other is incorrect. You may think one is better than the other but clearly, that sentiment is not universally shared. If we change the production build, we eliminate the immediate problem. For 0.13, we can reduce the usage of qemu_malloc() such that it stops becoming an issue. Then no one ever has to agree about which form is better and we can move on with our lives. Regards, Anthony Liguori
On 12/07/2009 06:32 PM, Anthony Liguori wrote: >> If we apply the patch, the callers are no longer incorrect. Since >> we're winding down development on that tree, I see no reason for the >> production build to be correct and the development tree to be incorrect. > > > The problem with this whole discussion is taking the position that one > form is "correct" while the other form is "incorrect". It's not so > black and white. > > I don't think a reasonable person can claim that either form of > qemu_malloc() is absolutely correct while the other is incorrect. You > may think one is better than the other but clearly, that sentiment is > not universally shared. > I'm less concerned about qemu_malloc() and more about qemu itself (though qemu_malloc() as patched fulfils all the standard's requirements). > If we change the production build, we eliminate the immediate problem. What about developers that hit the assert? Do they send patches that fix code that works in production just so they can run in developer mode? > For 0.13, we can reduce the usage of qemu_malloc() such that it stops > becoming an issue. Then no one ever has to agree about which form is > better and we can move on with our lives. I agree, and the proposed changes are an improvement in their own right.
On Mon, 7 Dec 2009, Markus Armbruster wrote: > malc <av1474@comtv.ru> 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. > > > 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. > > 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. > > Initially, all allocations should be treated as "unknown". I want a way > to mark an allocation as "known empty fine". I figure you want a way to > mark "known empty bad". > > >> * load_elf32(), load_elf64() in hw/elf_ops.h: > >> > >> size = ehdr.e_phnum * sizeof(phdr[0]); > >> lseek(fd, ehdr.e_phoff, SEEK_SET); > >> phdr = qemu_mallocz(size); > >> > >> This aborts when the ELF file has no program header table, because > >> then e_phnum is zero (TIS ELF 1.2 page 1-6). Even if we require the > >> ELF file to have a program header table, aborting is not an acceptable > >> way to reject an unsuitable or malformed ELF file. > > > > If there's a malformed ELF files that must be supported (and by supported > > - user notification is meant) then things should be checked, because having > > gargantuan size will force qemu_mallocz to abort via OOM check (even > > with Linux and overcommit, since malloc will fallback to mmap which > > will fail) and this argument falls apart. > > ELF files with empty PHT are *not* malformed. Empty PHT is explicitly > permitted by ELF TIS. Likewise for empty segments. I already gave you > chapter & verse. Uh, it's you who brought the whole malformed issue. > > >> * load_elf32(), load_elf64() in hw/elf_ops.h: > >> > >> mem_size = ph->p_memsz; > >> /* XXX: avoid allocating */ > >> data = qemu_mallocz(mem_size); > >> > >> This aborts when the segment occupies zero bytes in the image file > >> (TIS ELF 1.2 page 2-2). > >> > >> * bdrv_open2() in block.c: > >> > >> bs->opaque = qemu_mallocz(drv->instance_size); > >> > >> However, vvfat_write_target.instance_size is zero. Not sure whether > >> this actually bites or is "only" a time bomb. > >> > >> * qemu_aio_get() in block.c: > >> > >> acb = qemu_mallocz(pool->aiocb_size); > >> > >> No existing instance of BlockDriverAIOCB has a zero aiocb_size. > >> Probably okay. > >> > >> * defaultallocator_create_displaysurface() in console.c has > >> > >> surface->data = (uint8_t*) qemu_mallocz(surface->linesize * surface->height); > >> > >> With Xen, surface->linesize and surface->height come out of > >> xenfb_configure_fb(), which set xenfb->width and xenfb->height to > >> values obtained from the guest. If a buggy guest sets one to zero, we > >> abort. Not an good way to handle that. > > > > What is? Let's suppose height is zero, without explicit checks, user > > will never know why his screen doesn't update, with abort he will at > > least know that something is very wrong. > > 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. > > I'm not arguing against treating case 0 specially ever. > > >> Non-Xen uses aren't obviously correct either, but I didn't dig deeper. > >> > >> * load_device_tree() in device_tree.c: > >> > >> *sizep = 0; > >> dt_size = get_image_size(filename_path); > >> if (dt_size < 0) { > >> printf("Unable to get size of device tree file '%s'\n", > >> filename_path); > >> goto fail; > >> } > >> > >> /* Expand to 2x size to give enough room for manipulation. */ > >> dt_size *= 2; > >> /* First allocate space in qemu for device tree */ > >> fdt = qemu_mallocz(dt_size); > >> > >> We abort if the image is empty. Not an acceptable way to handle that. > >> > >> Based on this sample, I'm confident there are many more uses broken by > >> the change. > > > > Based on this sample i'm confident, we can eventually fix them should those > > become the problem. > > You broke working code. I'm glad you're confident we can fix it > "eventually". What about 0.12? Ship it broken? I'm fixing those as they arrive. > > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > >> --- > >> block/qcow2-snapshot.c | 5 ----- > >> qemu-malloc.c | 14 ++------------ > >> 2 files changed, 2 insertions(+), 17 deletions(-) > >> > >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c > >> index 94cb838..e3b208c 100644 > >> --- a/block/qcow2-snapshot.c > >> +++ b/block/qcow2-snapshot.c > >> @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) > >> QCowSnapshot *sn; > >> int i; > >> > >> - if (!s->nb_snapshots) { > >> - *psn_tab = NULL; > >> - return s->nb_snapshots; > >> - } > >> - > >> sn_tab = qemu_mallocz(s->nb_snapshots * sizeof(QEMUSnapshotInfo)); > >> for(i = 0; i < s->nb_snapshots; i++) { > >> sn_info = sn_tab + i; > >> diff --git a/qemu-malloc.c b/qemu-malloc.c > >> index 295d185..aeeb78b 100644 > >> --- a/qemu-malloc.c > >> +++ b/qemu-malloc.c > >> @@ -44,22 +44,12 @@ void qemu_free(void *ptr) > >> > >> void *qemu_malloc(size_t size) > >> { > >> - if (!size) { > >> - abort(); > >> - } > >> - return oom_check(malloc(size)); > >> + return oom_check(malloc(size ? size : 1)); > >> } > >> > >> void *qemu_realloc(void *ptr, size_t size) > >> { > >> - if (size) { > >> - return oom_check(realloc(ptr, size)); > >> - } else { > >> - if (ptr) { > >> - return realloc(ptr, size); > >> - } > >> - } > >> - abort(); > >> + return oom_check(realloc(ptr, size ? size : 1)); > >> } > > > > 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 How anyone can use this interface and it's implementations portably or "sanely" is beyond me. Do read the discussion the linked above.
On Mon, 7 Dec 2009, Anthony Liguori wrote: > 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. > > > > While it's always fun to argue about standards interpretation, I wanted to > capture some action items from the discussion that I think there is agreement > about. Since I want to make changes for 0.12, I think it would be best to try > and settle these now so we can do this before -rc2. > > For 0.12.0-rc2: > > I will send out a patch tonight or tomorrow changing qemu_malloc() to return > malloc(1) when size=0 only for production builds (via --enable-zero-mallocs). > Development trees will maintain their current behavior. > > For 0.13: > > Someone (Marcus?) will introduce four new allocation functions. > > type *qemu_new(type, n_types); > type *qemu_new0(type, n_types); > > type *qemu_renew(type, mem, n_types); > type *qemu_renew0(type, mem, n_types); > > NB: The names are borrowed from glib. I'm not tied to them. > > Will do our best to convert old code to use these functions where ever > possible. New code will be required to use these functions unless not > possible. n_types==0 is valid. sizeof(type)==0 is valid. Both circumstances > return a unique non-NULL pointer. If memory allocation fails, execution will > abort. > > The existing qemu_malloc() will maintain it's current behavior (with the > exception of the relaxed size==0 assertion for production releases). > > Does anyone object to this moving forward? Yeah, i object to the split production/development qemu_malloc[z].
Avi Kivity wrote: > What about developers that hit the assert? Do they send patches that > fix code that works in production just so they can run in developer mode? Sounds like a good way to get developers to help convert from qemu_malloc() to qemu_new*() :-) If it helps, we can do a grep -l through qemu, put up a wiki page with all of the files, and allow people to sign up to convert individual files. I'm sure we could convert the whole tree over in a short period of time if we parallelize the effort and make some attempt to coordinate. Regards, Anthony Liguori
malc wrote: >> Does anyone object to this moving forward? >> > > Yeah, i object to the split production/development qemu_malloc[z]. > It's clear to me that there are still improper callers of qemu_malloc() in the tree. How do you propose we address this for 0.12? Aborting in a production build is a rather hostile thing to do if it can be avoided. Regards, Anthony Liguori
On 12/07/2009 06:59 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> What about developers that hit the assert? Do they send patches that >> fix code that works in production just so they can run in developer >> mode? > > Sounds like a good way to get developers to help convert from > qemu_malloc() to qemu_new*() :-) > In 0.12? My objection was to different behaviour of 0.12 in dev and production modes.
Avi Kivity wrote: > On 12/07/2009 06:59 PM, Anthony Liguori wrote: >> Avi Kivity wrote: >>> What about developers that hit the assert? Do they send patches >>> that fix code that works in production just so they can run in >>> developer mode? >> >> Sounds like a good way to get developers to help convert from >> qemu_malloc() to qemu_new*() :-) >> > > In 0.12? > > My objection was to different behaviour of 0.12 in dev and production > modes. It's a temporary problem that hopefully will be addressed quickly in the 0.13 cycle. Regards, Anthony Liguori
On Mon, 7 Dec 2009, Anthony Liguori wrote: > malc wrote: > > > Does anyone object to this moving forward? > > > > > > > Yeah, i object to the split production/development qemu_malloc[z]. > > > > It's clear to me that there are still improper callers of qemu_malloc() in the > tree. How do you propose we address this for 0.12? > > Aborting in a production build is a rather hostile thing to do if it can be > avoided. The only real issue encountered so far was eb0b64f7a, there are claims that "maybe there are more", well i can also claim that there are abusers of the interface that just weren't encoutered yet, and those will potentially lead to hard to track bugs, wiped out HDDs, general dissatisfaction and so on and so forth. Truth is that no one performed thorough audit so those are pure speculation. As for 0.12, go wild, i don't care, but only if it lives in it's own zero happy branch.
On 12/07/2009 07:09 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 12/07/2009 06:59 PM, Anthony Liguori wrote: >>> Avi Kivity wrote: >>>> What about developers that hit the assert? Do they send patches >>>> that fix code that works in production just so they can run in >>>> developer mode? >>> >>> Sounds like a good way to get developers to help convert from >>> qemu_malloc() to qemu_new*() :-) >>> >> >> In 0.12? >> >> My objection was to different behaviour of 0.12 in dev and production >> modes. > > It's a temporary problem that hopefully will be addressed quickly in > the 0.13 cycle. > I don't understand. People will develop patches for 0.12 for a while as bugs are reported and fixed.
Avi Kivity wrote: > I don't understand. People will develop patches for 0.12 for a while > as bugs are reported and fixed. What's the exact problem here? Regards, Anthony Liguori
On 12/07/2009 07:17 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> I don't understand. People will develop patches for 0.12 for a while >> as bugs are reported and fixed. > > What's the exact problem here? > Bug reported against qemu. Developer develops patch, while testing qemu crashes on unrelated assert(size == 0).
> So the only correct way would be to write: > > array = malloc(size); > if (array == NULL && size != 0) { > return -ENOMEM; > } > Of course we can differentiate. A failed malloc will abort, a successful one will not. > If you were writing portable code. But that's not what people write. You > can argue that qemu_malloc() can have any semantics we want and while that's > true, it doesn't change my above statement. I think the main argument for > this behavior in qemu is that people are used to using this idiom with > malloc() but it's a non-portable practice. > > If qemu_malloc() didn't carry the name "malloc()" then semantics with size=0 > would be a different discussion. But so far, all qemu_* functions tend to > behave almost exactly like their C counterparts. Relying on the result of > size=0 with malloc() is broken. We can change qemu_malloc to qemu_alloc_memory(), or whatever. But from the moment we do things like abort on failing, we are already deviating from its C counterpart.
Avi Kivity wrote: > On 12/07/2009 07:17 PM, Anthony Liguori wrote: >> Avi Kivity wrote: >>> I don't understand. People will develop patches for 0.12 for a >>> while as bugs are reported and fixed. >> >> What's the exact problem here? >> > > Bug reported against qemu. Developer develops patch, while testing > qemu crashes on unrelated assert(size == 0). 1) Developer develops a patch against 0.12, it works, and they submit it to upstream. 2) Upstream crashes because of assert(size==0). 3) Developer is publicly shamed for developing against a release instead of a git tree. The problem is (1), not (2). Not to mention that we won't allow qemu_malloc() uses in upstream anymore which should make (2) impossible. Regards, Anthony Liguori
On Mon, Dec 7, 2009 at 5:50 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > 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. >> > > While it's always fun to argue about standards interpretation, I wanted to > capture some action items from the discussion that I think there is > agreement about. Since I want to make changes for 0.12, I think it would be > best to try and settle these now so we can do this before -rc2. > > For 0.12.0-rc2: > > I will send out a patch tonight or tomorrow changing qemu_malloc() to return > malloc(1) when size=0 only for production builds (via > --enable-zero-mallocs). Development trees will maintain their current > behavior. > > For 0.13: > > Someone (Marcus?) will introduce four new allocation functions. > > type *qemu_new(type, n_types); > type *qemu_new0(type, n_types); > > type *qemu_renew(type, mem, n_types); > type *qemu_renew0(type, mem, n_types); > > NB: The names are borrowed from glib. I'm not tied to them. > > Will do our best to convert old code to use these functions where ever > possible. New code will be required to use these functions unless not > possible. n_types==0 is valid. sizeof(type)==0 is valid. Both > circumstances return a unique non-NULL pointer. If memory allocation fails, > execution will abort. > > The existing qemu_malloc() will maintain it's current behavior (with the > exception of the relaxed size==0 assertion for production releases). > > Does anyone object to this moving forward? Excellent plan.
On 12/07/2009 07:40 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 12/07/2009 07:17 PM, Anthony Liguori wrote: >>> Avi Kivity wrote: >>>> I don't understand. People will develop patches for 0.12 for a >>>> while as bugs are reported and fixed. >>> >>> What's the exact problem here? >>> >> >> Bug reported against qemu. Developer develops patch, while testing >> qemu crashes on unrelated assert(size == 0). > > 1) Developer develops a patch against 0.12, it works, and they submit > it to upstream. > 2) Upstream crashes because of assert(size==0). > 3) Developer is publicly shamed for developing against a release > instead of a git tree. > > The problem is (1), not (2). Not to mention that we won't allow > qemu_malloc() uses in upstream anymore which should make (2) impossible. My problem is with stable-0.12. Consider upstream fixed. 1) Bug reported against qemu-0.12.0. 2) Developer writes patch against master, submits, all is well except for the CODING_STYLE argument it triggers. 3) Developer writes patch against stable-0.12, can't test because testing crashes in some place where production doesn't crash. The patch doesn't have to do anything with memory allocation. By introducing gratuitous differences between developer and production mode, you're reducing quality.
Avi Kivity wrote: > My problem is with stable-0.12. Consider upstream fixed. > > 1) Bug reported against qemu-0.12.0. > 2) Developer writes patch against master, submits, all is well except > for the CODING_STYLE argument it triggers. > 3) Developer writes patch against stable-0.12, can't test because > testing crashes in some place where production doesn't crash. Stable-0.12 always carries a VERSION of 0.12.x where x < 50. This means that the stable-0.12 branch will always behave like a production release. You don't get -Werror on stable-0.XX and you won't get zero malloc()s assert. Regards, Anthony Liguori
On 12/07/2009 08:59 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> My problem is with stable-0.12. Consider upstream fixed. >> >> 1) Bug reported against qemu-0.12.0. >> 2) Developer writes patch against master, submits, all is well except >> for the CODING_STYLE argument it triggers. >> 3) Developer writes patch against stable-0.12, can't test because >> testing crashes in some place where production doesn't crash. > > Stable-0.12 always carries a VERSION of 0.12.x where x < 50. This > means that the stable-0.12 branch will always behave like a production > release. > > You don't get -Werror on stable-0.XX and you won't get zero malloc()s > assert. > That's good enough for me. Allow 0 for 0.12 and new allocation functions for mainline, then?
Avi Kivity wrote: > On 12/07/2009 08:59 PM, Anthony Liguori wrote: >> Avi Kivity wrote: >>> My problem is with stable-0.12. Consider upstream fixed. >>> >>> 1) Bug reported against qemu-0.12.0. >>> 2) Developer writes patch against master, submits, all is well >>> except for the CODING_STYLE argument it triggers. >>> 3) Developer writes patch against stable-0.12, can't test because >>> testing crashes in some place where production doesn't crash. >> >> Stable-0.12 always carries a VERSION of 0.12.x where x < 50. This >> means that the stable-0.12 branch will always behave like a >> production release. >> >> You don't get -Werror on stable-0.XX and you won't get zero malloc()s >> assert. >> > > That's good enough for me. Allow 0 for 0.12 and new allocation > functions for mainline, then? Yup. Regards, Anthony Liguori
malc <av1474@comtv.ru> writes: > On Mon, 7 Dec 2009, Markus Armbruster wrote: > >> malc <av1474@comtv.ru> 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 <aj@arthur.rhein-neckar.de> * 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.
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. >> > > While it's always fun to argue about standards interpretation, I > wanted to capture some action items from the discussion that I think > there is agreement about. Since I want to make changes for 0.12, I > think it would be best to try and settle these now so we can do this > before -rc2. > > For 0.12.0-rc2: > > I will send out a patch tonight or tomorrow changing qemu_malloc() to > return malloc(1) when size=0 only for production builds (via > --enable-zero-mallocs). Development trees will maintain their current > behavior. I don't think 0.12 needs the compile-time option, it's a production release after all. But since I won't actually hurt 0.12, I don't mind. > For 0.13: > > Someone (Marcus?) will introduce four new allocation functions. > > type *qemu_new(type, n_types); > type *qemu_new0(type, n_types); > > type *qemu_renew(type, mem, n_types); > type *qemu_renew0(type, mem, n_types); > > NB: The names are borrowed from glib. I'm not tied to them. I'll see what I can do post release. > Will do our best to convert old code to use these functions where ever > possible. New code will be required to use these functions unless not > possible. n_types==0 is valid. sizeof(type)==0 is valid. Both > circumstances return a unique non-NULL pointer. If memory allocation > fails, execution will abort. > > The existing qemu_malloc() will maintain it's current behavior (with > the exception of the relaxed size==0 assertion for production > releases). > > Does anyone object to this moving forward? Thanks, works for me.
Am 07.12.2009 18:09, schrieb malc: > On Mon, 7 Dec 2009, Anthony Liguori wrote: > >> malc wrote: >>>> Does anyone object to this moving forward? >>>> >>> >>> Yeah, i object to the split production/development qemu_malloc[z]. >>> >> >> It's clear to me that there are still improper callers of qemu_malloc() in the >> tree. How do you propose we address this for 0.12? >> >> Aborting in a production build is a rather hostile thing to do if it can be >> avoided. > > The only real issue encountered so far was eb0b64f7a, there are claims > that "maybe there are more", 702ef63f was the one that started this discussion. And Markus had list of five other places that can very likely crash with the abort. Every single crash is one crash too much in a production environment. Turning the abort off is the only sane option there. Having the abort in place in the development branch is a concession to you. Crashes don't have as bad impact there, so we probably can afford it. I'm fine with that as long as there is a plan to move forward to a better API. > well i can also claim that there are abusers > of the interface that just weren't encoutered yet, Right, but have you found a single one of them yet? If not, in terms of committed patches the score is 4:0 for legitimate users broken by abort() vs. abusers found by abort(). Kevin
On Tue, 8 Dec 2009, Markus Armbruster wrote: > malc <av1474@comtv.ru> writes: > [..snip..] > glibc frees since 1999. From glibc/ChangeLog.10: > > 1999-04-28 Andreas Jaeger <aj@arthur.rhein-neckar.de> > > * malloc/malloc.c (REALLOC_ZERO_BYTES_FREES): Define it to follow > ISO C9x and Unix98. Problem is - this is NOT what C9x where x > 0 says. > > > 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. >
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 94cb838..e3b208c 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -381,11 +381,6 @@ int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab) QCowSnapshot *sn; int i; - if (!s->nb_snapshots) { - *psn_tab = NULL; - return s->nb_snapshots; - } - sn_tab = qemu_mallocz(s->nb_snapshots * sizeof(QEMUSnapshotInfo)); for(i = 0; i < s->nb_snapshots; i++) { sn_info = sn_tab + i; diff --git a/qemu-malloc.c b/qemu-malloc.c index 295d185..aeeb78b 100644 --- a/qemu-malloc.c +++ b/qemu-malloc.c @@ -44,22 +44,12 @@ void qemu_free(void *ptr) void *qemu_malloc(size_t size) { - if (!size) { - abort(); - } - return oom_check(malloc(size)); + return oom_check(malloc(size ? size : 1)); } void *qemu_realloc(void *ptr, size_t size) { - if (size) { - return oom_check(realloc(ptr, size)); - } else { - if (ptr) { - return realloc(ptr, size); - } - } - abort(); + return oom_check(realloc(ptr, size ? size : 1)); } void *qemu_mallocz(size_t size)
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: * load_elf32(), load_elf64() in hw/elf_ops.h: size = ehdr.e_phnum * sizeof(phdr[0]); lseek(fd, ehdr.e_phoff, SEEK_SET); phdr = qemu_mallocz(size); This aborts when the ELF file has no program header table, because then e_phnum is zero (TIS ELF 1.2 page 1-6). Even if we require the ELF file to have a program header table, aborting is not an acceptable way to reject an unsuitable or malformed ELF file. * load_elf32(), load_elf64() in hw/elf_ops.h: mem_size = ph->p_memsz; /* XXX: avoid allocating */ data = qemu_mallocz(mem_size); This aborts when the segment occupies zero bytes in the image file (TIS ELF 1.2 page 2-2). * bdrv_open2() in block.c: bs->opaque = qemu_mallocz(drv->instance_size); However, vvfat_write_target.instance_size is zero. Not sure whether this actually bites or is "only" a time bomb. * qemu_aio_get() in block.c: acb = qemu_mallocz(pool->aiocb_size); No existing instance of BlockDriverAIOCB has a zero aiocb_size. Probably okay. * defaultallocator_create_displaysurface() in console.c has surface->data = (uint8_t*) qemu_mallocz(surface->linesize * surface->height); With Xen, surface->linesize and surface->height come out of xenfb_configure_fb(), which set xenfb->width and xenfb->height to values obtained from the guest. If a buggy guest sets one to zero, we abort. Not an good way to handle that. Non-Xen uses aren't obviously correct either, but I didn't dig deeper. * load_device_tree() in device_tree.c: *sizep = 0; dt_size = get_image_size(filename_path); if (dt_size < 0) { printf("Unable to get size of device tree file '%s'\n", filename_path); goto fail; } /* Expand to 2x size to give enough room for manipulation. */ dt_size *= 2; /* First allocate space in qemu for device tree */ fdt = qemu_mallocz(dt_size); We abort if the image is empty. Not an acceptable way to handle that. Based on this sample, I'm confident there are many more uses broken by the change. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/qcow2-snapshot.c | 5 ----- qemu-malloc.c | 14 ++------------ 2 files changed, 2 insertions(+), 17 deletions(-)