Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
03.05.2021 14:05, Kevin Wolf wrote:
Like other error paths, this one needs to call tran_finalize() and clean
up the BlockReopenQueue, too.
Fixes: CID 1452772
Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
Signed-off-by: Kevin Wolf
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 5c0ced6238..69615fabd1 100644
--- a/block.c
+++ b/block.c
@@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
ret = bdrv_flush(bs_entry->state.bs);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error flushing drive");
-goto cleanup;
+goto abort;
}
}
Reviewed-by: Vladimir Sementsov-Ogievskiy
--
Best regards,
Vladimir
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
03.05.2021 17:33, Kevin Wolf wrote: Am 03.05.2021 um 15:09 hat Vladimir Sementsov-Ogievskiy geschrieben: 03.05.2021 15:41, Kevin Wolf wrote: Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: 03.05.2021 14:05, Kevin Wolf wrote: Like other error paths, this one needs to call tran_finalize() and clean up the BlockReopenQueue, too. We don't need the "abort" loop on that path. And clean-up of BlockReopenQueue is at "cleanup:" label. The cleanup of the BlockReopenQueue itself is there, but not of all fields in it. Specifically, these lines are needed: qobject_unref(bs_entry->state.explicit_options); qobject_unref(bs_entry->state.options); The references are taken in bdrv_reopen_queue_child() and either used in commit or released on abort. Doing nothing with them leaks them. Oops. Somehow I decided they are set in _prepare. So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object") I don't like it because I think every call that doesn't end in a commit, should jump to the abort label to make sure that everything that remains unused because of this is correctly cleaned up. agree now.. Still, don't we miss these two qobject_unref() calls on success path? No, they are put to use in bdrv_reopen_commit(): qobject_unref(bs->explicit_options); qobject_unref(bs->options); bs->explicit_options = reopen_state->explicit_options; bs->options= reopen_state->options; Kevin OK then. I should have checked myself :\ -- Best regards, Vladimir
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
Am 03.05.2021 um 15:09 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.05.2021 15:41, Kevin Wolf wrote: > > Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > 03.05.2021 14:05, Kevin Wolf wrote: > > > > Like other error paths, this one needs to call tran_finalize() and clean > > > > up the BlockReopenQueue, too. > > > > > > We don't need the "abort" loop on that path. And clean-up of > > > BlockReopenQueue is at "cleanup:" label. > > > > The cleanup of the BlockReopenQueue itself is there, but not of all > > fields in it. Specifically, these lines are needed: > > > > qobject_unref(bs_entry->state.explicit_options); > > qobject_unref(bs_entry->state.options); > > > > The references are taken in bdrv_reopen_queue_child() and either used in > > commit or released on abort. Doing nothing with them leaks them. > > Oops. Somehow I decided they are set in _prepare. > > > > > > So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: > > > bdrv_reopen_multiple(): fix leak of tran object") > > > > I don't like it because I think every call that doesn't end in a commit, > > should jump to the abort label to make sure that everything that remains > > unused because of this is correctly cleaned up. > > > > > agree now.. > > Still, don't we miss these two qobject_unref() calls on success path? No, they are put to use in bdrv_reopen_commit(): qobject_unref(bs->explicit_options); qobject_unref(bs->options); bs->explicit_options = reopen_state->explicit_options; bs->options= reopen_state->options; Kevin
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
03.05.2021 15:41, Kevin Wolf wrote: Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: 03.05.2021 14:05, Kevin Wolf wrote: Like other error paths, this one needs to call tran_finalize() and clean up the BlockReopenQueue, too. We don't need the "abort" loop on that path. And clean-up of BlockReopenQueue is at "cleanup:" label. The cleanup of the BlockReopenQueue itself is there, but not of all fields in it. Specifically, these lines are needed: qobject_unref(bs_entry->state.explicit_options); qobject_unref(bs_entry->state.options); The references are taken in bdrv_reopen_queue_child() and either used in commit or released on abort. Doing nothing with them leaks them. Oops. Somehow I decided they are set in _prepare. So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: bdrv_reopen_multiple(): fix leak of tran object") I don't like it because I think every call that doesn't end in a commit, should jump to the abort label to make sure that everything that remains unused because of this is correctly cleaned up. agree now.. Still, don't we miss these two qobject_unref() calls on success path? -- Best regards, Vladimir
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
Am 03.05.2021 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.05.2021 14:05, Kevin Wolf wrote: > > Like other error paths, this one needs to call tran_finalize() and clean > > up the BlockReopenQueue, too. > > We don't need the "abort" loop on that path. And clean-up of > BlockReopenQueue is at "cleanup:" label. The cleanup of the BlockReopenQueue itself is there, but not of all fields in it. Specifically, these lines are needed: qobject_unref(bs_entry->state.explicit_options); qobject_unref(bs_entry->state.options); The references are taken in bdrv_reopen_queue_child() and either used in commit or released on abort. Doing nothing with them leaks them. > So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: > bdrv_reopen_multiple(): fix leak of tran object") I don't like it because I think every call that doesn't end in a commit, should jump to the abort label to make sure that everything that remains unused because of this is correctly cleaned up. Kevin
Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
03.05.2021 14:05, Kevin Wolf wrote:
Like other error paths, this one needs to call tran_finalize() and clean
up the BlockReopenQueue, too.
We don't need the "abort" loop on that path. And clean-up of BlockReopenQueue is at
"cleanup:" label.
So I'd prefer Peter's suggestion (my "[PATCH 2/6] block: bdrv_reopen_multiple(): fix
leak of tran object")
Fixes: CID 1452772
Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
Signed-off-by: Kevin Wolf
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 5c0ced6238..69615fabd1 100644
--- a/block.c
+++ b/block.c
@@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
ret = bdrv_flush(bs_entry->state.bs);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error flushing drive");
-goto cleanup;
+goto abort;
}
}
--
Best regards,
Vladimir
[PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple()
Like other error paths, this one needs to call tran_finalize() and clean
up the BlockReopenQueue, too.
Fixes: CID 1452772
Fixes: 72373e40fbc7e4218061a8211384db362d3e7348
Signed-off-by: Kevin Wolf
---
block.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 5c0ced6238..69615fabd1 100644
--- a/block.c
+++ b/block.c
@@ -4052,7 +4052,7 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue,
Error **errp)
ret = bdrv_flush(bs_entry->state.bs);
if (ret < 0) {
error_setg_errno(errp, -ret, "Error flushing drive");
-goto cleanup;
+goto abort;
}
}
--
2.30.2
