rbd: avoid qemu_rbd_snap_list() memory leak when no snapshots

When there are no snapshots qemu_rbd_snap_list() returns 0 and the
snapshot table pointer is NULL.  Don't forget to free the snaps buffer
we allocated for librbd rbd_snap_list().

Cc: qemu-stable@nongnu.org
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/rbd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoting Stefan Hajnoczi (2013-08-14 07:13:52)
> When there are no snapshots qemu_rbd_snap_list() returns 0 and the
> snapshot table pointer is NULL.  Don't forget to free the snaps buffer
> we allocated for librbd rbd_snap_list().
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Ping for 1.6.1

> ---
>  block/rbd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index cb71751..4e26fea 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -934,7 +934,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
>      do {
>          snaps = g_malloc(sizeof(*snaps) * max_snaps);
>          snap_count = rbd_snap_list(s->image, snaps, &max_snaps);
> -        if (snap_count < 0) {
> +        if (snap_count <= 0) {
>              g_free(snaps);
>          }
>      } while (snap_count == -ERANGE);
> -- 
> 1.8.3.1

Am 25.09.2013 um 02:28 hat Michael Roth geschrieben:
> Quoting Stefan Hajnoczi (2013-08-14 07:13:52)
> > When there are no snapshots qemu_rbd_snap_list() returns 0 and the
> > snapshot table pointer is NULL.  Don't forget to free the snaps buffer
> > we allocated for librbd rbd_snap_list().
> > 
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Ping for 1.6.1

Applied it to the block branch for now, but...

> > ---
> >  block/rbd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/block/rbd.c b/block/rbd.c
> > index cb71751..4e26fea 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -934,7 +934,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
> >      do {
> >          snaps = g_malloc(sizeof(*snaps) * max_snaps);
> >          snap_count = rbd_snap_list(s->image, snaps, &max_snaps);
> > -        if (snap_count < 0) {
> > +        if (snap_count <= 0) {
> >              g_free(snaps);
> >          }
> >      } while (snap_count == -ERANGE);

...I think this isn't a complete fix. In the successful case we still
leak snaps. The g_free() should probably be moved to after the done:
label in a v2 of the patch.

Kevin

On Wed, Sep 25, 2013 at 10:06:11AM +0200, Kevin Wolf wrote:
> Am 25.09.2013 um 02:28 hat Michael Roth geschrieben:
> > Quoting Stefan Hajnoczi (2013-08-14 07:13:52)
> > > When there are no snapshots qemu_rbd_snap_list() returns 0 and the
> > > snapshot table pointer is NULL.  Don't forget to free the snaps buffer
> > > we allocated for librbd rbd_snap_list().
> > > 
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > Ping for 1.6.1
> 
> Applied it to the block branch for now, but...
> 
> > > ---
> > >  block/rbd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/block/rbd.c b/block/rbd.c
> > > index cb71751..4e26fea 100644
> > > --- a/block/rbd.c
> > > +++ b/block/rbd.c
> > > @@ -934,7 +934,7 @@ static int qemu_rbd_snap_list(BlockDriverState *bs,
> > >      do {
> > >          snaps = g_malloc(sizeof(*snaps) * max_snaps);
> > >          snap_count = rbd_snap_list(s->image, snaps, &max_snaps);
> > > -        if (snap_count < 0) {
> > > +        if (snap_count <= 0) {
> > >              g_free(snaps);
> > >          }
> > >      } while (snap_count == -ERANGE);
> 
> ...I think this isn't a complete fix. In the successful case we still
> leak snaps. The g_free() should probably be moved to after the done:
> label in a v2 of the patch.

You are right.  I'm sending a v2.  rbd_snap_list_end() does not free
snaps itself, only the strings that snaps[i].name points to.  Therefore
we need to free snaps.