| Message ID | 20210510125340.903323-4-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,1/3] xen-mapcache: avoid a race on memory map while using MAP_FIXED | expand |
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
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
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(); - 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; + } + 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);
... 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> --- hw/block/xen-block.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)