* [PATCH 0/2] block: Fix Transaction leaks
@ 2021-05-03 11:05 Kevin Wolf
2021-05-03 11:05 ` [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() Kevin Wolf
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Kevin Wolf @ 2021-05-03 11:05 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz
These are two follow-up fixes for Vladimir's "block: update graph
permissions update". The bugs were reported by Coverity.
Kevin Wolf (2):
block: Fix Transaction leak in bdrv_root_attach_child()
block: Fix Transaction leak in bdrv_reopen_multiple()
block.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() 2021-05-03 11:05 [PATCH 0/2] block: Fix Transaction leaks Kevin Wolf @ 2021-05-03 11:05 ` Kevin Wolf 2021-05-03 11:43 ` Vladimir Sementsov-Ogievskiy 2021-05-03 11:49 ` Max Reitz 2021-05-03 11:05 ` [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() Kevin Wolf 2021-05-04 6:56 ` [PATCH 0/2] block: Fix Transaction leaks Vladimir Sementsov-Ogievskiy 2 siblings, 2 replies; 17+ messages in thread From: Kevin Wolf @ 2021-05-03 11:05 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz The error path needs to call tran_finalize(), too. Fixes: CID 1452773 Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 874c22c43e..5c0ced6238 100644 --- a/block.c +++ b/block.c @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, child_role, perm, shared_perm, opaque, &child, tran, errp); if (ret < 0) { - bdrv_unref(child_bs); - return NULL; + assert(child == NULL); + goto out; } ret = bdrv_refresh_perms(child_bs, errp); - tran_finalize(tran, ret); +out: + tran_finalize(tran, ret); bdrv_unref(child_bs); return child; } -- 2.30.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() 2021-05-03 11:05 ` [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() Kevin Wolf @ 2021-05-03 11:43 ` Vladimir Sementsov-Ogievskiy 2021-05-03 11:49 ` Max Reitz 1 sibling, 0 replies; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-03 11:43 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: mreitz, qemu-devel 03.05.2021 14:05, Kevin Wolf wrote: > The error path needs to call tran_finalize(), too. > > Fixes: CID 1452773 > Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/block.c b/block.c > index 874c22c43e..5c0ced6238 100644 > --- a/block.c > +++ b/block.c > @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, > child_role, perm, shared_perm, opaque, > &child, tran, errp); > if (ret < 0) { > - bdrv_unref(child_bs); > - return NULL; > + assert(child == NULL); > + goto out; > } > > ret = bdrv_refresh_perms(child_bs, errp); > - tran_finalize(tran, ret); > > +out: > + tran_finalize(tran, ret); > bdrv_unref(child_bs); > return child; > } > I like my additional comment and assertion in "[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child", still this is OK too: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() 2021-05-03 11:05 ` [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() Kevin Wolf 2021-05-03 11:43 ` Vladimir Sementsov-Ogievskiy @ 2021-05-03 11:49 ` Max Reitz 2021-05-03 11:51 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 17+ messages in thread From: Max Reitz @ 2021-05-03 11:49 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: vsementsov, qemu-devel On 03.05.21 13:05, Kevin Wolf wrote: > The error path needs to call tran_finalize(), too. > > Fixes: CID 1452773 > Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/block.c b/block.c > index 874c22c43e..5c0ced6238 100644 > --- a/block.c > +++ b/block.c > @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, > child_role, perm, shared_perm, opaque, > &child, tran, errp); > if (ret < 0) { > - bdrv_unref(child_bs); > - return NULL; > + assert(child == NULL); > + goto out; > } > > ret = bdrv_refresh_perms(child_bs, errp); > - tran_finalize(tran, ret); > > +out: > + tran_finalize(tran, ret); > bdrv_unref(child_bs); > return child; Looks OK, so: Reviewed-by: Max Reitz <mreitz@redhat.com> However, the function’s description says that it will return NULL on error. But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is that right? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() 2021-05-03 11:49 ` Max Reitz @ 2021-05-03 11:51 ` Vladimir Sementsov-Ogievskiy 2021-05-03 11:53 ` Max Reitz 0 siblings, 1 reply; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-03 11:51 UTC (permalink / raw) To: Max Reitz, Kevin Wolf, qemu-block; +Cc: qemu-devel 03.05.2021 14:49, Max Reitz wrote: > On 03.05.21 13:05, Kevin Wolf wrote: >> The error path needs to call tran_finalize(), too. >> >> Fixes: CID 1452773 >> Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/block.c b/block.c >> index 874c22c43e..5c0ced6238 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, >> child_role, perm, shared_perm, opaque, >> &child, tran, errp); >> if (ret < 0) { >> - bdrv_unref(child_bs); >> - return NULL; >> + assert(child == NULL); >> + goto out; >> } >> ret = bdrv_refresh_perms(child_bs, errp); >> - tran_finalize(tran, ret); >> +out: >> + tran_finalize(tran, ret); >> bdrv_unref(child_bs); >> return child; > > Looks OK, so: > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > However, the function’s description says that it will return NULL on error. But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is that right? > No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child" -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() 2021-05-03 11:51 ` Vladimir Sementsov-Ogievskiy @ 2021-05-03 11:53 ` Max Reitz 2021-05-03 12:14 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 17+ messages in thread From: Max Reitz @ 2021-05-03 11:53 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, Kevin Wolf, qemu-block; +Cc: qemu-devel On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote: > 03.05.2021 14:49, Max Reitz wrote: >> On 03.05.21 13:05, Kevin Wolf wrote: >>> The error path needs to call tran_finalize(), too. >>> >>> Fixes: CID 1452773 >>> Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/block.c b/block.c >>> index 874c22c43e..5c0ced6238 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -2918,13 +2918,14 @@ BdrvChild >>> *bdrv_root_attach_child(BlockDriverState *child_bs, >>> child_role, perm, shared_perm, >>> opaque, >>> &child, tran, errp); >>> if (ret < 0) { >>> - bdrv_unref(child_bs); >>> - return NULL; >>> + assert(child == NULL); >>> + goto out; >>> } >>> ret = bdrv_refresh_perms(child_bs, errp); >>> - tran_finalize(tran, ret); >>> +out: >>> + tran_finalize(tran, ret); >>> bdrv_unref(child_bs); >>> return child; >> >> Looks OK, so: >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> However, the function’s description says that it will return NULL on >> error. But if bdrv_refresh_perms() fails, it will still return a >> non-NULL child. Is that right? >> > > No, it's reset to NULL on transaction abort, so code is correct. It's > not obvious, and I've added a comment and assertion in my version of > this fix "[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child" The fact that the transaction keeps the pointer to this local variable around is a bit horrifying, but well. Max ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() 2021-05-03 11:53 ` Max Reitz @ 2021-05-03 12:14 ` Vladimir Sementsov-Ogievskiy 2021-05-03 12:31 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-03 12:14 UTC (permalink / raw) To: Max Reitz, Kevin Wolf, qemu-block; +Cc: qemu-devel 03.05.2021 14:53, Max Reitz wrote: > On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote: >> 03.05.2021 14:49, Max Reitz wrote: >>> On 03.05.21 13:05, Kevin Wolf wrote: >>>> The error path needs to call tran_finalize(), too. >>>> >>>> Fixes: CID 1452773 >>>> Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 >>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>> --- >>>> block.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/block.c b/block.c >>>> index 874c22c43e..5c0ced6238 100644 >>>> --- a/block.c >>>> +++ b/block.c >>>> @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, >>>> child_role, perm, shared_perm, opaque, >>>> &child, tran, errp); >>>> if (ret < 0) { >>>> - bdrv_unref(child_bs); >>>> - return NULL; >>>> + assert(child == NULL); >>>> + goto out; >>>> } >>>> ret = bdrv_refresh_perms(child_bs, errp); >>>> - tran_finalize(tran, ret); >>>> +out: >>>> + tran_finalize(tran, ret); >>>> bdrv_unref(child_bs); >>>> return child; >>> >>> Looks OK, so: >>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>> >>> However, the function’s description says that it will return NULL on error. But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is that right? >>> >> >> No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child" > > The fact that the transaction keeps the pointer to this local variable around is a bit horrifying, but well. > Yes this looks overcomplicated here. But it's useful for bdrv_set_backing_noperm where we would have to add a separate entry to @tran to rollback bs->backing change. Probably, it's better to refactor this thing. Or at least document that out-pointer argument goes into transaction, and should be valid up to transaction finalization. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() 2021-05-03 12:14 ` Vladimir Sementsov-Ogievskiy @ 2021-05-03 12:31 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-03 12:31 UTC (permalink / raw) To: Max Reitz, Kevin Wolf, qemu-block; +Cc: qemu-devel 03.05.2021 15:14, Vladimir Sementsov-Ogievskiy wrote: > 03.05.2021 14:53, Max Reitz wrote: >> On 03.05.21 13:51, Vladimir Sementsov-Ogievskiy wrote: >>> 03.05.2021 14:49, Max Reitz wrote: >>>> On 03.05.21 13:05, Kevin Wolf wrote: >>>>> The error path needs to call tran_finalize(), too. >>>>> >>>>> Fixes: CID 1452773 >>>>> Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1 >>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>>> --- >>>>> block.c | 7 ++++--- >>>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/block.c b/block.c >>>>> index 874c22c43e..5c0ced6238 100644 >>>>> --- a/block.c >>>>> +++ b/block.c >>>>> @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs, >>>>> child_role, perm, shared_perm, opaque, >>>>> &child, tran, errp); >>>>> if (ret < 0) { >>>>> - bdrv_unref(child_bs); >>>>> - return NULL; >>>>> + assert(child == NULL); >>>>> + goto out; >>>>> } >>>>> ret = bdrv_refresh_perms(child_bs, errp); >>>>> - tran_finalize(tran, ret); >>>>> +out: >>>>> + tran_finalize(tran, ret); >>>>> bdrv_unref(child_bs); >>>>> return child; >>>> >>>> Looks OK, so: >>>> >>>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>>> >>>> However, the function’s description says that it will return NULL on error. But if bdrv_refresh_perms() fails, it will still return a non-NULL child. Is that right? >>>> >>> >>> No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix leak of tran in bdrv_root_attach_child" >> >> The fact that the transaction keeps the pointer to this local variable around is a bit horrifying, but well. >> > > Yes this looks overcomplicated here. But it's useful for bdrv_set_backing_noperm where we would have to add a separate entry to @tran to rollback bs->backing change. Probably, it's better to refactor this thing. Or at least document that out-pointer argument goes into transaction, and should be valid up to transaction finalization. > > > documentation done: [PATCH 7/6] block: document child argument of bdrv_attach_child_common() -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() 2021-05-03 11:05 [PATCH 0/2] block: Fix Transaction leaks Kevin Wolf 2021-05-03 11:05 ` [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() Kevin Wolf @ 2021-05-03 11:05 ` Kevin Wolf 2021-05-03 11:40 ` Vladimir Sementsov-Ogievskiy 2021-05-04 6:51 ` Vladimir Sementsov-Ogievskiy 2021-05-04 6:56 ` [PATCH 0/2] block: Fix Transaction leaks Vladimir Sementsov-Ogievskiy 2 siblings, 2 replies; 17+ messages in thread From: Kevin Wolf @ 2021-05-03 11:05 UTC (permalink / raw) To: qemu-block; +Cc: kwolf, vsementsov, qemu-devel, mreitz 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 <kwolf@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() 2021-05-03 11:05 ` [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() Kevin Wolf @ 2021-05-03 11:40 ` Vladimir Sementsov-Ogievskiy 2021-05-03 12:41 ` Kevin Wolf 2021-05-04 6:51 ` Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-03 11:40 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: mreitz, qemu-devel 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 <kwolf@redhat.com> > --- > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() 2021-05-03 11:40 ` Vladimir Sementsov-Ogievskiy @ 2021-05-03 12:41 ` Kevin Wolf 2021-05-03 13:09 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 17+ messages in thread From: Kevin Wolf @ 2021-05-03 12:41 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() 2021-05-03 12:41 ` Kevin Wolf @ 2021-05-03 13:09 ` Vladimir Sementsov-Ogievskiy 2021-05-03 14:33 ` Kevin Wolf 0 siblings, 1 reply; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-03 13:09 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, mreitz, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() 2021-05-03 13:09 ` Vladimir Sementsov-Ogievskiy @ 2021-05-03 14:33 ` Kevin Wolf 2021-05-04 6:52 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 17+ messages in thread From: Kevin Wolf @ 2021-05-03 14:33 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() 2021-05-03 14:33 ` Kevin Wolf @ 2021-05-04 6:52 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-04 6:52 UTC (permalink / raw) To: Kevin Wolf; +Cc: qemu-block, mreitz, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() 2021-05-03 11:05 ` [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() Kevin Wolf 2021-05-03 11:40 ` Vladimir Sementsov-Ogievskiy @ 2021-05-04 6:51 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-04 6:51 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: mreitz, qemu-devel 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 <kwolf@redhat.com> > --- > 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 <vsementsov@virtuozzo.com> -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] block: Fix Transaction leaks 2021-05-03 11:05 [PATCH 0/2] block: Fix Transaction leaks Kevin Wolf 2021-05-03 11:05 ` [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() Kevin Wolf 2021-05-03 11:05 ` [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() Kevin Wolf @ 2021-05-04 6:56 ` Vladimir Sementsov-Ogievskiy 2021-05-05 10:25 ` Kevin Wolf 2 siblings, 1 reply; 17+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-05-04 6:56 UTC (permalink / raw) To: Kevin Wolf, qemu-block; +Cc: mreitz, qemu-devel 03.05.2021 14:05, Kevin Wolf wrote: > These are two follow-up fixes for Vladimir's "block: update graph > permissions update". The bugs were reported by Coverity. > > Kevin Wolf (2): > block: Fix Transaction leak in bdrv_root_attach_child() > block: Fix Transaction leak in bdrv_reopen_multiple() > > block.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > I'll rebase my "[PATCH 0/6] block permission updated follow-up" on top of this. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/2] block: Fix Transaction leaks 2021-05-04 6:56 ` [PATCH 0/2] block: Fix Transaction leaks Vladimir Sementsov-Ogievskiy @ 2021-05-05 10:25 ` Kevin Wolf 0 siblings, 0 replies; 17+ messages in thread From: Kevin Wolf @ 2021-05-05 10:25 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy; +Cc: qemu-devel, qemu-block, mreitz Am 04.05.2021 um 08:56 hat Vladimir Sementsov-Ogievskiy geschrieben: > 03.05.2021 14:05, Kevin Wolf wrote: > > These are two follow-up fixes for Vladimir's "block: update graph > > permissions update". The bugs were reported by Coverity. > > > > Kevin Wolf (2): > > block: Fix Transaction leak in bdrv_root_attach_child() > > block: Fix Transaction leak in bdrv_reopen_multiple() > > > > block.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > I'll rebase my "[PATCH 0/6] block permission updated follow-up" on top > of this. Ok, thanks. I've applied this to my block branch now. Kevin ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-05-05 10:26 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-03 11:05 [PATCH 0/2] block: Fix Transaction leaks Kevin Wolf 2021-05-03 11:05 ` [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child() Kevin Wolf 2021-05-03 11:43 ` Vladimir Sementsov-Ogievskiy 2021-05-03 11:49 ` Max Reitz 2021-05-03 11:51 ` Vladimir Sementsov-Ogievskiy 2021-05-03 11:53 ` Max Reitz 2021-05-03 12:14 ` Vladimir Sementsov-Ogievskiy 2021-05-03 12:31 ` Vladimir Sementsov-Ogievskiy 2021-05-03 11:05 ` [PATCH 2/2] block: Fix Transaction leak in bdrv_reopen_multiple() Kevin Wolf 2021-05-03 11:40 ` Vladimir Sementsov-Ogievskiy 2021-05-03 12:41 ` Kevin Wolf 2021-05-03 13:09 ` Vladimir Sementsov-Ogievskiy 2021-05-03 14:33 ` Kevin Wolf 2021-05-04 6:52 ` Vladimir Sementsov-Ogievskiy 2021-05-04 6:51 ` Vladimir Sementsov-Ogievskiy 2021-05-04 6:56 ` [PATCH 0/2] block: Fix Transaction leaks Vladimir Sementsov-Ogievskiy 2021-05-05 10:25 ` Kevin Wolf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.