In the case of concurrent VM operations, it is possible to have a null pointer
reference in qemuMonitorOpen. In the case of concurrent VM shutdown, the
priv->monconf will be changed in qemuProcessStop. qemuMonitorOpen releases the
lock before calling qemuMonitorOpenUnix and then references priv->monconf. The
path variable in monconf will cause a null pointer, so it's better to back up the
path content in monconf before releasing the lock.

Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
---
 src/qemu/qemu_monitor.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index c2808c75a3..792b895570 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -679,6 +679,7 @@ qemuMonitorOpen(virDomainObj *vm,
 {
     VIR_AUTOCLOSE fd = -1;
     qemuMonitor *ret = NULL;
+    g_aurofree char *path = NULL;
 
     if (config->type != VIR_DOMAIN_CHR_TYPE_UNIX) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -687,8 +688,10 @@ qemuMonitorOpen(virDomainObj *vm,
         return NULL;
     }
 
+    path = g_strdup(config->data.nix.path);
+
     virObjectUnlock(vm);
-    fd = qemuMonitorOpenUnix(config->data.nix.path);
+    fd = qemuMonitorOpenUnix(path);
     virObjectLock(vm);
 
     if (fd < 0)
-- 
2.33.0

On 9/28/22 15:53, Jiang Jiacheng wrote:
> In the case of concurrent VM operations, it is possible to have a null pointer
> reference in qemuMonitorOpen. In the case of concurrent VM shutdown, the
> priv->monconf will be changed in qemuProcessStop. qemuMonitorOpen releases the
> lock before calling qemuMonitorOpenUnix and then references priv->monconf. The
> path variable in monconf will cause a null pointer, so it's better to back up the
> path content in monconf before releasing the lock.
> 
> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
> ---
>  src/qemu/qemu_monitor.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index c2808c75a3..792b895570 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -679,6 +679,7 @@ qemuMonitorOpen(virDomainObj *vm,
>  {
>      VIR_AUTOCLOSE fd = -1;
>      qemuMonitor *ret = NULL;
> +    g_aurofree char *path = NULL;
>  
>      if (config->type != VIR_DOMAIN_CHR_TYPE_UNIX) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -687,8 +688,10 @@ qemuMonitorOpen(virDomainObj *vm,
>          return NULL;
>      }
>  
> +    path = g_strdup(config->data.nix.path);
> +
>      virObjectUnlock(vm);
> -    fd = qemuMonitorOpenUnix(config->data.nix.path);
> +    fd = qemuMonitorOpenUnix(path);
>      virObjectLock(vm);
>  
>      if (fd < 0)

I'm failing to see how is this possible. Connecting to a monitor
inherently has a job, qemuProcessStop also has a job, inherently.

And even if there was a path, the @config is an object and all that
qemuProcessStop does is just unrefs it. Hence, a path string being freed
seems more like a refcounting issue to me. Thus, this looks more correct:


diff --git c/src/qemu/qemu_monitor.c w/src/qemu/qemu_monitor.c
index 5eba154d96..286f0ae7ff 100644
--- c/src/qemu/qemu_monitor.c
+++ w/src/qemu/qemu_monitor.c
@@ -687,9 +687,11 @@ qemuMonitorOpen(virDomainObj *vm,
         return NULL;
     }

+    virObjectRef(config);
     virObjectUnlock(vm);
     fd = qemuMonitorOpenUnix(config->data.nix.path);
     virObjectLock(vm);
+    virObjectUnref(config);

     if (fd < 0)
         return NULL;

Michal