Re: [PULL 3/3] xen-block: Use specific blockdev driver

[Peter Maydell]

On Fri, 2 Jun 2023 at 18:04, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 10 May 2021 at 13:53, Anthony PERARD <anthony.perard@citrix.com> 
> wrote:
> >
> > ... when a xen-block backend instance is created via xenstore.
> >
> > Following 8d17adf34f50 ("block: remove support for using "file" driver
> > with block/char devices"), using the "file" blockdev driver for
> > everything doesn't work anymore, we need to use the "host_device"
> > driver when the disk image is a block device and "file" driver when it
> > is a regular file.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > Acked-by: Paul Durrant <paul@xen.org>
> > Message-Id: <20210430163432.468894-1-anthony.perard@citrix.com>
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>
> Hi; Coverity points out (CID 1508722) that this introduces a
> memory leak in the new error codepath:

I just realized I forgot to cc the current Xen maintainers,
so I'm doing that now. I think the fix for this leak should
be fairly straightforward.

> > ---
> >  hw/block/xen-block.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> > index 83754a4344..674953f1ad 100644
> > --- a/hw/block/xen-block.c
> > +++ b/hw/block/xen-block.c
> > @@ -728,6 +728,8 @@ static XenBlockDrive *xen_block_drive_create(const char 
> > *id,
> >      XenBlockDrive *drive = NULL;
> >      QDict *file_layer;
> >      QDict *driver_layer;
> > +    struct stat st;
> > +    int rc;
> >
> >      if (params) {
> >          char **v = g_strsplit(params, ":", 2);
> > @@ -761,7 +763,17 @@ static XenBlockDrive *xen_block_drive_create(const 
> > char *id,
> >      file_layer = qdict_new();
> >      driver_layer = qdict_new();
>
> You can see here that we allocate file_layer and driver_layer
> as new qdict objects...
>
> >
> > -    qdict_put_str(file_layer, "driver", "file");
> > +    rc = stat(filename, &st);
> > +    if (rc) {
> > +        error_setg_errno(errp, errno, "Could not stat file '%s'", 
> > filename);
> > +        goto done;
>
> ...but here if the stat() fails we will bail out to
> the 'done' label, and the code there does not dereference
> these qdicts, so they will leak.
>
> The easy fix is to move the two calls to qdict_new() to
> below this if() rather than above it.
>
> > +    }
> > +    if (S_ISBLK(st.st_mode)) {
> > +        qdict_put_str(file_layer, "driver", "host_device");
> > +    } else {
> > +        qdict_put_str(file_layer, "driver", "file");
> > +    }
> > +
> >      qdict_put_str(file_layer, "filename", filename);
> >      g_free(filename);

thanks
-- PMM