回复: Re: [PATCH] block/curl: free s->password in cleanup paths

回复: Re: [PATCH] block/curl: free s->password in cleanup paths

[赵国汗]

[-- Attachment #1: Type: text/html, Size: 3653 bytes --]

Re: 回复: Re: [PATCH] block/curl: free s->password in cleanup paths

[Philippe Mathieu-Daudé]

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>
> 
> 
> ---
> 

[PATCH] block/curl: factor common cleanup and free password

[zhaoguohan]

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

[PATCH v2] block/curl: factor common cleanup and free password

[zhaoguohan]

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

Re: [PATCH v2] block/curl: factor common cleanup and free password

[Daniel P. Berrangé]

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 :|