On 03/07/2011 08:17 AM, Daniel P. Berrange wrote:
> On Mon, Mar 07, 2011 at 02:13:23PM +0100, Jiri Denemark wrote:
>> On Mon, Mar 07, 2011 at 11:15:38 +0000, Daniel P. Berrange wrote:
>>>> - if (eventLoop.handles[i].ff)
>>>> + if (eventLoop.handles[i].ff) {
>>>> + virMutexUnlock(&eventLoop.lock);
>>>> (eventLoop.handles[i].ff)(eventLoop.handles[i].opaque);
>>>> + virMutexLock(&eventLoop.lock);
>>>> + }
>>> I'm a little concerned as to whether the rest of the code in
>>> virEventCleanupHandles/CleanupTimeouts is safe, if we release
>>> the lock here. eg, if some other thread calls virEventAddTimeout
>>> of AddHandle, is there any way this could cause problems for us
>>> here. So far I think this is safe because AddTimeout/AddHandle
>>> will simply append to the end of the array we're iterating over,
>>> but would like a second opinion before ACK'ing
>> I don't think it's safe to unlock eventloop.lock even in the existing
>> Dispatch{Timeouts,Handles} cases because Add{Timeout,Handle} use realloc which
>> is allowed to allocate a new array, move the contents of the old one into it
>> and free the old array. So the for loop can easily end up accessing memory
>> which has already been freed.
> That's a very unlikely scenario, but yes it could happen. We'd need to
> save a copy of the row we're accessing. Thus instead of
>
> if (eventLoop.handles[i].ff) {
> virMutexUnlock(&eventLoop.lock);
> (eventLoop.handles[i].ff)(eventLoop.handles[i].opaque);
> virMutexLock(&eventLoop.lock);
> }
>
> We probably need
>
> if (eventLoop.handles[i].ff) {
> virFreeCallback ff = eventLoop.handles[i].ff;
> void *opaque = eventLoop.handles[i].opaque;
> virMutexUnlock(&eventLoop.lock);
> ff(opaque);
> virMutexLock(&eventLoop.lock);
> }
I tested this patch on the two cleanup functions with a save/restore of
both a transient and a persistent domain, and both completed without
problems.
Based on this, and Dan's explanation of the way that the eventLoop lock
*should* work, I can ACK a version of Wen Congyang's patch with Dan's
suggestion from above.