| Message ID | 20241111145214.8261-1-farosas@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix leak in send_qmp_error_event | expand |
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); ---
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 --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
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(-)