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