diff mbox series

block: Fix leak in send_qmp_error_event

Message ID 20241111145214.8261-1-farosas@suse.de
State New
Headers show
Series block: Fix leak in send_qmp_error_event | expand

Commit Message

Fabiano Rosas Nov. 11, 2024, 2:52 p.m. UTC
ASAN detected a leak when running the ahci-test
/ahci/io/dma/lba28/retry:

Direct leak of 35 byte(s) in 1 object(s) allocated from:
    #0 in malloc
    #1 in __vasprintf_internal
    #2 in vasprintf
    #3 in g_vasprintf
    #4 in g_strdup_vprintf
    #5 in g_strdup_printf
    #6 in object_get_canonical_path ../qom/object.c:2096:19
    #7 in blk_get_attached_dev_id_or_path ../block/block-backend.c:1033:12
    #8 in blk_get_attached_dev_path ../block/block-backend.c:1047:12
    #9 in send_qmp_error_event ../block/block-backend.c:2140:36
    #10 in blk_error_action ../block/block-backend.c:2172:9
    #11 in ide_handle_rw_error ../hw/ide/core.c:875:5
    #12 in ide_dma_cb ../hw/ide/core.c:894:13
    #13 in dma_complete ../system/dma-helpers.c:107:9
    #14 in dma_blk_cb ../system/dma-helpers.c:129:9
    #15 in blk_aio_complete ../block/block-backend.c:1552:9
    #16 in blk_aio_write_entry ../block/block-backend.c:1619:5
    #17 in coroutine_trampoline ../util/coroutine-ucontext.c:175:9

Plug the leak by freeing the device path string.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 block/block-backend.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé Nov. 11, 2024, 4:58 p.m. UTC | #1
On 11/11/24 14:52, Fabiano Rosas wrote:
> ASAN detected a leak when running the ahci-test
> /ahci/io/dma/lba28/retry:
> 
> Direct leak of 35 byte(s) in 1 object(s) allocated from:
>      #0 in malloc
>      #1 in __vasprintf_internal
>      #2 in vasprintf
>      #3 in g_vasprintf
>      #4 in g_strdup_vprintf
>      #5 in g_strdup_printf
>      #6 in object_get_canonical_path ../qom/object.c:2096:19
>      #7 in blk_get_attached_dev_id_or_path ../block/block-backend.c:1033:12
>      #8 in blk_get_attached_dev_path ../block/block-backend.c:1047:12
>      #9 in send_qmp_error_event ../block/block-backend.c:2140:36
>      #10 in blk_error_action ../block/block-backend.c:2172:9
>      #11 in ide_handle_rw_error ../hw/ide/core.c:875:5
>      #12 in ide_dma_cb ../hw/ide/core.c:894:13
>      #13 in dma_complete ../system/dma-helpers.c:107:9
>      #14 in dma_blk_cb ../system/dma-helpers.c:129:9
>      #15 in blk_aio_complete ../block/block-backend.c:1552:9
>      #16 in blk_aio_write_entry ../block/block-backend.c:1619:5
>      #17 in coroutine_trampoline ../util/coroutine-ucontext.c:175:9
> 
> Plug the leak by freeing the device path string.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>   block/block-backend.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 85bcdedcef..a3b7f00188 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -2134,13 +2134,14 @@ static void send_qmp_error_event(BlockBackend *blk,
>   {
>       IoOperationType optype;
>       BlockDriverState *bs = blk_bs(blk);
> +    char *path = blk_get_attached_dev_path(blk);

Preferably using g_autofree,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   
>       optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
> -    qapi_event_send_block_io_error(blk_name(blk),
> -                                   blk_get_attached_dev_path(blk),
> +    qapi_event_send_block_io_error(blk_name(blk), path,
>                                      bs ? bdrv_get_node_name(bs) : NULL, optype,
>                                      action, blk_iostatus_is_enabled(blk),
>                                      error == ENOSPC, strerror(error));
> +    g_free(path);
>   }
>   
>   /* This is done by device models because, while the block layer knows

Having read this patch, we should improve the doc for this methods.
I'll post the following later:

-- >8 --
diff --git a/include/sysemu/block-backend-io.h 
b/include/sysemu/block-backend-io.h
index d174275a5c..ba8dfcc7d0 100644
--- a/include/sysemu/block-backend-io.h
+++ b/include/sysemu/block-backend-io.h
@@ -32,6 +32,13 @@ void blk_set_allow_aio_context_change(BlockBackend 
*blk, bool allow);
  void blk_set_disable_request_queuing(BlockBackend *blk, bool disable);
  bool blk_iostatus_is_enabled(const BlockBackend *blk);

+/*
+ * Return the qdev ID, or if no ID is assigned the QOM path,
+ * of the block device attached to the BlockBackend.
+ *
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
  char *blk_get_attached_dev_id(BlockBackend *blk);

  BlockAIOCB *blk_aio_pwrite_zeroes(BlockBackend *blk, int64_t offset,
diff --git a/block/block-backend.c b/block/block-backend.c
index 85bcdedcef..6128012953 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1019,6 +1019,10 @@ DeviceState *blk_get_attached_dev(BlockBackend *blk)
      return blk->dev;
  }

+/*
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
  static char *blk_get_attached_dev_id_or_path(BlockBackend *blk, bool 
want_id)
  {
      DeviceState *dev = blk->dev;
@@ -1033,15 +1037,15 @@ static char 
*blk_get_attached_dev_id_or_path(BlockBackend *blk, bool want_id)
      return object_get_canonical_path(OBJECT(dev)) ?: g_strdup("");
  }

-/*
- * Return the qdev ID, or if no ID is assigned the QOM path, of the block
- * device attached to the BlockBackend.
- */
  char *blk_get_attached_dev_id(BlockBackend *blk)
  {
      return blk_get_attached_dev_id_or_path(blk, true);
  }

+/*
+ * The caller is responsible for releasing the value returned
+ * with g_free() after use.
+ */
  static char *blk_get_attached_dev_path(BlockBackend *blk)
  {
      return blk_get_attached_dev_id_or_path(blk, false);
---
Fabiano Rosas Nov. 11, 2024, 5:10 p.m. UTC | #2
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 11/11/24 14:52, Fabiano Rosas wrote:
>> ASAN detected a leak when running the ahci-test
>> /ahci/io/dma/lba28/retry:
>> 
>> Direct leak of 35 byte(s) in 1 object(s) allocated from:
>>      #0 in malloc
>>      #1 in __vasprintf_internal
>>      #2 in vasprintf
>>      #3 in g_vasprintf
>>      #4 in g_strdup_vprintf
>>      #5 in g_strdup_printf
>>      #6 in object_get_canonical_path ../qom/object.c:2096:19
>>      #7 in blk_get_attached_dev_id_or_path ../block/block-backend.c:1033:12
>>      #8 in blk_get_attached_dev_path ../block/block-backend.c:1047:12
>>      #9 in send_qmp_error_event ../block/block-backend.c:2140:36
>>      #10 in blk_error_action ../block/block-backend.c:2172:9
>>      #11 in ide_handle_rw_error ../hw/ide/core.c:875:5
>>      #12 in ide_dma_cb ../hw/ide/core.c:894:13
>>      #13 in dma_complete ../system/dma-helpers.c:107:9
>>      #14 in dma_blk_cb ../system/dma-helpers.c:129:9
>>      #15 in blk_aio_complete ../block/block-backend.c:1552:9
>>      #16 in blk_aio_write_entry ../block/block-backend.c:1619:5
>>      #17 in coroutine_trampoline ../util/coroutine-ucontext.c:175:9
>> 
>> Plug the leak by freeing the device path string.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   block/block-backend.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 85bcdedcef..a3b7f00188 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -2134,13 +2134,14 @@ static void send_qmp_error_event(BlockBackend *blk,
>>   {
>>       IoOperationType optype;
>>       BlockDriverState *bs = blk_bs(blk);
>> +    char *path = blk_get_attached_dev_path(blk);
>
> Preferably using g_autofree,

Of course, I'll repost. Thanks!
diff mbox series

Patch

diff --git a/block/block-backend.c b/block/block-backend.c
index 85bcdedcef..a3b7f00188 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2134,13 +2134,14 @@  static void send_qmp_error_event(BlockBackend *blk,
 {
     IoOperationType optype;
     BlockDriverState *bs = blk_bs(blk);
+    char *path = blk_get_attached_dev_path(blk);
 
     optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-    qapi_event_send_block_io_error(blk_name(blk),
-                                   blk_get_attached_dev_path(blk),
+    qapi_event_send_block_io_error(blk_name(blk), path,
                                    bs ? bdrv_get_node_name(bs) : NULL, optype,
                                    action, blk_iostatus_is_enabled(blk),
                                    error == ENOSPC, strerror(error));
+    g_free(path);
 }
 
 /* This is done by device models because, while the block layer knows