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

[Gonglei (Arei)]

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 <address@hidden>
> ---
>  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.

Best regards,
-Gonglei