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); } Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|