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

[Peter Maydell <peter.maydell@linaro.org>]

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:

> ---
>  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