[libvirt PATCH] qemu: fix release of virDomainObjPtr in SSH key APIs
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
participants (3)
-
Daniel Henrique Barboza -
Daniel P. Berrangé -
Michal Privoznik