[libvirt] [libvirt-glib 5/5] mainloop: don't reschedule deleted timeouts/watches

[Daniel P. Berrange]

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 :|