[libvirt] [PATCH 0/3] vz: util: some crash fixes
Maxim Nestratov (3): util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects vz: fix crash when parsing unexpected disk configuration util: fix potential crash in virDiskNameParse src/util/virclosecallbacks.c | 2 ++ src/util/virutil.c | 3 +++ src/vz/vz_sdk.c | 3 +++ 3 files changed, 8 insertions(+) -- 1.8.3.1
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 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..2fab56b 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); @@ -177,6 +178,7 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, goto cleanup; virObjectUnref(vm); + virObjectUnref(closeCallbacks); ret = 0; cleanup: virObjectUnlock(closeCallbacks); -- 1.8.3.1
On Mon, Jun 06, 2016 at 20:59:22 +0300, 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.
Do you happen to have any kind of reproducer for this crash?
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 | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..2fab56b 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); @@ -177,6 +178,7 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, goto cleanup;
virObjectUnref(vm); + virObjectUnref(closeCallbacks);
If this cleared the last reference, closeCallbacks is invalid at that point.
ret = 0; cleanup: virObjectUnlock(closeCallbacks);
But this would dereference and use it. Peter
09.06.2016 15:03, Peter Krempa пишет:
On Mon, Jun 06, 2016 at 20:59:22 +0300, 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. Do you happen to have any kind of reproducer for this crash?
Unfortunately no.
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 | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..2fab56b 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); @@ -177,6 +178,7 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, goto cleanup;
virObjectUnref(vm); + virObjectUnref(closeCallbacks); If this cleared the last reference, closeCallbacks is invalid at that point.
Hm, yes, you are right. Actually I assumed that locking an object guarantees the user of the object its validity. So I really thought that locking function takes a reference and unlocking releases it. And it turned out not to be true. Effectively it's an error prone way of dealing with locking/unlocking functions. Am I the only one who finds it a bit problematic? Or is there some cases when you need to lock objects and not to reference them that I don't see?
ret = 0; cleanup: virObjectUnlock(closeCallbacks);
But this would dereference and use it.
Peter
09.06.2016 15:23, Maxim Nestratov пишет:
09.06.2016 15:03, Peter Krempa пишет:
On Mon, Jun 06, 2016 at 20:59:22 +0300, 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. Do you happen to have any kind of reproducer for this crash?
Unfortunately no.
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 | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 82d4071..2fab56b 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); @@ -177,6 +178,7 @@ virCloseCallbacksUnset(virCloseCallbacksPtr closeCallbacks, goto cleanup; virObjectUnref(vm); + virObjectUnref(closeCallbacks); If this cleared the last reference, closeCallbacks is invalid at that point.
Hm, yes, you are right. Actually I assumed that locking an object guarantees the user of the object its validity. So I really thought that locking function takes a reference and unlocking releases it. And it turned out not to be true. Effectively it's an error prone way of dealing with locking/unlocking functions. Am I the only one who finds it a bit problematic? Or is there some cases when you need to lock objects and not to reference them that I don't see?
Disregard please. It's true only if you get a locked object from a list or by some getter function, otherwise a caller should guarantee validity of the object, which is untrue in my case and I'm trying to fix it (eventually incorrectly). I'll resend another version.
ret = 0; cleanup: virObjectUnlock(closeCallbacks);
But this would dereference and use it.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
As it turned out PrlVmDev_GetStackIndex can return negative values without reporting an error, which is incorrect but nevertheless. After that we feed this negative index to virIndexToDiskName, which in turn returns NULL and we set it to virDomainDiskDef.dst. Using virDiskNameToBusDeviceIndex with a virDomainDiskDef structure which has NULL dst field crashes. Fix this by returning an error in prlsdkGetDiskId in such cases. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7eb78ca..947db3a 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -543,6 +543,9 @@ prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, char **dst) return -1; } + if (NULL == *dst) + return -1; + return 0; } -- 1.8.3.1
On 06.06.2016 20:59, Maxim Nestratov wrote:
As it turned out PrlVmDev_GetStackIndex can return negative values without reporting an error, which is incorrect but nevertheless. After that we feed this negative index to virIndexToDiskName, which in turn returns NULL and we set it to virDomainDiskDef.dst. Using virDiskNameToBusDeviceIndex with a virDomainDiskDef structure which has NULL dst field crashes. Fix this by returning an error in prlsdkGetDiskId in such cases.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7eb78ca..947db3a 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -543,6 +543,9 @@ prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, char **dst) return -1; }
+ if (NULL == *dst) + return -1; + return 0; }
ACK
07.06.2016 9:24, Nikolay Shirokovskiy пишет:
On 06.06.2016 20:59, Maxim Nestratov wrote:
As it turned out PrlVmDev_GetStackIndex can return negative values without reporting an error, which is incorrect but nevertheless. After that we feed this negative index to virIndexToDiskName, which in turn returns NULL and we set it to virDomainDiskDef.dst. Using virDiskNameToBusDeviceIndex with a virDomainDiskDef structure which has NULL dst field crashes. Fix this by returning an error in prlsdkGetDiskId in such cases.
Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/vz/vz_sdk.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 7eb78ca..947db3a 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -543,6 +543,9 @@ prlsdkGetDiskId(PRL_HANDLE disk, bool isCt, int *bus, char **dst) return -1; }
+ if (NULL == *dst) + return -1; + return 0; }
ACK
Pushed now. Thanks. Maxim
As far as virDiskNameToIndex calls virDiskNameParse and doesn't check parameters and in most cases is used with disk->dst as a parameter and disk->dst is by virIndexToDiskName, which can return NULL, virDiskNameParse can crash in such cases. Let's be paranoic and sustain even such incorrect usage. Signed-off-by: Maxim Nestratov <mnestratov@virtuozzo.com> --- src/util/virutil.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/util/virutil.c b/src/util/virutil.c index d80d994..05e136d 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -556,6 +556,9 @@ int virDiskNameParse(const char *name, int *disk, int *partition) static char const* const drive_prefix[] = {"fd", "hd", "vd", "sd", "xvd", "ubd"}; size_t i; + if (!name) + return -1; + for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) { if (STRPREFIX(name, drive_prefix[i])) { ptr = name + strlen(drive_prefix[i]); -- 1.8.3.1
On Mon, Jun 06, 2016 at 20:59:24 +0300, Maxim Nestratov wrote:
As far as virDiskNameToIndex calls virDiskNameParse and doesn't check parameters and in most cases is used with disk->dst as a parameter and disk->dst is by virIndexToDiskName, which can return NULL, virDiskNameParse can crash in such cases. Let's be paranoic and sustain even such incorrect usage.
We expect disk->dst to be assigned all the time in more than just this code thus I don't think adding this makes any sense. Peter
09.06.2016 14:58, Peter Krempa пишет:
On Mon, Jun 06, 2016 at 20:59:24 +0300, Maxim Nestratov wrote:
As far as virDiskNameToIndex calls virDiskNameParse and doesn't check parameters and in most cases is used with disk->dst as a parameter and disk->dst is by virIndexToDiskName, which can return NULL, virDiskNameParse can crash in such cases. Let's be paranoic and sustain even such incorrect usage. We expect disk->dst to be assigned all the time in more than just this code thus I don't think adding this makes any sense.
Peter
Ok. Up to you. Maxim
participants (3)
-
Maxim Nestratov -
Nikolay Shirokovskiy -
Peter Krempa