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
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; > } >
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
© 2016 - 2024 Red Hat, Inc.