[libvirt PATCH] qemu_domainjob: Make copy of owner API

Jiri Denemark 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/718bfef15fdf80b56a0b8f6e3f1370497a6f9309.1614191420.git.jdenemar@redhat.com
src/qemu/qemu_domainjob.c | 12 ++++++------
src/qemu/qemu_process.c   |  2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
[libvirt PATCH] qemu_domainjob: Make copy of owner API
Posted by Jiri Denemark 3 years, 2 months ago
Using the job owner API name directly works fine as long as it is a
static string or the owner's thread is still running. However, this is
not always the case. For example, when the owner API name is filled in a
job when we're reconnecting to existing domains after daemon restart,
the dynamically allocated owner name will disappear with the
reconnecting thread. Any follow up usage of the pointer will read random
memory.

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
---
 src/qemu/qemu_domainjob.c | 12 ++++++------
 src/qemu/qemu_process.c   |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
index 3c2c6b9179..b58d6837ad 100644
--- a/src/qemu/qemu_domainjob.c
+++ b/src/qemu/qemu_domainjob.c
@@ -190,7 +190,7 @@ qemuDomainObjResetJob(qemuDomainJobObjPtr job)
 {
     job->active = QEMU_JOB_NONE;
     job->owner = 0;
-    job->ownerAPI = NULL;
+    g_clear_pointer(&job->ownerAPI, g_free);
     job->started = 0;
 }
 
@@ -200,7 +200,7 @@ qemuDomainObjResetAgentJob(qemuDomainJobObjPtr job)
 {
     job->agentActive = QEMU_AGENT_JOB_NONE;
     job->agentOwner = 0;
-    job->agentOwnerAPI = NULL;
+    g_clear_pointer(&job->agentOwnerAPI, g_free);
     job->agentStarted = 0;
 }
 
@@ -210,7 +210,7 @@ qemuDomainObjResetAsyncJob(qemuDomainJobObjPtr job)
 {
     job->asyncJob = QEMU_ASYNC_JOB_NONE;
     job->asyncOwner = 0;
-    job->asyncOwnerAPI = NULL;
+    g_clear_pointer(&job->asyncOwnerAPI, g_free);
     job->asyncStarted = 0;
     job->phase = 0;
     job->mask = QEMU_JOB_DEFAULT_MASK;
@@ -890,7 +890,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
                       obj, obj->def->name);
             priv->job.active = job;
             priv->job.owner = virThreadSelfID();
-            priv->job.ownerAPI = virThreadJobGet();
+            priv->job.ownerAPI = g_strdup(virThreadJobGet());
             priv->job.started = now;
         } else {
             VIR_DEBUG("Started async job: %s (vm=%p name=%s)",
@@ -901,7 +901,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
             priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE;
             priv->job.asyncJob = asyncJob;
             priv->job.asyncOwner = virThreadSelfID();
-            priv->job.asyncOwnerAPI = virThreadJobGet();
+            priv->job.asyncOwnerAPI = g_strdup(virThreadJobGet());
             priv->job.asyncStarted = now;
             priv->job.current->started = now;
         }
@@ -917,7 +917,7 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
                   qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
         priv->job.agentActive = agentJob;
         priv->job.agentOwner = virThreadSelfID();
-        priv->job.agentOwnerAPI = virThreadJobGet();
+        priv->job.agentOwnerAPI = g_strdup(virThreadJobGet());
         priv->job.agentStarted = now;
     }
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index bfa742577f..398f63282e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3732,7 +3732,7 @@ qemuProcessRecoverJob(virQEMUDriverPtr driver,
         /* Restore the config of the async job which is not persisted */
         priv->jobs_queued++;
         priv->job.asyncJob = QEMU_ASYNC_JOB_BACKUP;
-        priv->job.asyncOwnerAPI = virThreadJobGet();
+        priv->job.asyncOwnerAPI = g_strdup(virThreadJobGet());
         priv->job.asyncStarted = now;
 
         qemuDomainObjSetAsyncJobMask(vm, (QEMU_JOB_DEFAULT_MASK |
-- 
2.30.0

Re: [libvirt PATCH] qemu_domainjob: Make copy of owner API
Posted by Peter Krempa 3 years, 2 months ago
On Wed, Feb 24, 2021 at 19:30:20 +0100, Jiri Denemark wrote:
> Using the job owner API name directly works fine as long as it is a
> static string or the owner's thread is still running. However, this is
> not always the case. For example, when the owner API name is filled in a
> job when we're reconnecting to existing domains after daemon restart,
> the dynamically allocated owner name will disappear with the
> reconnecting thread. Any follow up usage of the pointer will read random
> memory.
> 
> Signed-off-by: Jiri Denemark <jdenemar@redhat.com>
> ---
>  src/qemu/qemu_domainjob.c | 12 ++++++------
>  src/qemu/qemu_process.c   |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Peter Krempa <pkrempa@redhat.com>