[libvirt] [PATCH 1/3] util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects

[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 at 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 at entry=0x7f08101d47b0, cb=cb at entry=0x7f081d078fc0 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:163
>> 4  in qemuProcessAutoDestroyRemove (driver=driver at entry=0x7f081018be50, vm=vm at entry=0x7f08101d47b0) at qemu/qemu_process.c:6368
>> 5  in qemuProcessStop (driver=driver at entry=0x7f081018be50, vm=vm at entry=0x7f08101d47b0, reason=reason at entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN, asyncJob=asyncJob at entry=QEMU_ASYNC_JOB_NONE, flags=flags at 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 at 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 at 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