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

[Markus Armbruster]

malc <address@hidden> writes:

> On Tue, 19 May 2009, Markus Armbruster wrote:
>
>> malc <address@hidden> 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.