Re: [Qemu-devel] [patch] qcow2: double free snapshots

["Zhang Haoyu" <zhanghy@sangfor.com>]

>Hi,
>
>> Subject: [Qemu-devel] [patch] qcow2: double free snapshots
>> 
>> In qcow2_open(), if qcow2_read_snapshots() failed, qcow2_open() ->
>> qcow2_free_snapshots() will be called, NULL snapshots dereference happened.
>> because qcow2_free_snapshots has been performed before in the fail case of
>> qcow2_read_snapshots().
>> shown as below callstack,
>> qcow2_open
>> |- qcow2_read_snapshots
>> |-- goto fail;
>> |-- qcow2_free_snapshots
>> |- goto fail;
>> |- qcow2_free_snapshots  /* on this case, NULL snapshots dereference
>> happened.  */
>> 
>> Signed-off-by: Zhang Haoyu <zhanghy@sangfor.com>
>> ---
>>  block/qcow2-snapshot.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 0aa9def..15c0513 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -31,6 +31,10 @@ void qcow2_free_snapshots(BlockDriverState *bs)
>>      BDRVQcowState *s = bs->opaque;
>>      int i;
>> 
>> +    if (!s || !s->snapshots) {
>> +        return;
>> +    }
>> +
>>      for(i = 0; i < s->nb_snapshots; i++) {
>>          g_free(s->snapshots[i].name);
>>          g_free(s->snapshots[i].id_str);
>> --
>> 1.9.4.msysgit.0
>> 
>Yes, the code path is right. But I don't think this check is necessary.
>The first free has set "s->nb_snapshots = 0", so it is impossibility go into
>g_free(s->snapshots[i].name), and g_free(s->snapshots) is OK, though it is NULL.
>
Yes, you are right, ignore this patch, please.

Thanks,
Zhang Haoyu
>Best regards,
>-Gonglei