* 回复: Re: [PATCH] block/curl: free s->password in cleanup paths @ 2026-03-24 3:25 赵国汗 2026-03-24 4:53 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 5+ messages in thread From: 赵国汗 @ 2026-03-24 3:25 UTC (permalink / raw) To: zhaoguohan_salmon, Kevin Wolf, Hanna Reitz, Philippe Mathieu-Daudé Cc: qemu-block, qemu-devel [-- Attachment #1: Type: text/html, Size: 3653 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: 回复: Re: [PATCH] block/curl: free s->password in cleanup paths 2026-03-24 3:25 回复: Re: [PATCH] block/curl: free s->password in cleanup paths 赵国汗 @ 2026-03-24 4:53 ` Philippe Mathieu-Daudé 2026-03-24 6:10 ` [PATCH] block/curl: factor common cleanup and free password zhaoguohan 2026-03-24 6:13 ` [PATCH v2] " zhaoguohan 0 siblings, 2 replies; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2026-03-24 4:53 UTC (permalink / raw) To: 赵国汗, zhaoguohan_salmon, Kevin Wolf, Hanna Reitz Cc: qemu-block, qemu-devel On 24/3/26 04:25, 赵国汗 wrote: > > Should we directly call curl_close() here instead? Otherwise > > > factor a common curl_cleanup() out and reuse? > > Thanks for the suggestion. > > I don't think curl_close() can be used directly from out_noclean, since > out_noclean is reached from partially initialized states. > > curl_close() assumes a successfully opened instance and tears down the > aio/curl state via curl_detach_aio_context(), while out_noclean is used > before curl_attach_aio_context() and, in some cases, before s->sockets is > initialized. > > So I think reusing curl_close() here would conflate the open-failure and > normal close paths. > > If we want to deduplicate this later, factoring out only the common field > cleanup into a small helper should be safer. Yes, this is what I meant. Regards, Phil. > *主 题：*Re: [PATCH] block/curl: free s->password in cleanup paths > *日 期：*2026年03月20日17:24 > *发件人：*Philippe Mathieu-Daudé > *收件人：*zhaoguohan_salmon,Kevin Wolf,Hanna Reitz,Philippe Mathieu-Daudé > *抄送人：*qemu-block,qemu-devel > > Hi, On 20/3/26 07:30, zhaoguohan_salmon@163.com wrote: > From: GuoHan > Zhao <zhaoguohan@kylinos.cn> > > When password-secret is used, > curl_open() resolves it with > qcrypto_secret_lookup_as_utf8() and > stores the returned buffer in > s->password. > > Unlike s- > >proxypassword, s->password is not freed either in the open > failure > path or in curl_close(), so the resolved secret leaks once it > has been > allocated. > > Free s->password in both cleanup paths. > > Signed-off- > by: GuoHan Zhao <zhaoguohan@kylinos.cn> > --- > block/curl.c | 2 ++ > 1 > file changed, 2 insertions(+) > > diff --git a/block/curl.c b/block/ > curl.c > index 66aecfb20ec6..419df78258bc 100644 > --- a/block/curl.c > > +++ b/block/curl.c > @@ -903,6 +903,7 @@ out_noclean: > g_free(s- > >cookie); > g_free(s->url); > g_free(s->username); > + g_free(s- > >password); > g_free(s->proxyusername); > g_free(s->proxypassword); > > if (s->sockets) { Should we directly call curl_close() here instead? > Otherwise factor a common curl_cleanup() out and reuse? > @@ -1014,6 > +1015,7 @@ static void curl_close(BlockDriverState *bs) > g_free(s- > >cookie); > g_free(s->url); > g_free(s->username); > + g_free(s- > >password); > g_free(s->proxyusername); > g_free(s->proxypassword); > } > </zhaoguohan@kylinos.cn></zhaoguohan@kylinos.cn> > > > --- > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] block/curl: factor common cleanup and free password 2026-03-24 4:53 ` Philippe Mathieu-Daudé @ 2026-03-24 6:10 ` zhaoguohan 2026-03-24 6:13 ` [PATCH v2] " zhaoguohan 1 sibling, 0 replies; 5+ messages in thread From: zhaoguohan @ 2026-03-24 6:10 UTC (permalink / raw) To: Kevin Wolf, Hanna Reitz, Philippe Mathieu-Daudé Cc: qemu-block, qemu-devel, GuoHan Zhao From: GuoHan Zhao <zhaoguohan@kylinos.cn> Factor duplicated cleanup code into a helper and reuse it in the open failure path and curl_close(). Also free s->password, fixing a leak. Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn> --- block/curl.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/block/curl.c b/block/curl.c index 66aecfb20ec6..577b8d5a4e67 100644 --- a/block/curl.c +++ b/block/curl.c @@ -622,6 +622,27 @@ static void curl_attach_aio_context(BlockDriverState *bs, curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb); } +static void curl_cleanup(BDRVCURLState *s) +{ + g_free(s->cookie); + s->cookie = NULL; + g_free(s->url); + s->url = NULL; + g_free(s->username); + s->username = NULL; + g_free(s->password); + s->password = NULL; + g_free(s->proxyusername); + s->proxyusername = NULL; + g_free(s->proxypassword); + s->proxypassword = NULL; + if (s->sockets) { + curl_drop_all_sockets(s->sockets); + g_hash_table_destroy(s->sockets); + s->sockets = NULL; + } +} + static QemuOptsList runtime_opts = { .name = "curl", .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), @@ -900,15 +921,7 @@ out: state->curl = NULL; out_noclean: qemu_mutex_destroy(&s->mutex); - g_free(s->cookie); - g_free(s->url); - g_free(s->username); - g_free(s->proxyusername); - g_free(s->proxypassword); - if (s->sockets) { - curl_drop_all_sockets(s->sockets); - g_hash_table_destroy(s->sockets); - } + curl_cleanup(s); qemu_opts_del(opts); return -EINVAL; } @@ -1010,12 +1023,7 @@ static void curl_close(BlockDriverState *bs) curl_detach_aio_context(bs); qemu_mutex_destroy(&s->mutex); - g_hash_table_destroy(s->sockets); - g_free(s->cookie); - g_free(s->url); - g_free(s->username); - g_free(s->proxyusername); - g_free(s->proxypassword); + curl_cleanup(s); } static int64_t coroutine_fn curl_co_getlength(BlockDriverState *bs) -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] block/curl: factor common cleanup and free password 2026-03-24 4:53 ` Philippe Mathieu-Daudé 2026-03-24 6:10 ` [PATCH] block/curl: factor common cleanup and free password zhaoguohan @ 2026-03-24 6:13 ` zhaoguohan 2026-03-24 9:19 ` Daniel P. Berrangé 1 sibling, 1 reply; 5+ messages in thread From: zhaoguohan @ 2026-03-24 6:13 UTC (permalink / raw) To: Kevin Wolf, Hanna Reitz, Philippe Mathieu-Daudé Cc: qemu-block, qemu-devel, GuoHan Zhao From: GuoHan Zhao <zhaoguohan@kylinos.cn> Factor duplicated cleanup code into a helper and reuse it in the open failure path and curl_close(). Also free s->password, fixing a leak. Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn> --- block/curl.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/block/curl.c b/block/curl.c index 66aecfb20ec6..577b8d5a4e67 100644 --- a/block/curl.c +++ b/block/curl.c @@ -622,6 +622,27 @@ static void curl_attach_aio_context(BlockDriverState *bs, curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb); } +static void curl_cleanup(BDRVCURLState *s) +{ + g_free(s->cookie); + s->cookie = NULL; + g_free(s->url); + s->url = NULL; + g_free(s->username); + s->username = NULL; + g_free(s->password); + s->password = NULL; + g_free(s->proxyusername); + s->proxyusername = NULL; + g_free(s->proxypassword); + s->proxypassword = NULL; + if (s->sockets) { + curl_drop_all_sockets(s->sockets); + g_hash_table_destroy(s->sockets); + s->sockets = NULL; + } +} + static QemuOptsList runtime_opts = { .name = "curl", .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), @@ -900,15 +921,7 @@ out: state->curl = NULL; out_noclean: qemu_mutex_destroy(&s->mutex); - g_free(s->cookie); - g_free(s->url); - g_free(s->username); - g_free(s->proxyusername); - g_free(s->proxypassword); - if (s->sockets) { - curl_drop_all_sockets(s->sockets); - g_hash_table_destroy(s->sockets); - } + curl_cleanup(s); qemu_opts_del(opts); return -EINVAL; } @@ -1010,12 +1023,7 @@ static void curl_close(BlockDriverState *bs) curl_detach_aio_context(bs); qemu_mutex_destroy(&s->mutex); - g_hash_table_destroy(s->sockets); - g_free(s->cookie); - g_free(s->url); - g_free(s->username); - g_free(s->proxyusername); - g_free(s->proxypassword); + curl_cleanup(s); } static int64_t coroutine_fn curl_co_getlength(BlockDriverState *bs) -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] block/curl: factor common cleanup and free password 2026-03-24 6:13 ` [PATCH v2] " zhaoguohan @ 2026-03-24 9:19 ` Daniel P. Berrangé 0 siblings, 0 replies; 5+ messages in thread From: Daniel P. Berrangé @ 2026-03-24 9:19 UTC (permalink / raw) To: zhaoguohan Cc: Kevin Wolf, Hanna Reitz, Philippe Mathieu-Daudé, qemu-block, qemu-devel On Tue, Mar 24, 2026 at 02:13:45PM +0800, zhaoguohan@kylinos.cn wrote: > From: GuoHan Zhao <zhaoguohan@kylinos.cn> > > Factor duplicated cleanup code into a helper and reuse it in the > open failure path and curl_close(). > > Also free s->password, fixing a leak. Please don't mix bug fixes with refactorings. These should be separate commits. > Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn> > --- > block/curl.c | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > diff --git a/block/curl.c b/block/curl.c > index 66aecfb20ec6..577b8d5a4e67 100644 > --- a/block/curl.c > +++ b/block/curl.c > @@ -622,6 +622,27 @@ static void curl_attach_aio_context(BlockDriverState *bs, > curl_multi_setopt(s->multi, CURLMOPT_TIMERFUNCTION, curl_timer_cb); > } > > +static void curl_cleanup(BDRVCURLState *s) > +{ > + g_free(s->cookie); > + s->cookie = NULL; Use this pattern: g_clear_pointer(&s->cookie, g_free); > + g_free(s->url); > + s->url = NULL; > + g_free(s->username); > + s->username = NULL; > + g_free(s->password); > + s->password = NULL; > + g_free(s->proxyusername); > + s->proxyusername = NULL; > + g_free(s->proxypassword); > + s->proxypassword = NULL; > + if (s->sockets) { > + curl_drop_all_sockets(s->sockets); > + g_hash_table_destroy(s->sockets); > + s->sockets = NULL; g_clear_pointer(&s->sockets, g_hash_table_destroy) > + } > +} > + > static QemuOptsList runtime_opts = { > .name = "curl", > .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head), > @@ -900,15 +921,7 @@ out: > state->curl = NULL; > out_noclean: > qemu_mutex_destroy(&s->mutex); > - g_free(s->cookie); > - g_free(s->url); > - g_free(s->username); > - g_free(s->proxyusername); > - g_free(s->proxypassword); > - if (s->sockets) { > - curl_drop_all_sockets(s->sockets); > - g_hash_table_destroy(s->sockets); > - } > + curl_cleanup(s); > qemu_opts_del(opts); > return -EINVAL; > } > @@ -1010,12 +1023,7 @@ static void curl_close(BlockDriverState *bs) > curl_detach_aio_context(bs); > qemu_mutex_destroy(&s->mutex); > > - g_hash_table_destroy(s->sockets); > - g_free(s->cookie); > - g_free(s->url); > - g_free(s->username); > - g_free(s->proxyusername); > - g_free(s->proxypassword); > + curl_cleanup(s); > } > > static int64_t coroutine_fn curl_co_getlength(BlockDriverState *bs) > -- > 2.43.0 > > With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :| ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-03-24 9:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-24 3:25 回复: Re: [PATCH] block/curl: free s->password in cleanup paths 赵国汗 2026-03-24 4:53 ` Philippe Mathieu-Daudé 2026-03-24 6:10 ` [PATCH] block/curl: factor common cleanup and free password zhaoguohan 2026-03-24 6:13 ` [PATCH v2] " zhaoguohan 2026-03-24 9:19 ` Daniel P. Berrangé
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.