[libvirt] [PATCH] util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects
There is a possibility that qemu driver frees by unreferencing its closeCallbacks pointer as it has the only reference to the object, while in fact not all users of CloseCallbacks called thier virCloseCallbacksUnset. Backtrace is the following: Thread #1: 0 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 1 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 2 in virThreadPoolFree (pool=0x7f0810110b50) at util/virthreadpool.c:266 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 4 in virStateCleanup () at libvirt.c:808 5 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1660 Thread #2: 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at util/virobject.c:169 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=<optimized out>) at util/virobject.c:365 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 4 in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5854 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at qemu/qemu_driver.c:4585 7 qemuProcessEventHandler (data=<optimized out>, opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at util/virthreadpool.c:145 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 10 in start_thread () from /lib64/libpthread.so.0 Let's reference CloseCallbacks object in virCloseCallbacksSet and unreference in virCloseCallbacksUnset. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/util/virclosecallbacks.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..891a92b 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, virObjectRef(vm); } + virObjectRef(closeCallbacks); ret = 0; cleanup: virObjectUnlock(closeCallbacks); @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, ret = 0; cleanup: virObjectUnlock(closeCallbacks); + if (!ret) + virObjectUnref(closeCallbacks); return ret; } -- 2.4.11
On 05.09.2016 18:42, Maxim Nestratov wrote:
There is a possibility that qemu driver frees by unreferencing its closeCallbacks pointer as it has the only reference to the object, while in fact not all users of CloseCallbacks called thier virCloseCallbacksUnset.
Backtrace is the following: Thread #1: 0 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 1 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 2 in virThreadPoolFree (pool=0x7f0810110b50) at util/virthreadpool.c:266 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 4 in virStateCleanup () at libvirt.c:808 5 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1660
Thread #2: 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at util/virobject.c:169 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=<optimized out>) at util/virobject.c:365 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 4 in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5854 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at qemu/qemu_driver.c:4585 7 qemuProcessEventHandler (data=<optimized out>, opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at util/virthreadpool.c:145 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 10 in start_thread () from /lib64/libpthread.so.0
Let's reference CloseCallbacks object in virCloseCallbacksSet and unreference in virCloseCallbacksUnset.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/util/virclosecallbacks.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..891a92b 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, virObjectRef(vm); }
+ virObjectRef(closeCallbacks); ret = 0; cleanup: virObjectUnlock(closeCallbacks); @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, ret = 0; cleanup: virObjectUnlock(closeCallbacks); + if (!ret) + virObjectUnref(closeCallbacks);
Might as well put this a few lines above, just before 'ret = 0' line. ACK with that changed. Michal
06-Sep-16 17:42, Michal Privoznik пишет:
On 05.09.2016 18:42, Maxim Nestratov wrote:
There is a possibility that qemu driver frees by unreferencing its closeCallbacks pointer as it has the only reference to the object, while in fact not all users of CloseCallbacks called thier virCloseCallbacksUnset.
Backtrace is the following: Thread #1: 0 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 1 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 2 in virThreadPoolFree (pool=0x7f0810110b50) at util/virthreadpool.c:266 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 4 in virStateCleanup () at libvirt.c:808 5 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1660
Thread #2: 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at util/virobject.c:169 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=<optimized out>) at util/virobject.c:365 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 4 in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5854 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at qemu/qemu_driver.c:4585 7 qemuProcessEventHandler (data=<optimized out>, opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at util/virthreadpool.c:145 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 10 in start_thread () from /lib64/libpthread.so.0
Let's reference CloseCallbacks object in virCloseCallbacksSet and unreference in virCloseCallbacksUnset.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/util/virclosecallbacks.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..891a92b 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, virObjectRef(vm); }
+ virObjectRef(closeCallbacks); ret = 0; cleanup: virObjectUnlock(closeCallbacks); @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, ret = 0; cleanup: virObjectUnlock(closeCallbacks); + if (!ret) + virObjectUnref(closeCallbacks); Might as well put this a few lines above, just before 'ret = 0' line.
ACK with that changed.
Michal
No, we can't do this, as it could be the last reference to closeCallbacks and after derefencing it, virObjectUnlock would touch freed memory. Maxim
On 07.09.2016 11:41, Maxim Nestratov wrote:
06-Sep-16 17:42, Michal Privoznik пишет:
On 05.09.2016 18:42, Maxim Nestratov wrote:
There is a possibility that qemu driver frees by unreferencing its closeCallbacks pointer as it has the only reference to the object, while in fact not all users of CloseCallbacks called thier virCloseCallbacksUnset.
Backtrace is the following: Thread #1: 0 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 1 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 2 in virThreadPoolFree (pool=0x7f0810110b50) at util/virthreadpool.c:266 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 4 in virStateCleanup () at libvirt.c:808 5 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1660
Thread #2: 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at util/virobject.c:169 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=<optimized out>) at util/virobject.c:365 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 4 in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5854 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at qemu/qemu_driver.c:4585 7 qemuProcessEventHandler (data=<optimized out>, opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at util/virthreadpool.c:145 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 10 in start_thread () from /lib64/libpthread.so.0
Let's reference CloseCallbacks object in virCloseCallbacksSet and unreference in virCloseCallbacksUnset.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/util/virclosecallbacks.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..891a92b 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, virObjectRef(vm); } + virObjectRef(closeCallbacks); ret = 0; cleanup: virObjectUnlock(closeCallbacks); @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, ret = 0; cleanup: virObjectUnlock(closeCallbacks); + if (!ret) + virObjectUnref(closeCallbacks); Might as well put this a few lines above, just before 'ret = 0' line.
ACK with that changed.
Michal
No, we can't do this, as it could be the last reference to closeCallbacks and after derefencing it, virObjectUnlock would touch freed memory.
Ah, good point. Stick to the original then. Michal
07-Sep-16 12:45, Michal Privoznik пишет:
On 07.09.2016 11:41, Maxim Nestratov wrote:
06-Sep-16 17:42, Michal Privoznik пишет:
On 05.09.2016 18:42, Maxim Nestratov wrote:
There is a possibility that qemu driver frees by unreferencing its closeCallbacks pointer as it has the only reference to the object, while in fact not all users of CloseCallbacks called thier virCloseCallbacksUnset.
Backtrace is the following: Thread #1: 0 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 1 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 2 in virThreadPoolFree (pool=0x7f0810110b50) at util/virthreadpool.c:266 3 in qemuStateCleanup () at qemu/qemu_driver.c:1116 4 in virStateCleanup () at libvirt.c:808 5 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1660
Thread #2: 0 in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x7f0837c694d0) at util/virobject.c:169 1 in virObjectIsClass (anyobj=anyobj@entry=0x7f08101d4760, klass=<optimized out>) at util/virobject.c:365 2 in virObjectLock (anyobj=0x7f08101d4760) at util/virobject.c:317 3 in virCloseCallbacksUnset (closeCallbacks=0x7f08101d4760, vm=vm@entry=0x7f08101d47b0, cb=cb@entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163 4 in qemuProcessAutoDestroyRemove (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0) at qemu/qemu_process.c:6368 5 in qemuProcessStop (driver=driver@entry=0x7f081018be50, vm=vm@entry=0x7f08101d47b0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE, flags=flags@entry=0) at qemu/qemu_process.c:5854 6 in processMonitorEOFEvent (vm=0x7f08101d47b0, driver=0x7f081018be50) at qemu/qemu_driver.c:4585 7 qemuProcessEventHandler (data=<optimized out>, opaque=0x7f081018be50) at qemu/qemu_driver.c:4629 8 in virThreadPoolWorker (opaque=opaque@entry=0x7f0837c4f820) at util/virthreadpool.c:145 9 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 10 in start_thread () from /lib64/libpthread.so.0
Let's reference CloseCallbacks object in virCloseCallbacksSet and unreference in virCloseCallbacksUnset.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/util/virclosecallbacks.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..891a92b 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -141,6 +141,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr closeCallbacks, virObjectRef(vm); } + virObjectRef(closeCallbacks); ret = 0; cleanup: virObjectUnlock(closeCallbacks); @@ -180,6 +181,8 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, ret = 0; cleanup: virObjectUnlock(closeCallbacks); + if (!ret) + virObjectUnref(closeCallbacks); Might as well put this a few lines above, just before 'ret = 0' line.
ACK with that changed.
Michal No, we can't do this, as it could be the last reference to closeCallbacks and after derefencing it, virObjectUnlock would touch freed memory.
Ah, good point. Stick to the original then.
Michal
Pushed now. Thanks. Maxim
participants (2)
-
Maxim Nestratov -
Michal Privoznik