Re: [libvirt] [PATCH] util: fix crash in virClassIsDerivedFrom for CloseCallbacks objects
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@(a)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(a)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