[PATCH 1/1] qemuProcessEventSubmit : fix potential use after free
Coverity scan reports use after free issue. In error case, don't free vm object as it will be unlocked+freed in the parent function like qemuProcessHandleReset(). Signed-off-by: Shaleen Bathla <shaleen.bathla@oracle.com> --- src/qemu/qemu_process.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9fc7eada5220..a4133b37cf22 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -287,7 +287,6 @@ qemuProcessEventSubmit(virDomainObj *vm, event->data = data; if (virThreadPoolSendJob(driver->workerPool, 0, event) < 0) { - virObjectUnref(vm); qemuProcessEventFree(event); } } -- 2.31.1
On Tue, Jan 10, 2023 at 11:12:55 +0530, Shaleen Bathla wrote:
Coverity scan reports use after free issue. In error case, don't free vm object as it will be unlocked+freed in the parent function like qemuProcessHandleReset().
This explanation doesn't make too much sense to me ...
Signed-off-by: Shaleen Bathla <shaleen.bathla@oracle.com> --- src/qemu/qemu_process.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9fc7eada5220..a4133b37cf22 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -287,7 +287,6 @@ qemuProcessEventSubmit(virDomainObj *vm, event->data = data;
if (virThreadPoolSendJob(driver->workerPool, 0, event) < 0) { - virObjectUnref(vm);
... this virObjectUnref() call here is to undo the virObjectRef() that was done couple lines above in case when we could not submit the event to the worker thread. There is no code in between where we'd unlock the VM. Could you elaborate a bit more how you see the bug happening? Ideally also attach the coverity report.
On Tue, Jan 10, 2023 at 09:00:42AM +0100, Peter Krempa wrote:
On Tue, Jan 10, 2023 at 11:12:55 +0530, Shaleen Bathla wrote:
Coverity scan reports use after free issue. In error case, don't free vm object as it will be unlocked+freed in the parent function like qemuProcessHandleReset().
This explanation doesn't make too much sense to me ...
Signed-off-by: Shaleen Bathla <shaleen.bathla@oracle.com> --- src/qemu/qemu_process.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9fc7eada5220..a4133b37cf22 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -287,7 +287,6 @@ qemuProcessEventSubmit(virDomainObj *vm, event->data = data;
if (virThreadPoolSendJob(driver->workerPool, 0, event) < 0) { - virObjectUnref(vm);
... this virObjectUnref() call here is to undo the virObjectRef() that was done couple lines above in case when we could not submit the event to the worker thread.
There is no code in between where we'd unlock the VM.
Could you elaborate a bit more how you see the bug happening? Ideally also attach the coverity report.
Looks like this was a false positive in coverity scan. CID in libvirt is 403592. I use gui coverity, not sure how to attach report. Should I attach screenshot? I think this can be eliminated if we use virObjectUnref(event->vm) instead. I can send a v2 patch with the above change if agreed? Regards, Shaleen Bathla
participants (2)
-
Peter Krempa -
Shaleen Bathla