Re: [Qemu-devel] [PATCH] xen-block: stop leaking memory in xen_block_drive_create()

Re: [Qemu-devel] [PATCH] xen-block: stop leaking memory in xen_block_drive_create()

[Paul Durrant]

Apologies... typo-ed qemu-devel...

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 19 February 2019 16:35
> To: qeme-devel@nongnu.org; qemu-block@nongnu.org; xen-
> devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Peter Maydell
> <peter.maydell@linaro.org>; Stefano Stabellini <sstabellini@kernel.org>;
> Anthony Perard <anthony.perard@citrix.com>; Kevin Wolf <kwolf@redhat.com>;
> Max Reitz <mreitz@redhat.com>
> Subject: [PATCH] xen-block: stop leaking memory in
> xen_block_drive_create()
> 
> The locally allocated QDict-s need to be freed. ('file_layer' will be
> freed implicitly since it is added as an object to 'driver_layer').
> 
> Spotted by Coverity: CID 1398649
> 
> While in the neighbourhood free 'driver' and 'filename' as soon as they
> are
> added to the QDicts. Freeing after the 'done' label doesn't make that much
> sense as, if the error path jumps to that label, the values would be NULL
> anyway.
> 
> This patch also makes that more obvious by taking the error path if
> 'params' is NULL and then asserting that both driver and filename are
> non-NULL in the normal path.
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  hw/block/xen-block.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 37a456c207..70fc2455e8 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -743,12 +743,12 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>          }
> 
>          g_strfreev(v);
> -    }
> -
> -    if (!filename) {
> -        error_setg(errp, "no filename");
> +    } else {
> +        error_setg(errp, "no params");
>          goto done;
>      }
> +
> +    assert(filename);
>      assert(driver);
> 
>      drive = g_new0(XenBlockDrive, 1);
> @@ -758,6 +758,7 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
> 
>      qdict_put_str(file_layer, "driver", "file");
>      qdict_put_str(file_layer, "filename", filename);
> +    g_free(filename);
> 
>      if (mode && *mode != 'w') {
>          qdict_put_bool(file_layer, "read-only", true);
> @@ -793,16 +794,17 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>      driver_layer = qdict_new();
> 
>      qdict_put_str(driver_layer, "driver", driver);
> +    g_free(driver);
> +
>      qdict_put_obj(driver_layer, "file", QOBJECT(file_layer));
> 
>      g_assert(!drive->node_name);
>      drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
>                                                &local_err);
> 
> -done:
> -    g_free(driver);
> -    g_free(filename);
> +    qobject_unref(driver_layer);
> 
> +done:
>      if (local_err) {
>          error_propagate(errp, local_err);
>          xen_block_drive_destroy(drive, NULL);
> --
> 2.20.1.2.gb21ebb6

Re: [PATCH] xen-block: stop leaking memory in xen_block_drive_create()

[Paul Durrant]

Apologies... typo-ed qemu-devel...

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 19 February 2019 16:35
> To: qeme-devel@nongnu.org; qemu-block@nongnu.org; xen-
> devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Peter Maydell
> <peter.maydell@linaro.org>; Stefano Stabellini <sstabellini@kernel.org>;
> Anthony Perard <anthony.perard@citrix.com>; Kevin Wolf <kwolf@redhat.com>;
> Max Reitz <mreitz@redhat.com>
> Subject: [PATCH] xen-block: stop leaking memory in
> xen_block_drive_create()
> 
> The locally allocated QDict-s need to be freed. ('file_layer' will be
> freed implicitly since it is added as an object to 'driver_layer').
> 
> Spotted by Coverity: CID 1398649
> 
> While in the neighbourhood free 'driver' and 'filename' as soon as they
> are
> added to the QDicts. Freeing after the 'done' label doesn't make that much
> sense as, if the error path jumps to that label, the values would be NULL
> anyway.
> 
> This patch also makes that more obvious by taking the error path if
> 'params' is NULL and then asserting that both driver and filename are
> non-NULL in the normal path.
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  hw/block/xen-block.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 37a456c207..70fc2455e8 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -743,12 +743,12 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>          }
> 
>          g_strfreev(v);
> -    }
> -
> -    if (!filename) {
> -        error_setg(errp, "no filename");
> +    } else {
> +        error_setg(errp, "no params");
>          goto done;
>      }
> +
> +    assert(filename);
>      assert(driver);
> 
>      drive = g_new0(XenBlockDrive, 1);
> @@ -758,6 +758,7 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
> 
>      qdict_put_str(file_layer, "driver", "file");
>      qdict_put_str(file_layer, "filename", filename);
> +    g_free(filename);
> 
>      if (mode && *mode != 'w') {
>          qdict_put_bool(file_layer, "read-only", true);
> @@ -793,16 +794,17 @@ static XenBlockDrive *xen_block_drive_create(const
> char *id,
>      driver_layer = qdict_new();
> 
>      qdict_put_str(driver_layer, "driver", driver);
> +    g_free(driver);
> +
>      qdict_put_obj(driver_layer, "file", QOBJECT(file_layer));
> 
>      g_assert(!drive->node_name);
>      drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
>                                                &local_err);
> 
> -done:
> -    g_free(driver);
> -    g_free(filename);
> +    qobject_unref(driver_layer);
> 
> +done:
>      if (local_err) {
>          error_propagate(errp, local_err);
>          xen_block_drive_destroy(drive, NULL);
> --
> 2.20.1.2.gb21ebb6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH] xen-block: stop leaking memory in xen_block_drive_create()

[Peter Maydell]

Hi Paul -- you typoed the qemu-devel list email address; cc'd the right one.

thanks
-- PMM

On Tue, 19 Feb 2019 at 16:35, Paul Durrant <paul.durrant@citrix.com> wrote:
>
> The locally allocated QDict-s need to be freed. ('file_layer' will be
> freed implicitly since it is added as an object to 'driver_layer').
>
> Spotted by Coverity: CID 1398649
>
> While in the neighbourhood free 'driver' and 'filename' as soon as they are
> added to the QDicts. Freeing after the 'done' label doesn't make that much
> sense as, if the error path jumps to that label, the values would be NULL
> anyway.
>
> This patch also makes that more obvious by taking the error path if
> 'params' is NULL and then asserting that both driver and filename are
> non-NULL in the normal path.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  hw/block/xen-block.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 37a456c207..70fc2455e8 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -743,12 +743,12 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
>          }
>
>          g_strfreev(v);
> -    }
> -
> -    if (!filename) {
> -        error_setg(errp, "no filename");
> +    } else {
> +        error_setg(errp, "no params");
>          goto done;
>      }
> +
> +    assert(filename);
>      assert(driver);
>
>      drive = g_new0(XenBlockDrive, 1);
> @@ -758,6 +758,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
>
>      qdict_put_str(file_layer, "driver", "file");
>      qdict_put_str(file_layer, "filename", filename);
> +    g_free(filename);
>
>      if (mode && *mode != 'w') {
>          qdict_put_bool(file_layer, "read-only", true);
> @@ -793,16 +794,17 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
>      driver_layer = qdict_new();
>
>      qdict_put_str(driver_layer, "driver", driver);
> +    g_free(driver);
> +
>      qdict_put_obj(driver_layer, "file", QOBJECT(file_layer));
>
>      g_assert(!drive->node_name);
>      drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
>                                                &local_err);
>
> -done:
> -    g_free(driver);
> -    g_free(filename);
> +    qobject_unref(driver_layer);
>
> +done:
>      if (local_err) {
>          error_propagate(errp, local_err);
>          xen_block_drive_destroy(drive, NULL);
> --
> 2.20.1.2.gb21ebb6

Re: [PATCH] xen-block: stop leaking memory in xen_block_drive_create()

[Peter Maydell]

Hi Paul -- you typoed the qemu-devel list email address; cc'd the right one.

thanks
-- PMM

On Tue, 19 Feb 2019 at 16:35, Paul Durrant <paul.durrant@citrix.com> wrote:
>
> The locally allocated QDict-s need to be freed. ('file_layer' will be
> freed implicitly since it is added as an object to 'driver_layer').
>
> Spotted by Coverity: CID 1398649
>
> While in the neighbourhood free 'driver' and 'filename' as soon as they are
> added to the QDicts. Freeing after the 'done' label doesn't make that much
> sense as, if the error path jumps to that label, the values would be NULL
> anyway.
>
> This patch also makes that more obvious by taking the error path if
> 'params' is NULL and then asserting that both driver and filename are
> non-NULL in the normal path.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> ---
>  hw/block/xen-block.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index 37a456c207..70fc2455e8 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -743,12 +743,12 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
>          }
>
>          g_strfreev(v);
> -    }
> -
> -    if (!filename) {
> -        error_setg(errp, "no filename");
> +    } else {
> +        error_setg(errp, "no params");
>          goto done;
>      }
> +
> +    assert(filename);
>      assert(driver);
>
>      drive = g_new0(XenBlockDrive, 1);
> @@ -758,6 +758,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
>
>      qdict_put_str(file_layer, "driver", "file");
>      qdict_put_str(file_layer, "filename", filename);
> +    g_free(filename);
>
>      if (mode && *mode != 'w') {
>          qdict_put_bool(file_layer, "read-only", true);
> @@ -793,16 +794,17 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
>      driver_layer = qdict_new();
>
>      qdict_put_str(driver_layer, "driver", driver);
> +    g_free(driver);
> +
>      qdict_put_obj(driver_layer, "file", QOBJECT(file_layer));
>
>      g_assert(!drive->node_name);
>      drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
>                                                &local_err);
>
> -done:
> -    g_free(driver);
> -    g_free(filename);
> +    qobject_unref(driver_layer);
>
> +done:
>      if (local_err) {
>          error_propagate(errp, local_err);
>          xen_block_drive_destroy(drive, NULL);
> --
> 2.20.1.2.gb21ebb6

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Qemu-devel] [PATCH] xen-block: stop leaking memory in xen_block_drive_create()

[Anthony PERARD]

On Tue, Feb 19, 2019 at 04:36:28PM +0000, Paul Durrant wrote:
> > The locally allocated QDict-s need to be freed. ('file_layer' will be
> > freed implicitly since it is added as an object to 'driver_layer').
> > 
> > Spotted by Coverity: CID 1398649
> > 
> > While in the neighbourhood free 'driver' and 'filename' as soon as they
> > are
> > added to the QDicts. Freeing after the 'done' label doesn't make that much
> > sense as, if the error path jumps to that label, the values would be NULL
> > anyway.
> > 
> > This patch also makes that more obvious by taking the error path if
> > 'params' is NULL and then asserting that both driver and filename are
> > non-NULL in the normal path.
> > 
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH] xen-block: stop leaking memory in xen_block_drive_create()

[Anthony PERARD]

On Tue, Feb 19, 2019 at 04:36:28PM +0000, Paul Durrant wrote:
> > The locally allocated QDict-s need to be freed. ('file_layer' will be
> > freed implicitly since it is added as an object to 'driver_layer').
> > 
> > Spotted by Coverity: CID 1398649
> > 
> > While in the neighbourhood free 'driver' and 'filename' as soon as they
> > are
> > added to the QDicts. Freeing after the 'done' label doesn't make that much
> > sense as, if the error path jumps to that label, the values would be NULL
> > anyway.
> > 
> > This patch also makes that more obvious by taking the error path if
> > 'params' is NULL and then asserting that both driver and filename are
> > non-NULL in the normal path.
> > 
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH] xen-block: stop leaking memory in xen_block_drive_create()

[Paul Durrant]

The locally allocated QDict-s need to be freed. ('file_layer' will be
freed implicitly since it is added as an object to 'driver_layer').

Spotted by Coverity: CID 1398649

While in the neighbourhood free 'driver' and 'filename' as soon as they are
added to the QDicts. Freeing after the 'done' label doesn't make that much
sense as, if the error path jumps to that label, the values would be NULL
anyway.

This patch also makes that more obvious by taking the error path if
'params' is NULL and then asserting that both driver and filename are
non-NULL in the normal path.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
---
 hw/block/xen-block.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 37a456c207..70fc2455e8 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -743,12 +743,12 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
         }
 
         g_strfreev(v);
-    }
-
-    if (!filename) {
-        error_setg(errp, "no filename");
+    } else {
+        error_setg(errp, "no params");
         goto done;
     }
+
+    assert(filename);
     assert(driver);
 
     drive = g_new0(XenBlockDrive, 1);
@@ -758,6 +758,7 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
 
     qdict_put_str(file_layer, "driver", "file");
     qdict_put_str(file_layer, "filename", filename);
+    g_free(filename);
 
     if (mode && *mode != 'w') {
         qdict_put_bool(file_layer, "read-only", true);
@@ -793,16 +794,17 @@ static XenBlockDrive *xen_block_drive_create(const char *id,
     driver_layer = qdict_new();
 
     qdict_put_str(driver_layer, "driver", driver);
+    g_free(driver);
+
     qdict_put_obj(driver_layer, "file", QOBJECT(file_layer));
 
     g_assert(!drive->node_name);
     drive->node_name = xen_block_blockdev_add(drive->id, driver_layer,
                                               &local_err);
 
-done:
-    g_free(driver);
-    g_free(filename);
+    qobject_unref(driver_layer);
 
+done:
     if (local_err) {
         error_propagate(errp, local_err);
         xen_block_drive_destroy(drive, NULL);
-- 
2.20.1.2.gb21ebb6


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel