[Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

This patch is similar to a previous qemu_realloc() fix
(commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().

malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
this case.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-malloc.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/qemu-malloc.c b/qemu-malloc.c
index 6761857..2c60969 100644
--- a/qemu-malloc.c
+++ b/qemu-malloc.c
@@ -24,9 +24,9 @@
 #include "qemu-common.h"
 #include <stdlib.h>
 
-static void *oom_check(void *ptr)
+static void *oom_check(size_t size, void *ptr)
 {
-    if (ptr == NULL)
+    if (size != 0 && ptr == NULL)
         abort();
     return ptr;
 }
@@ -43,15 +43,12 @@ void qemu_free(void *ptr)
 
 void *qemu_malloc(size_t size)
 {
-    return oom_check(malloc(size));
+    return oom_check(size, malloc(size));
 }
 
 void *qemu_realloc(void *ptr, size_t size)
 {
-    if (size)
-        return oom_check(realloc(ptr, size));
-    else
-        return realloc(ptr, size);
+    return oom_check(size, realloc(ptr, size));
 }
 
 void *qemu_mallocz(size_t size)
-- 
1.6.3.rc4.29.g8146

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Mon, 18 May 2009, Eduardo Habkost wrote:

> This patch is similar to a previous qemu_realloc() fix
> (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
> 
> malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
> this case.

Only it wouldn't (on Linux):

$ cat malloc.c
#include <stdlib.h>

int main (void)
{
    printf ("%p\n", malloc (0));
    return 0;
}
$ gcc malloc.c
$ ./a.out 
0x10011008

Standard (in 7.20.3) says that malloc's behaviour in case of size being
zero is implementation defined.

Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.

The best(only?) thing to do is to check size passed to qemu_malloc[z]
and abort the program if this situation is encountered.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
> On Mon, 18 May 2009, Eduardo Habkost wrote:
> 
> > This patch is similar to a previous qemu_realloc() fix
> > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
> > 
> > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
> > this case.
> 
> Only it wouldn't (on Linux):
> 
> $ cat malloc.c
> #include <stdlib.h>
> 
> int main (void)
> {
>     printf ("%p\n", malloc (0));
>     return 0;
> }
> $ gcc malloc.c
> $ ./a.out 
> 0x10011008
> 
> Standard (in 7.20.3) says that malloc's behaviour in case of size being
> zero is implementation defined.
> 
> Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
> 
> The best(only?) thing to do is to check size passed to qemu_malloc[z]
> and abort the program if this situation is encountered.

Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
or a pointer, and on any case the value can be safely passed to free()
later.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Mon, 18 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
> > On Mon, 18 May 2009, Eduardo Habkost wrote:
> > 
> > > This patch is similar to a previous qemu_realloc() fix
> > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
> > > 
> > > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
> > > this case.
> > 
> > Only it wouldn't (on Linux):
> > 
> > $ cat malloc.c
> > #include <stdlib.h>
> > 
> > int main (void)
> > {
> >     printf ("%p\n", malloc (0));
> >     return 0;
> > }
> > $ gcc malloc.c
> > $ ./a.out 
> > 0x10011008
> > 
> > Standard (in 7.20.3) says that malloc's behaviour in case of size being
> > zero is implementation defined.
> > 
> > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
> > 
> > The best(only?) thing to do is to check size passed to qemu_malloc[z]
> > and abort the program if this situation is encountered.
> 
> Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
> or a pointer, and on any case the value can be safely passed to free()
> later.

I believe you haven't examined the commit that i referenced. Thing is
existing code used to, i'd venture a guess accidentaly, rely on the
behaviour that current GLIBC provides and consequently failed to
operate on AIX where malloc(0) returns NULL, IOW making qemu_malloc[z]
return whatever the underlying system returns is just hiding the bugs,
the code becomes unportable.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Mon, 18 May 2009, Eduardo Habkost wrote:
>
>> On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
>> > On Mon, 18 May 2009, Eduardo Habkost wrote:
>> > 
>> > > This patch is similar to a previous qemu_realloc() fix
>> > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
>> > > 
>> > > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
>> > > this case.
>> > 
[...]
>> > Standard (in 7.20.3) says that malloc's behaviour in case of size being
>> > zero is implementation defined.
>> > 
>> > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
>> > 
>> > The best(only?) thing to do is to check size passed to qemu_malloc[z]
>> > and abort the program if this situation is encountered.
>> 
>> Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
>> or a pointer, and on any case the value can be safely passed to free()
>> later.
>
> I believe you haven't examined the commit that i referenced. Thing is

Well, I just did, but I don't get it.

    diff --git a/block-qcow2.c b/block-qcow2.c
    index 9aa7261..d4556ef 100644
    --- a/block-qcow2.c
    +++ b/block-qcow2.c
    @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs)
         int64_t offset;
         uint32_t extra_data_size;

    +    if (!s->nb_snapshots) {
    +        s->snapshots = NULL;
    +        s->snapshots_size = 0;
    +        return 0;
    +    }
    +
         offset = s->snapshots_offset;
         s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
         if (!s->snapshots)

Can't see what this hunk accomplishes.  If we remove it, the loop
rejects, and we thus execute:

    offset = s->snapshots_offset;
    s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
    s->snapshots_size = offset - s->snapshots_offset;
    return 0;

Next hunk:

    @@ -2023,8 +2029,10 @@ static int qcow_snapshot_create(BlockDriverState *bs,
         snapshots1 = qemu_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot));
         if (!snapshots1)
             goto fail;
    -    memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot));
    -    qemu_free(s->snapshots);
    +    if (s->snapshots) {
    +        memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot)
    );
    +        qemu_free(s->snapshots);
    +    }
         s->snapshots = snapshots1;
         s->snapshots[s->nb_snapshots++] = *sn;

Again, I can't see the point.  if !s->snapshots, then s->nb_snapshots ==
0, doesn't it?  memcpy(snapshots1, NULL, 0) does nothing.  free(NULL)
does nothing.

I'm sure you didn't patch this for nothing, so I must miss something.
Could you enlighten me?

> existing code used to, i'd venture a guess accidentaly, rely on the
> behaviour that current GLIBC provides and consequently failed to
> operate on AIX where malloc(0) returns NULL,

Common error.

>                                              IOW making qemu_malloc[z]
> return whatever the underlying system returns is just hiding the bugs,
> the code becomes unportable.

Matter of taste.

1. Deal with the implementation-definedness.  Every caller that could
   pass zero needs to take care not to confuse empty allocation with an
   out of memory condition.

   This is easier than it sounds when you check for out of memory in
   just one place, like we do.

2. Remove the implementation-definedness.  Easiest way is to detect zero
   size in a wrapper (for us: qemu_malloc()) and bump it to one.

qemu_realloc() currently uses 1.

realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
be kind of nice to keep that for qemu_realloc() and qemu_malloc().

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Tue, 19 May 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Mon, 18 May 2009, Eduardo Habkost wrote:
> >
> >> On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
> >> > On Mon, 18 May 2009, Eduardo Habkost wrote:
> >> > 
> >> > > This patch is similar to a previous qemu_realloc() fix
> >> > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
> >> > > 
> >> > > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
> >> > > this case.
> >> > 
> [...]
> >> > Standard (in 7.20.3) says that malloc's behaviour in case of size being
> >> > zero is implementation defined.
> >> > 
> >> > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
> >> > 
> >> > The best(only?) thing to do is to check size passed to qemu_malloc[z]
> >> > and abort the program if this situation is encountered.
> >> 
> >> Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
> >> or a pointer, and on any case the value can be safely passed to free()
> >> later.
> >
> > I believe you haven't examined the commit that i referenced. Thing is
> 
> Well, I just did, but I don't get it.

Pretty good sign that something is wrong, no?
> 
>     diff --git a/block-qcow2.c b/block-qcow2.c
>     index 9aa7261..d4556ef 100644
>     --- a/block-qcow2.c
>     +++ b/block-qcow2.c
>     @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs)
>          int64_t offset;
>          uint32_t extra_data_size;
> 
>     +    if (!s->nb_snapshots) {
>     +        s->snapshots = NULL;
>     +        s->snapshots_size = 0;
>     +        return 0;
>     +    }
>     +
>          offset = s->snapshots_offset;
>          s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
>          if (!s->snapshots)
> 
> Can't see what this hunk accomplishes.  If we remove it, the loop
> rejects, and we thus execute:
> 
>     offset = s->snapshots_offset;
>     s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
>     s->snapshots_size = offset - s->snapshots_offset;
>     return 0;

Uh, no.

$ git cat-file -p 63c75dcd669d011f438421980b4379827da4bb1c^:block-qcow2.c | sed -n 1811,1815p
    offset = s->snapshots_offset;
    s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
    if (!s->snapshots)
        goto fail;

So before the patch qemu_mallocz(0) call would return NULL, goto fail 
would be executed and `qcow_read_snapshots' will return -1 and QEMU
will fail to start with very helpful message:
qemu: could not open disk image <image name>

> 
> Next hunk:
> 
>     @@ -2023,8 +2029,10 @@ static int qcow_snapshot_create(BlockDriverState *bs,
>          snapshots1 = qemu_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot));
>          if (!snapshots1)
>              goto fail;
>     -    memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot));
>     -    qemu_free(s->snapshots);
>     +    if (s->snapshots) {
>     +        memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot)
>     );
>     +        qemu_free(s->snapshots);
>     +    }
>          s->snapshots = snapshots1;
>          s->snapshots[s->nb_snapshots++] = *sn;
> 
> Again, I can't see the point.  if !s->snapshots, then s->nb_snapshots ==
> 0, doesn't it?  memcpy(snapshots1, NULL, 0) does nothing.  free(NULL)
> does nothing.

Wish i had your confidence and analysis skills, after skimming over
2666 lines of C in block-qcow2.c i can safely assert that this is
indeed the only condition under which s->snapshots can be NULL, hence
this hunk.

> I'm sure you didn't patch this for nothing, so I must miss something.
> Could you enlighten me?
> 

I hope that the above explanation is good enough, please do tell if it's not.

Also please notice how throwing second implemenation-defined behaviour of
malloc in to the mix led to the hunk #2, in which you see no point.

> > existing code used to, i'd venture a guess accidentaly, rely on the
> > behaviour that current GLIBC provides and consequently failed to
> > operate on AIX where malloc(0) returns NULL,
> 
> Common error.

Made by a very skilled programmer i might add.

> >                                              IOW making qemu_malloc[z]
> > return whatever the underlying system returns is just hiding the bugs,
> > the code becomes unportable.
> 
> Matter of taste.
> 
> 1. Deal with the implementation-definedness.  Every caller that could
>    pass zero needs to take care not to confuse empty allocation with an
>    out of memory condition.
> 
>    This is easier than it sounds when you check for out of memory in
>    just one place, like we do.
> 
> 2. Remove the implementation-definedness.  Easiest way is to detect zero
>    size in a wrapper (for us: qemu_malloc()) and bump it to one.

And mine:
  3. Abort the program if somebody tries it. Because so far history thought
     me that nobody does 1.
> 
> qemu_realloc() currently uses 1.
> 
> realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
> 

qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part
about qemu_malloc escapes me.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Tue, 19 May 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>> > On Mon, 18 May 2009, Eduardo Habkost wrote:
>> >
>> >> On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
>> >> > On Mon, 18 May 2009, Eduardo Habkost wrote:
>> >> > 
>> >> > > This patch is similar to a previous qemu_realloc() fix
>> >> > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
>> >> > > 
>> >> > > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
>> >> > > this case.
>> >> > 
>> [...]
>> >> > Standard (in 7.20.3) says that malloc's behaviour in case of size being
>> >> > zero is implementation defined.
>> >> > 
>> >> > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
>> >> > 
>> >> > The best(only?) thing to do is to check size passed to qemu_malloc[z]
>> >> > and abort the program if this situation is encountered.
>> >> 
>> >> Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
>> >> or a pointer, and on any case the value can be safely passed to free()
>> >> later.
>> >
>> > I believe you haven't examined the commit that i referenced. Thing is
>> 
>> Well, I just did, but I don't get it.
>
> Pretty good sign that something is wrong, no?

That's why I'm asking :)

>>     diff --git a/block-qcow2.c b/block-qcow2.c
>>     index 9aa7261..d4556ef 100644
>>     --- a/block-qcow2.c
>>     +++ b/block-qcow2.c
>>     @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs)
>>          int64_t offset;
>>          uint32_t extra_data_size;
>> 
>>     +    if (!s->nb_snapshots) {
>>     +        s->snapshots = NULL;
>>     +        s->snapshots_size = 0;
>>     +        return 0;
>>     +    }
>>     +
>>          offset = s->snapshots_offset;
>>          s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
>>          if (!s->snapshots)
>> 
>> Can't see what this hunk accomplishes.  If we remove it, the loop
>> rejects, and we thus execute:
>> 
>>     offset = s->snapshots_offset;
>>     s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
>>     s->snapshots_size = offset - s->snapshots_offset;
>>     return 0;
>
> Uh, no.
>
> $ git cat-file -p 63c75dcd669d011f438421980b4379827da4bb1c^:block-qcow2.c | sed -n 1811,1815p
>     offset = s->snapshots_offset;
>     s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
>     if (!s->snapshots)
>         goto fail;
>
> So before the patch qemu_mallocz(0) call would return NULL, goto fail 
> would be executed and `qcow_read_snapshots' will return -1 and QEMU
> will fail to start with very helpful message:
> qemu: could not open disk image <image name>

The test went away in commit 3ec88e80.  The hunk became pointless only
then.

>> Next hunk:
>> 
>>     @@ -2023,8 +2029,10 @@ static int qcow_snapshot_create(BlockDriverState *bs,
>>          snapshots1 = qemu_malloc((s->nb_snapshots + 1) * sizeof(QCowSnapshot));
>>          if (!snapshots1)
>>              goto fail;
>>     -    memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot));
>>     -    qemu_free(s->snapshots);
>>     +    if (s->snapshots) {
>>     +        memcpy(snapshots1, s->snapshots, s->nb_snapshots * sizeof(QCowSnapshot)
>>     );
>>     +        qemu_free(s->snapshots);
>>     +    }
>>          s->snapshots = snapshots1;
>>          s->snapshots[s->nb_snapshots++] = *sn;
>> 
>> Again, I can't see the point.  if !s->snapshots, then s->nb_snapshots ==
>> 0, doesn't it?  memcpy(snapshots1, NULL, 0) does nothing.  free(NULL)
>> does nothing.
>
> Wish i had your confidence and analysis skills, after skimming over
> 2666 lines of C in block-qcow2.c i can safely assert that this is
> indeed the only condition under which s->snapshots can be NULL, hence
> this hunk.
>
>> I'm sure you didn't patch this for nothing, so I must miss something.
>> Could you enlighten me?
>> 
>
> I hope that the above explanation is good enough, please do tell if it's not.

Thanks.

> Also please notice how throwing second implemenation-defined behaviour of
> malloc in to the mix led to the hunk #2, in which you see no point.
>
>> > existing code used to, i'd venture a guess accidentaly, rely on the
>> > behaviour that current GLIBC provides and consequently failed to
>> > operate on AIX where malloc(0) returns NULL,
>> 
>> Common error.
>
> Made by a very skilled programmer i might add.

I've fallen into this trap myself.

>> >                                              IOW making qemu_malloc[z]
>> > return whatever the underlying system returns is just hiding the bugs,
>> > the code becomes unportable.
>> 
>> Matter of taste.
>> 
>> 1. Deal with the implementation-definedness.  Every caller that could
>>    pass zero needs to take care not to confuse empty allocation with an
>>    out of memory condition.
>> 
>>    This is easier than it sounds when you check for out of memory in
>>    just one place, like we do.
>> 
>> 2. Remove the implementation-definedness.  Easiest way is to detect zero
>>    size in a wrapper (for us: qemu_malloc()) and bump it to one.
>
> And mine:
>   3. Abort the program if somebody tries it. Because so far history thought
>      me that nobody does 1.

Tries what?  Passing zero to qemu_malloc()?  That's legitimate.  And
with allocation functions that cannot return failure, it's hardly
dangerous, isn't it?

>> qemu_realloc() currently uses 1.
>> 
>> realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
>> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
>> 
>
> qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part
> about qemu_malloc escapes me.

qemu_malloc() & friends never fail.  Checking their value for failure is
pointless.  Therefore, 1. is practical.

2. is certainly practical as well.

3. is like 2, with the (size ? size : 1) pushed into callers.  I find
that mildly annoying.

I don't care whether we pick 1., 2. or 3., but I'd prefer we pick the
same for qemu_malloc() and qemu_realloc().  We currently use 1. for
qemu_realloc().  My point is: if you pick something else for
qemu_malloc(), please consider changing qemu_realloc().  That's all.

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Tue, 19 May 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 

[..snip..]

> >>     diff --git a/block-qcow2.c b/block-qcow2.c
> >>     index 9aa7261..d4556ef 100644
> >>     --- a/block-qcow2.c
> >>     +++ b/block-qcow2.c
> >>     @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs)
> >>          int64_t offset;
> >>          uint32_t extra_data_size;
> >> 
> >>     +    if (!s->nb_snapshots) {
> >>     +        s->snapshots = NULL;
> >>     +        s->snapshots_size = 0;
> >>     +        return 0;
> >>     +    }
> >>     +
> >>          offset = s->snapshots_offset;
> >>          s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> >>          if (!s->snapshots)
> >> 
> >> Can't see what this hunk accomplishes.  If we remove it, the loop
> >> rejects, and we thus execute:
> >> 

Once again, on Linux/GLIBC it will, on AIX it wont.

And FWIW despite behaviour of malloc(0) being marked as implementation
defined i have sa far was unable to find any documentaiton (Linux man
pages, GLIBC info files) witht the actual definition, unlike on AIX
where man pages make it crystal clear what happens.


> >>     offset = s->snapshots_offset;
> >>     s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> >>     s->snapshots_size = offset - s->snapshots_offset;
> >>     return 0;
> >
> > Uh, no.
> >
> > $ git cat-file -p 63c75dcd669d011f438421980b4379827da4bb1c^:block-qcow2.c | sed -n 1811,1815p
> >     offset = s->snapshots_offset;
> >     s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> >     if (!s->snapshots)
> >         goto fail;
> >
> > So before the patch qemu_mallocz(0) call would return NULL, goto fail 
> > would be executed and `qcow_read_snapshots' will return -1 and QEMU
> > will fail to start with very helpful message:
> > qemu: could not open disk image <image name>
> 
> The test went away in commit 3ec88e80.  The hunk became pointless only
> then.

Again, it's pointless only with your proposed addition, otherwise
instead of 'could not open disk image' one would get an out of memory
error.

[..snip..]

> 
> > Also please notice how throwing second implemenation-defined behaviour of
> > malloc in to the mix led to the hunk #2, in which you see no point.
> >
> >> > existing code used to, i'd venture a guess accidentaly, rely on the
> >> > behaviour that current GLIBC provides and consequently failed to
> >> > operate on AIX where malloc(0) returns NULL,
> >> 
> >> Common error.
> >
> > Made by a very skilled programmer i might add.
> 
> I've fallen into this trap myself.
> 
> >> >                                              IOW making qemu_malloc[z]
> >> > return whatever the underlying system returns is just hiding the bugs,
> >> > the code becomes unportable.
> >> 
> >> Matter of taste.
> >> 
> >> 1. Deal with the implementation-definedness.  Every caller that could
> >>    pass zero needs to take care not to confuse empty allocation with an
> >>    out of memory condition.
> >> 
> >>    This is easier than it sounds when you check for out of memory in
> >>    just one place, like we do.
> >> 
> >> 2. Remove the implementation-definedness.  Easiest way is to detect zero
> >>    size in a wrapper (for us: qemu_malloc()) and bump it to one.
> >
> > And mine:
> >   3. Abort the program if somebody tries it. Because so far history thought
> >      me that nobody does 1.
> 
> Tries what?  Passing zero to qemu_malloc()?  That's legitimate.  And
> with allocation functions that cannot return failure, it's hardly
> dangerous, isn't it?

That's legitimate only if one writes unportable code targeting single
system and knowing how it was defined. As for being dangerous, yes it
is: dereferencing the returned pointer, while UB, doesn't trigger a
SEGFAULT on, at least, this machine with Linux.

> >> qemu_realloc() currently uses 1.

void *qemu_realloc(void *ptr, size_t size)
{
    if (size)
        return oom_check(realloc(ptr, size));
    else
        return realloc(ptr, size);
}
 
There is nothing implementation defined about realloc(whatever, 0), it
has a defined meaning in POSIX:
http://opengroup.org/onlinepubs/007908775/xsh/realloc.html

So it doesn't use 1.

> >> 
> >> realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
> >> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
> >> 
> >
> > qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part
> > about qemu_malloc escapes me.
> 
> qemu_malloc() & friends never fail.  Checking their value for failure is
> pointless.  Therefore, 1. is practical.
> 
> 2. is certainly practical as well.
> 
> 3. is like 2, with the (size ? size : 1) pushed into callers.  I find
> that mildly annoying.

Huh, that's not at all what i proposed. What i had in mind is:

void *qemu_malloc(size_t size)
{
    if (!size) abort();
    return oom_check(malloc(size));
}

> 
> I don't care whether we pick 1., 2. or 3., but I'd prefer we pick the
> same for qemu_malloc() and qemu_realloc().  We currently use 1. for
> qemu_realloc().  My point is: if you pick something else for
> qemu_malloc(), please consider changing qemu_realloc().  That's all.

I see.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 06:06:56PM +0400, malc wrote:
> On Tue, 19 May 2009, Markus Armbruster wrote:
> 
> > malc <av1474@comtv.ru> writes:
> > 
> 
> [..snip..]
> 
> > >>     diff --git a/block-qcow2.c b/block-qcow2.c
> > >>     index 9aa7261..d4556ef 100644
> > >>     --- a/block-qcow2.c
> > >>     +++ b/block-qcow2.c
> > >>     @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs)
> > >>          int64_t offset;
> > >>          uint32_t extra_data_size;
> > >> 
> > >>     +    if (!s->nb_snapshots) {
> > >>     +        s->snapshots = NULL;
> > >>     +        s->snapshots_size = 0;
> > >>     +        return 0;
> > >>     +    }
> > >>     +
> > >>          offset = s->snapshots_offset;
> > >>          s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> > >>          if (!s->snapshots)
> > >> 
> > >> Can't see what this hunk accomplishes.  If we remove it, the loop
> > >> rejects, and we thus execute:
> > >> 
> 
> Once again, on Linux/GLIBC it will, on AIX it wont.

Why not? It will. If nb_snapshots is 0, it won't enter the loop. The
problem with that code was the "if (!s->snapshots)" check, not the
qemu_mallocz(0) call.


> 
> And FWIW despite behaviour of malloc(0) being marked as implementation
> defined i have sa far was unable to find any documentaiton (Linux man
> pages, GLIBC info files) witht the actual definition, unlike on AIX
> where man pages make it crystal clear what happens.

You don't need to have the exact behavior defined, as long as:

1) You call free(p) later
2) You don't dereference the returned pointer (just like you can't
   dereference p[n] on a malloc(n) block)
3) You don't assume anything about the returned value when size==0

My point is that this is valid malloc() usage, and there may be existing
qemu code relying on that, and I don't see any reason to put a trap for
code that would be valid malloc()/free() usage.


> 
> 
<snip>
> > 
> > Tries what?  Passing zero to qemu_malloc()?  That's legitimate.  And
> > with allocation functions that cannot return failure, it's hardly
> > dangerous, isn't it?
> 
> That's legitimate only if one writes unportable code targeting single
> system and knowing how it was defined.

No, that's legitimate and portable. You just can't assume anything about
the returned value.



> As for being dangerous, yes it
> is: dereferencing the returned pointer, while UB, doesn't trigger a
> SEGFAULT on, at least, this machine with Linux.
> 
> > >> qemu_realloc() currently uses 1.
> 
> void *qemu_realloc(void *ptr, size_t size)
> {
>     if (size)
>         return oom_check(realloc(ptr, size));
>     else
>         return realloc(ptr, size);
> }
>  
> There is nothing implementation defined about realloc(whatever, 0), it
> has a defined meaning in POSIX:
> http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> 
> So it doesn't use 1.
> 

realloc() return value is specified exactly the same way malloc() is:

"If size is 0, either a null pointer or a unique pointer that can be
successfully passed to free() is returned."


> > >> 
> > >> realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
> > >> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
> > >> 
> > >
> > > qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part
> > > about qemu_malloc escapes me.
> > 
> > qemu_malloc() & friends never fail.  Checking their value for failure is
> > pointless.  Therefore, 1. is practical.
> > 
> > 2. is certainly practical as well.
> > 
> > 3. is like 2, with the (size ? size : 1) pushed into callers.  I find
> > that mildly annoying.
> 
> Huh, that's not at all what i proposed. What i had in mind is:
> 
> void *qemu_malloc(size_t size)
> {
>     if (!size) abort();
>     return oom_check(malloc(size));
> }

Understood. And that's exactly what I think we should not do.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Tue, 19 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 06:06:56PM +0400, malc wrote:
> > On Tue, 19 May 2009, Markus Armbruster wrote:
> > 
> > > malc <av1474@comtv.ru> writes:
> > > 
> > 
> > [..snip..]
> > 
> > > >>     diff --git a/block-qcow2.c b/block-qcow2.c
> > > >>     index 9aa7261..d4556ef 100644
> > > >>     --- a/block-qcow2.c
> > > >>     +++ b/block-qcow2.c
> > > >>     @@ -1809,6 +1809,12 @@ static int qcow_read_snapshots(BlockDriverState *bs)
> > > >>          int64_t offset;
> > > >>          uint32_t extra_data_size;
> > > >> 
> > > >>     +    if (!s->nb_snapshots) {
> > > >>     +        s->snapshots = NULL;
> > > >>     +        s->snapshots_size = 0;
> > > >>     +        return 0;
> > > >>     +    }
> > > >>     +
> > > >>          offset = s->snapshots_offset;
> > > >>          s->snapshots = qemu_mallocz(s->nb_snapshots * sizeof(QCowSnapshot));
> > > >>          if (!s->snapshots)
> > > >> 
> > > >> Can't see what this hunk accomplishes.  If we remove it, the loop
> > > >> rejects, and we thus execute:
> > > >> 
> > 
> > Once again, on Linux/GLIBC it will, on AIX it wont.
> 
> Why not? It will. If nb_snapshots is 0, it won't enter the loop. The
> problem with that code was the "if (!s->snapshots)" check, not the
> qemu_mallocz(0) call.

Because qemu_mallocz on AIX will be terminated by oom_check.

> 
> 
> > 
> > And FWIW despite behaviour of malloc(0) being marked as implementation
> > defined i have sa far was unable to find any documentaiton (Linux man
> > pages, GLIBC info files) witht the actual definition, unlike on AIX
> > where man pages make it crystal clear what happens.
> 
> You don't need to have the exact behavior defined, as long as:

I certainly don't, the standard certainly says the implementation is
obliged to document it, that's what seprates implementation-defined
from unspecified behaviour.
 
> 1) You call free(p) later
> 2) You don't dereference the returned pointer (just like you can't
>    dereference p[n] on a malloc(n) block)
> 3) You don't assume anything about the returned value when size==0
> 
> My point is that this is valid malloc() usage, and there may be existing
> qemu code relying on that, and I don't see any reason to put a trap for
> code that would be valid malloc()/free() usage.

Okay, at this point we both expressed our points of view and have to
agree to disagree.
 
> 
> > 
> > 
> <snip>
> > > 
> > > Tries what?  Passing zero to qemu_malloc()?  That's legitimate.  And
> > > with allocation functions that cannot return failure, it's hardly
> > > dangerous, isn't it?
> > 
> > That's legitimate only if one writes unportable code targeting single
> > system and knowing how it was defined.
> 
> No, that's legitimate and portable. You just can't assume anything about
> the returned value.
> 
> 
> 
> > As for being dangerous, yes it
> > is: dereferencing the returned pointer, while UB, doesn't trigger a
> > SEGFAULT on, at least, this machine with Linux.
> > 
> > > >> qemu_realloc() currently uses 1.
> > 
> > void *qemu_realloc(void *ptr, size_t size)
> > {
> >     if (size)
> >         return oom_check(realloc(ptr, size));
> >     else
> >         return realloc(ptr, size);
> > }
> >  
> > There is nothing implementation defined about realloc(whatever, 0), it
> > has a defined meaning in POSIX:
> > http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> > 
> > So it doesn't use 1.
> > 
> 
> realloc() return value is specified exactly the same way malloc() is:
> 
> "If size is 0, either a null pointer or a unique pointer that can be
> successfully passed to free() is returned."

Nope, quoting from above page:

If size is 0 and ptr is not a null pointer, the object pointed to is 
freed.

> 
> 
> > > >> 
> > > >> realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
> > > >> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
> > > >> 
> > > >
> > > > qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part
> > > > about qemu_malloc escapes me.
> > > 
> > > qemu_malloc() & friends never fail.  Checking their value for failure is
> > > pointless.  Therefore, 1. is practical.
> > > 
> > > 2. is certainly practical as well.
> > > 
> > > 3. is like 2, with the (size ? size : 1) pushed into callers.  I find
> > > that mildly annoying.
> > 
> > Huh, that's not at all what i proposed. What i had in mind is:
> > 
> > void *qemu_malloc(size_t size)
> > {
> >     if (!size) abort();
> >     return oom_check(malloc(size));
> > }
> 
> Understood. And that's exactly what I think we should not do.
> 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 06:48:25PM +0400, malc wrote:
<snip>
> > > > >> 
> > > > >> Can't see what this hunk accomplishes.  If we remove it, the loop
> > > > >> rejects, and we thus execute:
> > > > >> 
> > > 
> > > Once again, on Linux/GLIBC it will, on AIX it wont.
> > 
> > Why not? It will. If nb_snapshots is 0, it won't enter the loop. The
> > problem with that code was the "if (!s->snapshots)" check, not the
> > qemu_mallocz(0) call.
> 
> Because qemu_mallocz on AIX will be terminated by oom_check.

That's exactly what the patch prevents from happening.


> 
> > > 
> > > And FWIW despite behaviour of malloc(0) being marked as implementation
> > > defined i have sa far was unable to find any documentaiton (Linux man
> > > pages, GLIBC info files) witht the actual definition, unlike on AIX
> > > where man pages make it crystal clear what happens.
> > 
> > You don't need to have the exact behavior defined, as long as:
> 
> I certainly don't, the standard certainly says the implementation is
> obliged to document it, that's what seprates implementation-defined
> from unspecified behaviour.
>  
> > 1) You call free(p) later
> > 2) You don't dereference the returned pointer (just like you can't
> >    dereference p[n] on a malloc(n) block)
> > 3) You don't assume anything about the returned value when size==0
> > 
> > My point is that this is valid malloc() usage, and there may be existing
> > qemu code relying on that, and I don't see any reason to put a trap for
> > code that would be valid malloc()/free() usage.
> 
> Okay, at this point we both expressed our points of view and have to
> agree to disagree.

Agreed.


<snip>
> > >  
> > > There is nothing implementation defined about realloc(whatever, 0), it
> > > has a defined meaning in POSIX:
> > > http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> > > 
> > > So it doesn't use 1.
> > > 
> > 
> > realloc() return value is specified exactly the same way malloc() is:
> > 
> > "If size is 0, either a null pointer or a unique pointer that can be
> > successfully passed to free() is returned."
> 
> Nope, quoting from above page:
> 
> If size is 0 and ptr is not a null pointer, the object pointed to is 
> freed.

I quoted the above from exactly the same page.

I really hope you are not proposing to make qemu_realloc(p, 0) work but
qemu_malloc(0) fail, because you would be breaking lots of
realloc()/malloc() equivalency assumptions.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Tue, 19 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 06:48:25PM +0400, malc wrote:
> <snip>
> > > > > >> 
> > > > > >> Can't see what this hunk accomplishes.  If we remove it, the loop
> > > > > >> rejects, and we thus execute:
> > > > > >> 
> > > > 
> > > > Once again, on Linux/GLIBC it will, on AIX it wont.
> > > 
> > > Why not? It will. If nb_snapshots is 0, it won't enter the loop. The
> > > problem with that code was the "if (!s->snapshots)" check, not the
> > > qemu_mallocz(0) call.
> > 
> > Because qemu_mallocz on AIX will be terminated by oom_check.
> 
> That's exactly what the patch prevents from happening.

And i said as much:

<quote>
Again, it's pointless only with your proposed addition, otherwise
instead of 'could not open disk image' one would get an out of memory
error.
</quote>

> 
> 
> > 
> > > > 
> > > > And FWIW despite behaviour of malloc(0) being marked as implementation
> > > > defined i have sa far was unable to find any documentaiton (Linux man
> > > > pages, GLIBC info files) witht the actual definition, unlike on AIX
> > > > where man pages make it crystal clear what happens.
> > > 
> > > You don't need to have the exact behavior defined, as long as:
> > 
> > I certainly don't, the standard certainly says the implementation is
> > obliged to document it, that's what seprates implementation-defined
> > from unspecified behaviour.
> >  
> > > 1) You call free(p) later
> > > 2) You don't dereference the returned pointer (just like you can't
> > >    dereference p[n] on a malloc(n) block)
> > > 3) You don't assume anything about the returned value when size==0
> > > 
> > > My point is that this is valid malloc() usage, and there may be existing
> > > qemu code relying on that, and I don't see any reason to put a trap for
> > > code that would be valid malloc()/free() usage.
> > 
> > Okay, at this point we both expressed our points of view and have to
> > agree to disagree.
> 
> Agreed.
> 
> 
> <snip>
> > > >  
> > > > There is nothing implementation defined about realloc(whatever, 0), it
> > > > has a defined meaning in POSIX:
> > > > http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> > > > 
> > > > So it doesn't use 1.
> > > > 
> > > 
> > > realloc() return value is specified exactly the same way malloc() is:
> > > 
> > > "If size is 0, either a null pointer or a unique pointer that can be
> > > successfully passed to free() is returned."
> > 
> > Nope, quoting from above page:
> > 
> > If size is 0 and ptr is not a null pointer, the object pointed to is 
> > freed.
> 
> I quoted the above from exactly the same page.
> 
> I really hope you are not proposing to make qemu_realloc(p, 0) work but
> qemu_malloc(0) fail, because you would be breaking lots of
> realloc()/malloc() equivalency assumptions.

That's exactly what i'm proposing.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 07:23:57PM +0400, malc wrote:
> On Tue, 19 May 2009, Eduardo Habkost wrote:
> 
> > On Tue, May 19, 2009 at 06:48:25PM +0400, malc wrote:
> > <snip>
> > > > > > >> 
> > > > > > >> Can't see what this hunk accomplishes.  If we remove it, the loop
> > > > > > >> rejects, and we thus execute:
> > > > > > >> 
> > > > > 
> > > > > Once again, on Linux/GLIBC it will, on AIX it wont.
> > > > 
> > > > Why not? It will. If nb_snapshots is 0, it won't enter the loop. The
> > > > problem with that code was the "if (!s->snapshots)" check, not the
> > > > qemu_mallocz(0) call.
> > > 
> > > Because qemu_mallocz on AIX will be terminated by oom_check.
> > 
> > That's exactly what the patch prevents from happening.
> 
> And i said as much:
> 
> <quote>
> Again, it's pointless only with your proposed addition, otherwise
> instead of 'could not open disk image' one would get an out of memory
> error.
> </quote>

We are already running in circles. I will stop arguing about that until
somebody else chimes in.


>
<snip>
> > > > 
> > > > realloc() return value is specified exactly the same way malloc() is:
> > > > 
> > > > "If size is 0, either a null pointer or a unique pointer that can be
> > > > successfully passed to free() is returned."
> > > 
> > > Nope, quoting from above page:
> > > 
> > > If size is 0 and ptr is not a null pointer, the object pointed to is 
> > > freed.
> > 
> > I quoted the above from exactly the same page.
> > 
> > I really hope you are not proposing to make qemu_realloc(p, 0) work but
> > qemu_malloc(0) fail, because you would be breaking lots of
> > realloc()/malloc() equivalency assumptions.
> 
> That's exactly what i'm proposing.

Now, _that_ sounds like a really bad idea. realloc(NULL, n) is specified
to be equivalent to malloc(n).

I can't prevent you from inventing a new malloc/free API that works
differently from every malloc/free implementation out there. All I can
say is that this sounds like a really bad idea.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Jamie Lokier]

Eduardo Habkost wrote:
> Now, _that_ sounds like a really bad idea. realloc(NULL, n) is specified
> to be equivalent to malloc(n).

No it isn't.  You can't make that substitution.

In the case where n == 0, realloc(NULL, n) is guaranteed to not
allocate anything and return NULL, whereas malloc(n) does not
guarantee that and in fact doesn't do that on a lot of implementations.

-- Jamie

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

(Replying only now because I wasn't CCed on the reply)

Excerpts from Jamie Lokier's message of Tue May 19 17:32:23 -0300 2009:
> Eduardo Habkost wrote:
> > Now, _that_ sounds like a really bad idea. realloc(NULL, n) is specified
> > to be equivalent to malloc(n).
> 
> No it isn't.  You can't make that substitution.
> 
> In the case where n == 0, realloc(NULL, n) is guaranteed to not
> allocate anything and return NULL, whereas malloc(n) does not
> guarantee that and in fact doesn't do that on a lot of implementations.

http://opengroup.org/onlinepubs/007908775/xsh/realloc.html

"If ptr is a null pointer, realloc() behaves like malloc() for the
specified size."

"If size is 0, either a null pointer or a unique pointer that can be
successfully passed to free() is returned."
-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Jamie Lokier]

Eduardo Habkost wrote:
> Excerpts from Jamie Lokier's message of Tue May 19 17:32:23 -0300 2009:
> > Eduardo Habkost wrote:
> > > Now, _that_ sounds like a really bad idea. realloc(NULL, n) is specified
> > > to be equivalent to malloc(n).
> > 
> > No it isn't.  You can't make that substitution.
> > 
> > In the case where n == 0, realloc(NULL, n) is guaranteed to not
> > allocate anything and return NULL, whereas malloc(n) does not
> > guarantee that and in fact doesn't do that on a lot of implementations.
> 
> http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> 
> "If ptr is a null pointer, realloc() behaves like malloc() for the
> specified size."
> 
> "If size is 0, either a null pointer or a unique pointer that can be
> successfully passed to free() is returned."

Oh.  Thanks!  I stand corrected; sorry for propagating misinformation.

The relevant part in the standard which makes the above not contradict
realloc's freeing behaviour, is "if size is 0 and __ptr is not a null
pointer__, the object pointed to is freed."

All this creates a different problem, unfortunately:

If you do:

    p = malloc(n)         /* Arbitrary n, could be zero. */
    free(p)

it works, but

    p = realloc(oldp, n)  /* Arbitrary n, could be zero. */
    realloc(p, 0)

is not guaranteed to free the allocated block, if n == 0 && p == NULL.

realloc(p, 0) is not equivalent to free(p) in this problem cases.

Which is IMHO another reason to either forbid n == 0, or warn about
it, or change it unambiguously to n == 1 in the qemu_ wrappers.

While we're here, the C language FAQ adds:

Q: Is it legal to pass a null pointer as the first argument to
    realloc? Why would you want to?

A: ANSI C sanctions this usage (and the related realloc(..., 0), which
   frees), although several earlier implementations do not support it, so
   it may not be fully portable.

-- Jamie

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 11:49:54PM +0100, Jamie Lokier wrote:
> Eduardo Habkost wrote:
> > Excerpts from Jamie Lokier's message of Tue May 19 17:32:23 -0300 2009:
> > > Eduardo Habkost wrote:
> > > > Now, _that_ sounds like a really bad idea. realloc(NULL, n) is specified
> > > > to be equivalent to malloc(n).
> > > 
> > > No it isn't.  You can't make that substitution.
> > > 
> > > In the case where n == 0, realloc(NULL, n) is guaranteed to not
> > > allocate anything and return NULL, whereas malloc(n) does not
> > > guarantee that and in fact doesn't do that on a lot of implementations.
> > 
> > http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> > 
> > "If ptr is a null pointer, realloc() behaves like malloc() for the
> > specified size."
> > 
> > "If size is 0, either a null pointer or a unique pointer that can be
> > successfully passed to free() is returned."
> 
> Oh.  Thanks!  I stand corrected; sorry for propagating misinformation.

:-)

> 
> The relevant part in the standard which makes the above not contradict
> realloc's freeing behaviour, is "if size is 0 and __ptr is not a null
> pointer__, the object pointed to is freed."
> 
> All this creates a different problem, unfortunately:
> 
> If you do:
> 
>     p = malloc(n)         /* Arbitrary n, could be zero. */
>     free(p)
> 
> it works, but
> 
>     p = realloc(oldp, n)  /* Arbitrary n, could be zero. */
>     realloc(p, 0)
> 
> is not guaranteed to free the allocated block, if n == 0 && p == NULL.

It is a problem only if you drop the return value of the second
realloc() call. You shouldn't do that.

> 
> realloc(p, 0) is not equivalent to free(p) in this problem cases.

Yes. realloc(p, 0) frees p, but it is not exactly equivalent to free(p)
because it does more than freeing p: it returns a new value you need to
free later. You still need to save the returned value and eventually
pass it to free() (or maybe another realloc() call, whose return value
will need to be saved, and so on).

> 
> Which is IMHO another reason to either forbid n == 0, or warn about
> it, or change it unambiguously to n == 1 in the qemu_ wrappers.

The example you have shown ignores the return value of realloc() and
that's the cause of the problem, not the possiblity of n==0.

That's the beauty of it: n==0 and realloc(NULL, n) don't need to be
treated like a special case by the caller, it Just Works. The only thing
you need to be careful about is not checking for NULL if you call
realloc(p, 0) or malloc(0). And, obviously, it has the same requirements
that apply for any other allocations, such as freeing it eventually, and
not making any out-of-bounds dereference of the returned pointer.

> 
> While we're here, the C language FAQ adds:
> 
> Q: Is it legal to pass a null pointer as the first argument to
>     realloc? Why would you want to?
> 
> A: ANSI C sanctions this usage (and the related realloc(..., 0), which
>    frees), although several earlier implementations do not support it, so
>    it may not be fully portable.

Old implementations, I guess. Is there any implementation we care about
that does that?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Jamie Lokier]

Eduardo Habkost wrote:
> I really hope you are not proposing to make qemu_realloc(p, 0) work but
> qemu_malloc(0) fail, because you would be breaking lots of
> realloc()/malloc() equivalency assumptions.

Careful.

realloc(p, 0) *always* frees p (or does nothing if p == NULL), on all
systems complying with ISO C, and always returns NULL.

malloc(0) on the other hand does not guarantee this,
so malloc(0) is *not* a substitute for realloc(NULL, 0).

-- Jamie

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

> On Tue, 19 May 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
[...]
> Once again, on Linux/GLIBC it will, on AIX it wont.
>
> And FWIW despite behaviour of malloc(0) being marked as implementation
> defined i have sa far was unable to find any documentaiton (Linux man
> pages, GLIBC info files) witht the actual definition, unlike on AIX
> where man pages make it crystal clear what happens.

malloc(3) says:

       If size is 0, then malloc() returns either NULL, or a unique
       pointer value that can later be successfully passed to free().

Matches ISO C.  It's all you need to know to write portable programs.

[...]
>> >> >                                              IOW making qemu_malloc[z]
>> >> > return whatever the underlying system returns is just hiding the bugs,
>> >> > the code becomes unportable.
>> >> 
>> >> Matter of taste.
>> >> 
>> >> 1. Deal with the implementation-definedness.  Every caller that could
>> >>    pass zero needs to take care not to confuse empty allocation with an
>> >>    out of memory condition.
>> >> 
>> >>    This is easier than it sounds when you check for out of memory in
>> >>    just one place, like we do.
>> >> 
>> >> 2. Remove the implementation-definedness.  Easiest way is to detect zero
>> >>    size in a wrapper (for us: qemu_malloc()) and bump it to one.
>> >
>> > And mine:
>> >   3. Abort the program if somebody tries it. Because so far history thought
>> >      me that nobody does 1.
>> 
>> Tries what?  Passing zero to qemu_malloc()?  That's legitimate.  And
>> with allocation functions that cannot return failure, it's hardly
>> dangerous, isn't it?
>
> That's legitimate only if one writes unportable code targeting single
> system and knowing how it was defined. As for being dangerous, yes it
> is: dereferencing the returned pointer, while UB, doesn't trigger a
> SEGFAULT on, at least, this machine with Linux.

malloc(0) is perfectly portable.  The fact that dereferencing the return
value may or may not fault doesn't change that.  What happens on
dereferencing of an invalid pointer is never portable.

>> >> qemu_realloc() currently uses 1.
>
> void *qemu_realloc(void *ptr, size_t size)
> {
>     if (size)
>         return oom_check(realloc(ptr, size));
>     else
>         return realloc(ptr, size);
> }
>  
> There is nothing implementation defined about realloc(whatever, 0), it
> has a defined meaning in POSIX:
> http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
>
> So it doesn't use 1.

Quoting from there:

    If ptr is a null pointer, realloc() behaves like malloc() for the
    specified size.

So, realloc(whatever, 0) uses 1. when malloc(0) does.

>> >> realloc(NULL, sz) is specified to be equivalent to malloc(sz).  It would
>> >> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
>> >> 
>> >
>> > qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The part
>> > about qemu_malloc escapes me.
>> 
>> qemu_malloc() & friends never fail.  Checking their value for failure is
>> pointless.  Therefore, 1. is practical.
>> 
>> 2. is certainly practical as well.
>> 
>> 3. is like 2, with the (size ? size : 1) pushed into callers.  I find
>> that mildly annoying.
>
> Huh, that's not at all what i proposed. What i had in mind is:
>
> void *qemu_malloc(size_t size)
> {
>     if (!size) abort();
>     return oom_check(malloc(size));
> }

Exactly.  Callers who used the fact that malloc(size) is perfectly
portable for size>=0 then have to use qemu_malloc(size ? size : 1).

>> I don't care whether we pick 1., 2. or 3., but I'd prefer we pick the
>> same for qemu_malloc() and qemu_realloc().  We currently use 1. for
>> qemu_realloc().  My point is: if you pick something else for
>> qemu_malloc(), please consider changing qemu_realloc().  That's all.
>
> I see.

Good.

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 05:00:27PM +0400, malc wrote:
> On Tue, 19 May 2009, Markus Armbruster wrote:
<snip>
> > >                                              IOW making qemu_malloc[z]
> > > return whatever the underlying system returns is just hiding the bugs,
> > > the code becomes unportable.
> > 
> > Matter of taste.
> > 
> > 1. Deal with the implementation-definedness.  Every caller that could
> >    pass zero needs to take care not to confuse empty allocation with an
> >    out of memory condition.
> > 
> >    This is easier than it sounds when you check for out of memory in
> >    just one place, like we do.
> > 
> > 2. Remove the implementation-definedness.  Easiest way is to detect zero
> >    size in a wrapper (for us: qemu_malloc()) and bump it to one.
> 
> And mine:
>   3. Abort the program if somebody tries it. Because so far history thought
>      me that nobody does 1.

Are you sure about that? There may be cases where qemu_malloc(0) is
called correctly, without the wrong assumptions about the returned
value.

You are proposing to make the qemu_malloc() API behavior diverge from
the standard C malloc() behavior and prevent usage that is valid for
malloc()/free() usage. Do you volunteer to audit all Qemu code to make
sure the new behavior is safe?  ;)

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Tue, 19 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 05:00:27PM +0400, malc wrote:
> > On Tue, 19 May 2009, Markus Armbruster wrote:
> <snip>
> > > >                                              IOW making qemu_malloc[z]
> > > > return whatever the underlying system returns is just hiding the bugs,
> > > > the code becomes unportable.
> > > 
> > > Matter of taste.
> > > 
> > > 1. Deal with the implementation-definedness.  Every caller that could
> > >    pass zero needs to take care not to confuse empty allocation with an
> > >    out of memory condition.
> > > 
> > >    This is easier than it sounds when you check for out of memory in
> > >    just one place, like we do.
> > > 
> > > 2. Remove the implementation-definedness.  Easiest way is to detect zero
> > >    size in a wrapper (for us: qemu_malloc()) and bump it to one.
> > 
> > And mine:
> >   3. Abort the program if somebody tries it. Because so far history thought
> >      me that nobody does 1.
> 
> Are you sure about that? There may be cases where qemu_malloc(0) is
> called correctly, without the wrong assumptions about the returned
> value.
> 
> You are proposing to make the qemu_malloc() API behavior diverge from
> the standard C malloc() behavior and prevent usage that is valid for
> malloc()/free() usage. Do you volunteer to audit all Qemu code to make
> sure the new behavior is safe?  ;)

That's the problem standard C does _not_ define the behaviour, and leaves
that to implementation. As for audit, that's precisely what aborting on
zero will try (and fail) to accomplish the offenders will be (given
unlimited time) caught. oom_check was added despite the fact that there
were places that correctly handled malloc's returning NULL. And i for
one do not know if there are/were places that called qemu_malloc with
zero and expected Linux behaviour.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 06:37:18PM +0400, malc wrote:
> On Tue, 19 May 2009, Eduardo Habkost wrote:
> 
> > On Tue, May 19, 2009 at 05:00:27PM +0400, malc wrote:
> > > On Tue, 19 May 2009, Markus Armbruster wrote:
> > <snip>
> > > > >                                              IOW making qemu_malloc[z]
> > > > > return whatever the underlying system returns is just hiding the bugs,
> > > > > the code becomes unportable.
> > > > 
> > > > Matter of taste.
> > > > 
> > > > 1. Deal with the implementation-definedness.  Every caller that could
> > > >    pass zero needs to take care not to confuse empty allocation with an
> > > >    out of memory condition.
> > > > 
> > > >    This is easier than it sounds when you check for out of memory in
> > > >    just one place, like we do.
> > > > 
> > > > 2. Remove the implementation-definedness.  Easiest way is to detect zero
> > > >    size in a wrapper (for us: qemu_malloc()) and bump it to one.
> > > 
> > > And mine:
> > >   3. Abort the program if somebody tries it. Because so far history thought
> > >      me that nobody does 1.
> > 
> > Are you sure about that? There may be cases where qemu_malloc(0) is
> > called correctly, without the wrong assumptions about the returned
> > value.
> > 
> > You are proposing to make the qemu_malloc() API behavior diverge from
> > the standard C malloc() behavior and prevent usage that is valid for
> > malloc()/free() usage. Do you volunteer to audit all Qemu code to make
> > sure the new behavior is safe?  ;)
> 
> That's the problem standard C does _not_ define the behaviour, and leaves
> that to implementation.

The only thing it doesn't define is either the returned pointer is NULL
or not, and that doesn't make malloc(0) automatically unportable,
because all the rest is perfectly defined:

1) You can't dereference the pointer (just like you can't
   dereference p[n] on a malloc(n) block)
2) You should pass the returned pointer to free() later


> As for audit, that's precisely what aborting on
> zero will try (and fail) to accomplish the offenders will be (given
> unlimited time) caught.

My point is that malloc(0) is not a bug, and I don't see a reason to
make it an offense and diverge from standard malloc() and free().

> oom_check was added despite the fact that there
> were places that correctly handled malloc's returning NULL. And i for
> one do not know if there are/were places that called qemu_malloc with
> zero and expected Linux behaviour.

I agree that expecting the Linux behaviour (non-NULL) is a bug. My point
is that there is no reason to consider malloc(0) a bug.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Tue, 19 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 06:37:18PM +0400, malc wrote:
> > On Tue, 19 May 2009, Eduardo Habkost wrote:
> > 
> > > On Tue, May 19, 2009 at 05:00:27PM +0400, malc wrote:
> > > > On Tue, 19 May 2009, Markus Armbruster wrote:
> > > <snip>
> > > > > >                                              IOW making qemu_malloc[z]
> > > > > > return whatever the underlying system returns is just hiding the bugs,
> > > > > > the code becomes unportable.
> > > > > 
> > > > > Matter of taste.
> > > > > 
> > > > > 1. Deal with the implementation-definedness.  Every caller that could
> > > > >    pass zero needs to take care not to confuse empty allocation with an
> > > > >    out of memory condition.
> > > > > 
> > > > >    This is easier than it sounds when you check for out of memory in
> > > > >    just one place, like we do.
> > > > > 
> > > > > 2. Remove the implementation-definedness.  Easiest way is to detect zero
> > > > >    size in a wrapper (for us: qemu_malloc()) and bump it to one.
> > > > 
> > > > And mine:
> > > >   3. Abort the program if somebody tries it. Because so far history thought
> > > >      me that nobody does 1.
> > > 
> > > Are you sure about that? There may be cases where qemu_malloc(0) is
> > > called correctly, without the wrong assumptions about the returned
> > > value.
> > > 
> > > You are proposing to make the qemu_malloc() API behavior diverge from
> > > the standard C malloc() behavior and prevent usage that is valid for
> > > malloc()/free() usage. Do you volunteer to audit all Qemu code to make
> > > sure the new behavior is safe?  ;)
> > 
> > That's the problem standard C does _not_ define the behaviour, and leaves
> > that to implementation.
> 
> The only thing it doesn't define is either the returned pointer is NULL
> or not, and that doesn't make malloc(0) automatically unportable,
> because all the rest is perfectly defined:
> 
> 1) You can't dereference the pointer (just like you can't
>    dereference p[n] on a malloc(n) block)
> 2) You should pass the returned pointer to free() later
> 

Alas your list is not exhaustive:

  3) Test the returned value against NULL

[Which is precisely what the qcow2 code did]

And

  4) Accidentally derefence it

And here one would get away with it on certain subset of systems.

> 
> > As for audit, that's precisely what aborting on
> > zero will try (and fail) to accomplish the offenders will be (given
> > unlimited time) caught.
> 
> My point is that malloc(0) is not a bug, and I don't see a reason to
> make it an offense and diverge from standard malloc() and free().
> 
> > oom_check was added despite the fact that there
> > were places that correctly handled malloc's returning NULL. And i for
> > one do not know if there are/were places that called qemu_malloc with
> > zero and expected Linux behaviour.
> 
> I agree that expecting the Linux behaviour (non-NULL) is a bug. My point
> is that there is no reason to consider malloc(0) a bug.

There is, due to the possibility of performing a 3) and a hard time 
catching that (unless someone solves halting problem or subset applicable
to QEMU thereof)

-- 
mailto:av1474@comtv.ru

[PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0 (was Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0)

[Eduardo Habkost]

On Tue, May 19, 2009 at 06:55:11PM +0400, malc wrote:
<snip>
> > > 
> > > That's the problem standard C does _not_ define the behaviour, and leaves
> > > that to implementation.
> > 
> > The only thing it doesn't define is either the returned pointer is NULL
> > or not, and that doesn't make malloc(0) automatically unportable,
> > because all the rest is perfectly defined:
> > 
> > 1) You can't dereference the pointer (just like you can't
> >    dereference p[n] on a malloc(n) block)
> > 2) You should pass the returned pointer to free() later
> > 
> 
> Alas your list is not exhaustive:
> 
>   3) Test the returned value against NULL
> 
> [Which is precisely what the qcow2 code did]
> 
[...]
> > 
> > I agree that expecting the Linux behaviour (non-NULL) is a bug. My point
> > is that there is no reason to consider malloc(0) a bug.
> 
> There is, due to the possibility of performing a 3) and a hard time 
> catching that (unless someone solves halting problem or subset applicable
> to QEMU thereof)

This is probably the only of your points which I agree with. What about
the following, then?

That would catch the cases you are worried about, but won't break
existing cases where malloc(0) is used correctly, and we won't be
creating a new malloc/free API that is incompabible from every other
malloc/free API out there.

-----------
>From c2e0dd58e3eb7e2fd749bcf4999f067b1ba19083 Mon Sep 17 00:00:00 2001
From: Eduardo Habkost <ehabkost@redhat.com>
Date: Tue, 19 May 2009 13:24:47 -0300
Subject: [PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0

This is an alternative to the fix I've proposed at:

Message-Id: <1242678676-19439-1-git-send-email-ehabkost@redhat.com>
http://marc.info/?l=qemu-devel&m=124267871605176

Instead of returning whatever realloc(p, 0) or malloc(0) of the system we are
running on returns, always return NULL on those cases.

This should make it easier to catch cases where code is incorrectly checking
the qemu_malloc() or qemu_realloc() return value for NULL when when size==0.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-malloc.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/qemu-malloc.c b/qemu-malloc.c
index 6761857..5346ece 100644
--- a/qemu-malloc.c
+++ b/qemu-malloc.c
@@ -43,15 +43,20 @@ void qemu_free(void *ptr)
 
 void *qemu_malloc(size_t size)
 {
+    if (!size)
+        return NULL;
+
     return oom_check(malloc(size));
 }
 
 void *qemu_realloc(void *ptr, size_t size)
 {
-    if (size)
-        return oom_check(realloc(ptr, size));
-    else
-        return realloc(ptr, size);
+    if (!size) {
+        free(ptr);
+        return NULL;
+    }
+
+    return oom_check(realloc(ptr, size));
 }
 
 void *qemu_mallocz(size_t size)
-- 
1.6.3.rc4.29.g8146


-- 
Eduardo

Re: [PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0 (was Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0)

[malc]

On Tue, 19 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 06:55:11PM +0400, malc wrote:
> <snip>
> > > > 
> > > > That's the problem standard C does _not_ define the behaviour, and leaves
> > > > that to implementation.
> > > 
> > > The only thing it doesn't define is either the returned pointer is NULL
> > > or not, and that doesn't make malloc(0) automatically unportable,
> > > because all the rest is perfectly defined:
> > > 
> > > 1) You can't dereference the pointer (just like you can't
> > >    dereference p[n] on a malloc(n) block)
> > > 2) You should pass the returned pointer to free() later
> > > 
> > 
> > Alas your list is not exhaustive:
> > 
> >   3) Test the returned value against NULL
> > 
> > [Which is precisely what the qcow2 code did]
> > 
> [...]
> > > 
> > > I agree that expecting the Linux behaviour (non-NULL) is a bug. My point
> > > is that there is no reason to consider malloc(0) a bug.
> > 
> > There is, due to the possibility of performing a 3) and a hard time 
> > catching that (unless someone solves halting problem or subset applicable
> > to QEMU thereof)
> 
> This is probably the only of your points which I agree with. What about
> the following, then?
> 
> That would catch the cases you are worried about, but won't break
> existing cases where malloc(0) is used correctly, and we won't be
> creating a new malloc/free API that is incompabible from every other
> malloc/free API out there.

Thanks for an attempt, but i don't like it either, since it sortof
breaks the (unspoken?) qemu_malloc/realloc contract that those will
never return NULL. I've commited the thing i had in mind.

[..snip..]

P.S. It probably says something about my experience in the field, but the
     only time i've encountered real-life usage of malloc(0) is in
     that qcow2 case, the code was produced by a programmer who among
     (numerous) other things wrote full C compiler and won IOCCC
     twice, yet even his code failed to work.

-- 
mailto:av1474@comtv.ru

Re: [PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0 (was Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0)

[Eduardo Habkost]

On Tue, May 19, 2009 at 10:40:08PM +0400, malc wrote:
> On Tue, 19 May 2009, Eduardo Habkost wrote:
> 
> > On Tue, May 19, 2009 at 06:55:11PM +0400, malc wrote:
> > <snip>
> > > > > 
> > > > > That's the problem standard C does _not_ define the behaviour, and leaves
> > > > > that to implementation.
> > > > 
> > > > The only thing it doesn't define is either the returned pointer is NULL
> > > > or not, and that doesn't make malloc(0) automatically unportable,
> > > > because all the rest is perfectly defined:
> > > > 
> > > > 1) You can't dereference the pointer (just like you can't
> > > >    dereference p[n] on a malloc(n) block)
> > > > 2) You should pass the returned pointer to free() later
> > > > 
> > > 
> > > Alas your list is not exhaustive:
> > > 
> > >   3) Test the returned value against NULL
> > > 
> > > [Which is precisely what the qcow2 code did]
> > > 
> > [...]
> > > > 
> > > > I agree that expecting the Linux behaviour (non-NULL) is a bug. My point
> > > > is that there is no reason to consider malloc(0) a bug.
> > > 
> > > There is, due to the possibility of performing a 3) and a hard time 
> > > catching that (unless someone solves halting problem or subset applicable
> > > to QEMU thereof)
> > 
> > This is probably the only of your points which I agree with. What about
> > the following, then?
> > 
> > That would catch the cases you are worried about, but won't break
> > existing cases where malloc(0) is used correctly, and we won't be
> > creating a new malloc/free API that is incompabible from every other
> > malloc/free API out there.
> 
> Thanks for an attempt, but i don't like it either, since it sortof
> breaks the (unspoken?) qemu_malloc/realloc contract that those will
> never return NULL. I've commited the thing i had in mind.

Asking for feedback before committing wouldn't hurt.

Now:

- Every caller that could pass 0 to qemu_malloc() have to make it an
  special case.
- Every caller that uses qemu_realloc() will have to add special cases,
  if size==0 is possible.
- We are not sure where those callers are.
- Qemu's API is incompatible with every other malloc/free API out there,
  but is not documented

-- 
Eduardo

Re: [PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0 (was Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0)

[Jamie Lokier]

malc wrote:
> Thanks for an attempt, but i don't like it either, since it sortof
> breaks the (unspoken?) qemu_malloc/realloc contract that those will
> never return NULL. I've commited the thing i had in mind.

realloc returns NULL if the size is zero, and frees the object if the
pointer is non-NULL.

-- Jamie

Re: [PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0 (was Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0)

[Kevin Wolf]

malc schrieb:
> Thanks for an attempt, but i don't like it either, since it sortof
> breaks the (unspoken?) qemu_malloc/realloc contract that those will
> never return NULL. I've commited the thing i had in mind.

I don't know of such a contract. If you want to have it, you should add
a comment which specifies what is guaranteed and what not. And best post
the patch to the list first this time.

Btw, even with your patch realloc(ptr, 0) with ptr != NULL still can
return NULL. And if you now go ahead and forbid size = 0 entirely, I'm
almost sure we have a regression as I didn't do the original patch to
allow this just for fun but because it's used...

Kevin

[Qemu-devel] Re: [PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0

[Markus Armbruster]

malc <av1474@comtv.ru> writes:

[Debate snipped...]
> never return NULL. I've commited the thing i had in mind.

Arguing this case has been a waste of time, for which I apologize.  I'll
try not to do that again.

> [..snip..]
>
> P.S. It probably says something about my experience in the field, but the
>      only time i've encountered real-life usage of malloc(0) is in
>      that qcow2 case, the code was produced by a programmer who among
>      (numerous) other things wrote full C compiler and won IOCCC
>      twice, yet even his code failed to work.

Read more code then, and more varied code.

[Qemu-devel] Re: [PATCH] Make qemu_alloc()/qemu_realloc() return NULL for size==0

[malc]

On Wed, 20 May 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> [Debate snipped...]
> > never return NULL. I've commited the thing i had in mind.
> 
> Arguing this case has been a waste of time, for which I apologize.  I'll
> try not to do that again.
> 
> > [..snip..]
> >
> > P.S. It probably says something about my experience in the field, but the
> >      only time i've encountered real-life usage of malloc(0) is in
> >      that qcow2 case, the code was produced by a programmer who among
> >      (numerous) other things wrote full C compiler and won IOCCC
> >      twice, yet even his code failed to work.
> 
> Read more code then, and more varied code.

Any suggested reading? 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Jamie Lokier]

malc wrote:
>   3) Test the returned value against NULL

Indeed, that makes malloc(0) a bad idea, and why GNU/Linux sensibly
makes it return a non-NULL pointer, to minimise unexpected behaviour
of programs.

>   4) Accidentally derefence it
> 
> And here one would get away with it on certain subset of systems.

That's not in the list of reliable things, though.

You mustn't accidentally dereference p[n] after calling p = malloc(n) either.

-- Jamie

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[Eduardo Habkost]

On Tue, May 19, 2009 at 04:17:55AM +0400, malc wrote:
> On Mon, 18 May 2009, Eduardo Habkost wrote:
> 
> > On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
> > > On Mon, 18 May 2009, Eduardo Habkost wrote:
> > > 
> > > > This patch is similar to a previous qemu_realloc() fix
> > > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
> > > > 
> > > > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
> > > > this case.
> > > 
> > > Only it wouldn't (on Linux):
> > > 
> > > $ cat malloc.c
> > > #include <stdlib.h>
> > > 
> > > int main (void)
> > > {
> > >     printf ("%p\n", malloc (0));
> > >     return 0;
> > > }
> > > $ gcc malloc.c
> > > $ ./a.out 
> > > 0x10011008
> > > 
> > > Standard (in 7.20.3) says that malloc's behaviour in case of size being
> > > zero is implementation defined.
> > > 
> > > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
> > > 
> > > The best(only?) thing to do is to check size passed to qemu_malloc[z]
> > > and abort the program if this situation is encountered.
> > 
> > Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
> > or a pointer, and on any case the value can be safely passed to free()
> > later.
> 
> I believe you haven't examined the commit that i referenced. Thing is
> existing code used to, i'd venture a guess accidentaly, rely on the
> behaviour that current GLIBC provides and consequently failed to
> operate on AIX where malloc(0) returns NULL, IOW making qemu_malloc[z]
> return whatever the underlying system returns is just hiding the bugs,
> the code becomes unportable.

The assumption that malloc(0) will return anything (either NULL or
not-NULL) is not portable. That's exactly the point of my patch: not
making any assumption about the returned value when size==0.

But calling malloc(0) is perfectly valid, as long as you call free() on
the returned value later. I don't see any reason to make the
qemu_malloc() behavior from the standard malloc() behavior. The sequence
"p=malloc(0);free(p)" is valid and works. Why would we prevent
"p=qemu_malloc(0);qemu_free(p)" from working?

Yes, we may have broken code that assumes that qemu_malloc(0) is not
NULL, and _that_ code is broken and must be fixed. But why would we
break the cases where qemu_malloc(0) is called and handled correctly?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0

[malc]

On Tue, 19 May 2009, Eduardo Habkost wrote:

> On Tue, May 19, 2009 at 04:17:55AM +0400, malc wrote:
> > On Mon, 18 May 2009, Eduardo Habkost wrote:
> > 
> > > On Tue, May 19, 2009 at 01:56:55AM +0400, malc wrote:
> > > > On Mon, 18 May 2009, Eduardo Habkost wrote:
> > > > 
> > > > > This patch is similar to a previous qemu_realloc() fix
> > > > > (commit 322691a5c9f1c8531554148d47c078b5be590805), but for qemu_malloc().
> > > > > 
> > > > > malloc(0) may correctly return NULL if size==0. We don't want to abort qemu on
> > > > > this case.
> > > > 
> > > > Only it wouldn't (on Linux):
> > > > 
> > > > $ cat malloc.c
> > > > #include <stdlib.h>
> > > > 
> > > > int main (void)
> > > > {
> > > >     printf ("%p\n", malloc (0));
> > > >     return 0;
> > > > }
> > > > $ gcc malloc.c
> > > > $ ./a.out 
> > > > 0x10011008
> > > > 
> > > > Standard (in 7.20.3) says that malloc's behaviour in case of size being
> > > > zero is implementation defined.
> > > > 
> > > > Try `git show 63c75dcd669d011f438421980b4379827da4bb1c'.
> > > > 
> > > > The best(only?) thing to do is to check size passed to qemu_malloc[z]
> > > > and abort the program if this situation is encountered.
> > > 
> > > Why? malloc(0) is as valid as realloc(p, 0). It will either return NULL
> > > or a pointer, and on any case the value can be safely passed to free()
> > > later.
> > 
> > I believe you haven't examined the commit that i referenced. Thing is
> > existing code used to, i'd venture a guess accidentaly, rely on the
> > behaviour that current GLIBC provides and consequently failed to
> > operate on AIX where malloc(0) returns NULL, IOW making qemu_malloc[z]
> > return whatever the underlying system returns is just hiding the bugs,
> > the code becomes unportable.
> 
> The assumption that malloc(0) will return anything (either NULL or
> not-NULL) is not portable. That's exactly the point of my patch: not
> making any assumption about the returned value when size==0.

Yes i got that, i just disagree with the outcome of the test.

> 
> But calling malloc(0) is perfectly valid, as long as you call free() on
> the returned value later. I don't see any reason to make the
> qemu_malloc() behavior from the standard malloc() behavior. The sequence
> "p=malloc(0);free(p)" is valid and works. Why would we prevent
> "p=qemu_malloc(0);qemu_free(p)" from working?
> 
> Yes, we may have broken code that assumes that qemu_malloc(0) is not
> NULL, and _that_ code is broken and must be fixed. But why would we
> break the cases where qemu_malloc(0) is called and handled correctly?

To reiterate, there was code that handled qemu_malloc(non_zero) failures
gracefuly yet the oom_check was introduced. Different tradeoffs i guess.

-- 
mailto:av1474@comtv.ru