ix double free in URI alias lookup and fix misleading error with libvirtd

[PATCH v2 1/9] virConnectOpenInternal: Avoid double free() when alias is an invalid URI

Posted by Peter Krempa 3 years, 5 months ago

Configuring an URI alias such as

  uri_aliases = [
      "blah=qemu://invaliduri@@@",
  ]

Results in a double free when the alias is used:

  $ virsh -c blah
  free(): double free detected in tcache 2
  Aborted (core dumped)

This happens as the 'alias' variable is first assigned to 'uristr' which
is cleared in the 'failed' label and then is explicitly freed again.

Fix this by switching to 'g_autofree' for alias and stealing it into
'uristr'.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/libvirt.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/libvirt.c b/src/libvirt.c
index b78b49a632..7e7c0fc8e3 100644
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -933,21 +933,19 @@ virConnectOpenInternal(const char *name,
     }

     if (uristr) {
-        char *alias = NULL;
+        g_autofree char *alias = NULL;

         if (!(flags & VIR_CONNECT_NO_ALIASES) &&
             virURIResolveAlias(conf, uristr, &alias) < 0)
             goto failed;

         if (alias) {
-            VIR_FREE(uristr);
-            uristr = alias;
+            g_free(uristr);
+            uristr = g_steal_pointer(&alias);
         }

-        if (!(ret->uri = virURIParse(uristr))) {
-            VIR_FREE(alias);
+        if (!(ret->uri = virURIParse(uristr)))
             goto failed;
-        }

         /* Avoid need for drivers to worry about NULLs, as
          * no one needs to distinguish "" vs NULL */
-- 
2.37.1

Re: [PATCH v2 1/9] virConnectOpenInternal: Avoid double free() when alias is an invalid URI

Posted by Ján Tomko 3 years, 5 months ago

On a Friday in 2022, Peter Krempa wrote:
>Configuring an URI alias such as
>
>  uri_aliases = [
>      "blah=qemu://invaliduri@@@",
>  ]
>
>Results in a double free when the alias is used:
>
>  $ virsh -c blah
>  free(): double free detected in tcache 2
>  Aborted (core dumped)
>
>This happens as the 'alias' variable is first assigned to 'uristr' which
>is cleared in the 'failed' label and then is explicitly freed again.
>
>Fix this by switching to 'g_autofree' for alias and stealing it into
>'uristr'.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/libvirt.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
>diff --git a/src/libvirt.c b/src/libvirt.c
>index b78b49a632..7e7c0fc8e3 100644
>--- a/src/libvirt.c
>+++ b/src/libvirt.c
>@@ -933,21 +933,19 @@ virConnectOpenInternal(const char *name,
>     }
>
>     if (uristr) {
>-        char *alias = NULL;
>+        g_autofree char *alias = NULL;
>

Is this necessary? In the case 'alias' will be assigned a value,
we will steal the value right away.

>         if (!(flags & VIR_CONNECT_NO_ALIASES) &&
>             virURIResolveAlias(conf, uristr, &alias) < 0)
>             goto failed;
>
>         if (alias) {
>-            VIR_FREE(uristr);
>-            uristr = alias;
>+            g_free(uristr);
>+            uristr = g_steal_pointer(&alias);
>         }
>
>-        if (!(ret->uri = virURIParse(uristr))) {
>-            VIR_FREE(alias);
>+        if (!(ret->uri = virURIParse(uristr)))
>             goto failed;
>-        }

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano