[libvirt PATCH] qemu: fix release of virDomainObjPtr in SSH key APIs

Daniel P. Berrangé posted 1 patch 3 years, 2 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210122170835.3522453-1-berrange@redhat.com
src/qemu/qemu_driver.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
[libvirt PATCH] qemu: fix release of virDomainObjPtr in SSH key APIs
Posted by Daniel P. Berrangé 3 years, 2 months ago
The qemuDomainObjFromDomain() API must be paired with
the virDomainObjEndAPI API. The qemuDomainAuthorizedSSHKeysGet
method simply did 'return -1' leaking a reference in two paths.

The qemuDomainAuthorizedSSHKeysSet method marked the object
as an autoptr while also have some code paths that will call
virDomainObjEndAPI, resulting in attempted double free.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/qemu/qemu_driver.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 027617deef..05e021cce4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20254,10 +20254,10 @@ qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom,
         return -1;
 
     if (virDomainAuthorizedSshKeysGetEnsureACL(dom->conn, vm->def) < 0)
-        return -1;
+        goto cleanup;
 
     if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
-        return -1;
+        goto cleanup;
 
     if (!qemuDomainAgentAvailable(vm, true))
         goto endagentjob;
@@ -20268,6 +20268,7 @@ qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom,
 
  endagentjob:
     qemuDomainObjEndAgentJob(vm);
+ cleanup:
     virDomainObjEndAPI(&vm);
     return rv;
 }
@@ -20281,7 +20282,7 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
                                unsigned int flags)
 {
     virQEMUDriverPtr driver = dom->conn->privateData;
-    g_autoptr(virDomainObj) vm = NULL;
+    virDomainObjPtr vm = NULL;
     qemuAgentPtr agent;
     const bool append = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND;
     const bool remove = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE;
@@ -20294,10 +20295,10 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
         return -1;
 
     if (virDomainAuthorizedSshKeysSetEnsureACL(dom->conn, vm->def) < 0)
-        return -1;
+        goto cleanup;
 
     if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
-        return -1;
+        goto cleanup;
 
     if (!qemuDomainAgentAvailable(vm, true))
         goto endagentjob;
@@ -20311,6 +20312,7 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
 
  endagentjob:
     qemuDomainObjEndAgentJob(vm);
+ cleanup:
     virDomainObjEndAPI(&vm);
     return rv;
 }
-- 
2.29.2

Re: [libvirt PATCH] qemu: fix release of virDomainObjPtr in SSH key APIs
Posted by Daniel Henrique Barboza 3 years, 2 months ago

On 1/22/21 2:08 PM, Daniel P. Berrangé wrote:
> The qemuDomainObjFromDomain() API must be paired with
> the virDomainObjEndAPI API. The qemuDomainAuthorizedSSHKeysGet
> method simply did 'return -1' leaking a reference in two paths.
> 
> The qemuDomainAuthorizedSSHKeysSet method marked the object
> as an autoptr while also have some code paths that will call
> virDomainObjEndAPI, resulting in attempted double free.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

>   src/qemu/qemu_driver.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 027617deef..05e021cce4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20254,10 +20254,10 @@ qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom,
>           return -1;
>   
>       if (virDomainAuthorizedSshKeysGetEnsureACL(dom->conn, vm->def) < 0)
> -        return -1;
> +        goto cleanup;
>   
>       if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
> -        return -1;
> +        goto cleanup;
>   
>       if (!qemuDomainAgentAvailable(vm, true))
>           goto endagentjob;
> @@ -20268,6 +20268,7 @@ qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom,
>   
>    endagentjob:
>       qemuDomainObjEndAgentJob(vm);
> + cleanup:
>       virDomainObjEndAPI(&vm);
>       return rv;
>   }
> @@ -20281,7 +20282,7 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
>                                  unsigned int flags)
>   {
>       virQEMUDriverPtr driver = dom->conn->privateData;
> -    g_autoptr(virDomainObj) vm = NULL;
> +    virDomainObjPtr vm = NULL;
>       qemuAgentPtr agent;
>       const bool append = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_APPEND;
>       const bool remove = flags & VIR_DOMAIN_AUTHORIZED_SSH_KEYS_SET_REMOVE;
> @@ -20294,10 +20295,10 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
>           return -1;
>   
>       if (virDomainAuthorizedSshKeysSetEnsureACL(dom->conn, vm->def) < 0)
> -        return -1;
> +        goto cleanup;
>   
>       if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
> -        return -1;
> +        goto cleanup;
>   
>       if (!qemuDomainAgentAvailable(vm, true))
>           goto endagentjob;
> @@ -20311,6 +20312,7 @@ qemuDomainAuthorizedSSHKeysSet(virDomainPtr dom,
>   
>    endagentjob:
>       qemuDomainObjEndAgentJob(vm);
> + cleanup:
>       virDomainObjEndAPI(&vm);
>       return rv;
>   }
> 

Re: [libvirt PATCH] qemu: fix release of virDomainObjPtr in SSH key APIs
Posted by Michal Privoznik 3 years, 2 months ago
On 1/22/21 6:08 PM, Daniel P. Berrangé wrote:
> The qemuDomainObjFromDomain() API must be paired with
> the virDomainObjEndAPI API. The qemuDomainAuthorizedSSHKeysGet
> method simply did 'return -1' leaking a reference in two paths.
> 
> The qemuDomainAuthorizedSSHKeysSet method marked the object
> as an autoptr while also have some code paths that will call
> virDomainObjEndAPI, resulting in attempted double free.

I think it's not a reference that's leaking, but a lock. 
qemuDomainObjFromDomain() returns a dom object that was locked and 
ref'ed. The g_autoptr() handles the unrefing but not unlocking.

Also, because virDomainObjEndAPI() clears out the pointer, the autounref 
callback does nothing, so there shouldn't be double free.

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > ---
>   src/qemu/qemu_driver.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 027617deef..05e021cce4 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20254,10 +20254,10 @@ qemuDomainAuthorizedSSHKeysGet(virDomainPtr dom,
>           return -1;
>   
>       if (virDomainAuthorizedSshKeysGetEnsureACL(dom->conn, vm->def) < 0)
> -        return -1;
> +        goto cleanup;
>   
>       if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0)
> -        return -1;
> +        goto cleanup;
>   

IOW, if either of these error conditions is hit then the domain is left 
locked. The patch is correct, but the error message is misleading.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal