On Wed, Jun 20, 2012 at 12:29:51PM +0200, Christophe Fergeau wrote:
The deletion of libvirt timeouts/watches is done in 2 steps: - the first step is synchronous and unregisters the timeout/watch from glib mainloop - the second step is asynchronous and triggered from the first step. It releases the memory used for bookkeeping for the timeout/watch being deleted
This is done this way to avoid some possible deadlocks when reentering the sync callback while freeing the memory associated with the timeout/watch.
However, it's possible to call gvir_event_update_handle after gvir_event_remove_handle but before _event_handle_remove does the final cleanup. When this happen, _update_handle will reregister the handle with glib mainloop, and bad things will happen when a glib callback is triggered for this event after _event_handle_remove has freed the memory associated with this handle watch.
That's clearly possible, but it is also illegal use of the events API by some client code, since the timer/watch id is not valid after calling remove(). Do you actually see this occurring in real life ?
This commit marks the timeouts and watches as removed in the synchronous _remove callback and makes sure removed timeouts/watches are ignored in _update callbacks. --- libvirt-glib/libvirt-glib-event.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/libvirt-glib/libvirt-glib-event.c b/libvirt-glib/libvirt-glib-event.c index 587a59e..657c1bf 100644 --- a/libvirt-glib/libvirt-glib-event.c +++ b/libvirt-glib/libvirt-glib-event.c @@ -87,7 +87,7 @@ struct gvir_event_handle int watch; int fd; int events; - int enabled; + int removed; GIOChannel *channel; guint source; virEventHandleCallback cb; @@ -99,6 +99,7 @@ struct gvir_event_timeout { int timer; int interval; + int removed; guint source; virEventTimeoutCallback cb; void *opaque; @@ -195,7 +196,7 @@ gvir_event_handle_find(int watch) continue; }
- if (h->watch == watch) { + if ((h->watch == watch) && !h->removed) { return h; } } @@ -289,6 +290,11 @@ gvir_event_handle_remove(int watch) g_source_remove(data->source); data->source = 0; data->events = 0; + /* since the actual watch deletion is done asynchronously, a handle_update call may + * reschedule the watch before it's fully deleted, that's why we need to mark it as + * 'removed' to prevent reuse + */ + data->removed = TRUE; g_idle_add(_event_handle_remove, data);
ret = 0; @@ -358,7 +364,7 @@ gvir_event_timeout_find(int timer) continue; }
- if (t->timer == timer) { + if ((t->timer == timer) && !t->removed) { return t; } } @@ -441,6 +447,11 @@ gvir_event_timeout_remove(int timer)
g_source_remove(data->source); data->source = 0; + /* since the actual timeout deletion is done asynchronously, a timeout_update call may + * reschedule the timeout before it's fully deleted, that's why we need to mark it as + * 'removed' to prevent reuse + */ + data->removed = TRUE; g_idle_add(_event_timeout_remove, data);
ret = 0;
ACK, because it doesn't hurt to be paranoid against badly written code. 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 :|