Patchwork [v2,4/4] qemu-char: do not operate on sources from finalize callbacks

login
register
mail settings
Submitter Paolo Bonzini
Date April 19, 2013, 3:32 p.m.
Message ID <1366385529-10329-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/238043/
State New
Headers show

Comments

Paolo Bonzini - April 19, 2013, 3:32 p.m.
Due to a glib bug, the finalize callback is called with the GMainContext
lock held.  Thus, any operation on the context from the callback will
cause recursive locking and a deadlock.  This happens, for example,
when a client disconnects from a socket chardev.

The fix for this is somewhat ugly, because we need to forego polymorphism
and implement our own function to destroy IOWatchPoll sources.  The
right thing to do here would be child sources, but we support older
glib versions that do not have them.  Not coincidentially, glib developers
found and fixed the deadlock as part of implementing child sources.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-char.c |   55 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 40 insertions(+), 15 deletions(-)
Sander Eikelenboom - April 19, 2013, 5:12 p.m.
Friday, April 19, 2013, 5:32:09 PM, you wrote:

> Due to a glib bug, the finalize callback is called with the GMainContext
> lock held.  Thus, any operation on the context from the callback will
> cause recursive locking and a deadlock.  This happens, for example,
> when a client disconnects from a socket chardev.

> The fix for this is somewhat ugly, because we need to forego polymorphism
> and implement our own function to destroy IOWatchPoll sources.  The
> right thing to do here would be child sources, but we support older
> glib versions that do not have them.  Not coincidentially, glib developers
> found and fixed the deadlock as part of implementing child sources.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Tested-by: Sander Eikelenboom <linux@eikelenboom.it>

> ---
>  qemu-char.c |   55 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 40 insertions(+), 15 deletions(-)

> diff --git a/qemu-char.c b/qemu-char.c
> index 6e897da..f29f9b1 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -643,12 +643,18 @@ static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
>  
>  static void io_watch_poll_finalize(GSource *source)
>  {
> +    /* Due to a glib bug, removing the last reference to a source
> +     * inside a finalize callback causes recursive locking (and a
> +     * deadlock).  This is not a problem inside other callbacks,
> +     * including dispatch callbacks, so we call io_remove_watch_poll
> +     * to remove this source.  At this point, iwp->src must
> +     * be NULL, or we would leak it.
> +     *
> +     * This would be solved much more elegantly by child sources,
> +     * but we support older glib versions that do not have them.
> +     */
>      IOWatchPoll *iwp = io_watch_poll_from_source(source);
-    if (iwp->>src) {
> -        g_source_destroy(iwp->src);
> -        g_source_unref(iwp->src);
-        iwp->>src = NULL;
> -    }
+    assert(iwp->>src == NULL);
>  }
>  
>  static GSourceFuncs io_watch_poll_funcs = {
> @@ -679,6 +685,25 @@ static guint io_add_watch_poll(GIOChannel *channel,
>      return tag;
>  }
>  
> +static void io_remove_watch_poll(guint tag)
> +{
> +    GSource *source;
> +    IOWatchPoll *iwp;
> +
> +    g_return_if_fail (tag > 0);
> +
> +    source = g_main_context_find_source_by_id(NULL, tag);
> +    g_return_if_fail (source != NULL);
> +
> +    iwp = io_watch_poll_from_source(source);
+    if (iwp->>src) {
> +        g_source_destroy(iwp->src);
> +        g_source_unref(iwp->src);
+        iwp->>src = NULL;
> +    }
> +    g_source_destroy(&iwp->parent);
> +}
> +
>  #ifndef _WIN32
>  static GIOChannel *io_channel_from_fd(int fd)
>  {
> @@ -788,7 +813,7 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>                                       len, &bytes_read, NULL);
>      if (status == G_IO_STATUS_EOF) {
>          if (s->fd_in_tag) {
> -            g_source_remove(s->fd_in_tag);
> +            io_remove_watch_poll(s->fd_in_tag);
>              s->fd_in_tag = 0;
>          }
>          qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> @@ -821,7 +846,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
>      FDCharDriver *s = chr->opaque;
>  
>      if (s->fd_in_tag) {
> -        g_source_remove(s->fd_in_tag);
> +        io_remove_watch_poll(s->fd_in_tag);
>          s->fd_in_tag = 0;
>      }
>  
> @@ -835,7 +860,7 @@ static void fd_chr_close(struct CharDriverState *chr)
>      FDCharDriver *s = chr->opaque;
>  
>      if (s->fd_in_tag) {
> -        g_source_remove(s->fd_in_tag);
> +        io_remove_watch_poll(s->fd_in_tag);
>          s->fd_in_tag = 0;
>      }
>  
> @@ -1145,7 +1170,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
>  
>      if (!connected) {
>          if (s->fd_tag) {
> -            g_source_remove(s->fd_tag);
> +            io_remove_watch_poll(s->fd_tag);
>              s->fd_tag = 0;
>          }
>          s->connected = 0;
> @@ -1173,7 +1198,7 @@ static void pty_chr_close(struct CharDriverState *chr)
>      int fd;
>  
>      if (s->fd_tag) {
> -        g_source_remove(s->fd_tag);
> +        io_remove_watch_poll(s->fd_tag);
>          s->fd_tag = 0;
>      }
>      fd = g_io_channel_unix_get_fd(s->fd);
> @@ -2252,7 +2277,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>      s->bufptr = s->bufcnt;
>      if (status != G_IO_STATUS_NORMAL) {
>          if (s->tag) {
> -            g_source_remove(s->tag);
> +            io_remove_watch_poll(s->tag);
>              s->tag = 0;
>          }
>          return FALSE;
> @@ -2273,7 +2298,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
>      NetCharDriver *s = chr->opaque;
>  
>      if (s->tag) {
> -        g_source_remove(s->tag);
> +        io_remove_watch_poll(s->tag);
>          s->tag = 0;
>      }
>  
> @@ -2286,7 +2311,7 @@ static void udp_chr_close(CharDriverState *chr)
>  {
>      NetCharDriver *s = chr->opaque;
>      if (s->tag) {
> -        g_source_remove(s->tag);
> +        io_remove_watch_poll(s->tag);
>          s->tag = 0;
>      }
>      if (s->chan) {
> @@ -2520,7 +2545,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
>              s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
>          }
>          if (s->tag) {
> -            g_source_remove(s->tag);
> +            io_remove_watch_poll(s->tag);
>              s->tag = 0;
>          }
>          g_io_channel_unref(s->chan);
> @@ -2635,7 +2660,7 @@ static void tcp_chr_close(CharDriverState *chr)
>      TCPCharDriver *s = chr->opaque;
>      if (s->fd >= 0) {
>          if (s->tag) {
> -            g_source_remove(s->tag);
> +            io_remove_watch_poll(s->tag);
>              s->tag = 0;
>          }
>          if (s->chan) {

Patch

diff --git a/qemu-char.c b/qemu-char.c
index 6e897da..f29f9b1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -643,12 +643,18 @@  static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
 
 static void io_watch_poll_finalize(GSource *source)
 {
+    /* Due to a glib bug, removing the last reference to a source
+     * inside a finalize callback causes recursive locking (and a
+     * deadlock).  This is not a problem inside other callbacks,
+     * including dispatch callbacks, so we call io_remove_watch_poll
+     * to remove this source.  At this point, iwp->src must
+     * be NULL, or we would leak it.
+     *
+     * This would be solved much more elegantly by child sources,
+     * but we support older glib versions that do not have them.
+     */
     IOWatchPoll *iwp = io_watch_poll_from_source(source);
-    if (iwp->src) {
-        g_source_destroy(iwp->src);
-        g_source_unref(iwp->src);
-        iwp->src = NULL;
-    }
+    assert(iwp->src == NULL);
 }
 
 static GSourceFuncs io_watch_poll_funcs = {
@@ -679,6 +685,25 @@  static guint io_add_watch_poll(GIOChannel *channel,
     return tag;
 }
 
+static void io_remove_watch_poll(guint tag)
+{
+    GSource *source;
+    IOWatchPoll *iwp;
+
+    g_return_if_fail (tag > 0);
+
+    source = g_main_context_find_source_by_id(NULL, tag);
+    g_return_if_fail (source != NULL);
+
+    iwp = io_watch_poll_from_source(source);
+    if (iwp->src) {
+        g_source_destroy(iwp->src);
+        g_source_unref(iwp->src);
+        iwp->src = NULL;
+    }
+    g_source_destroy(&iwp->parent);
+}
+
 #ifndef _WIN32
 static GIOChannel *io_channel_from_fd(int fd)
 {
@@ -788,7 +813,7 @@  static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
                                      len, &bytes_read, NULL);
     if (status == G_IO_STATUS_EOF) {
         if (s->fd_in_tag) {
-            g_source_remove(s->fd_in_tag);
+            io_remove_watch_poll(s->fd_in_tag);
             s->fd_in_tag = 0;
         }
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
@@ -821,7 +846,7 @@  static void fd_chr_update_read_handler(CharDriverState *chr)
     FDCharDriver *s = chr->opaque;
 
     if (s->fd_in_tag) {
-        g_source_remove(s->fd_in_tag);
+        io_remove_watch_poll(s->fd_in_tag);
         s->fd_in_tag = 0;
     }
 
@@ -835,7 +860,7 @@  static void fd_chr_close(struct CharDriverState *chr)
     FDCharDriver *s = chr->opaque;
 
     if (s->fd_in_tag) {
-        g_source_remove(s->fd_in_tag);
+        io_remove_watch_poll(s->fd_in_tag);
         s->fd_in_tag = 0;
     }
 
@@ -1145,7 +1170,7 @@  static void pty_chr_state(CharDriverState *chr, int connected)
 
     if (!connected) {
         if (s->fd_tag) {
-            g_source_remove(s->fd_tag);
+            io_remove_watch_poll(s->fd_tag);
             s->fd_tag = 0;
         }
         s->connected = 0;
@@ -1173,7 +1198,7 @@  static void pty_chr_close(struct CharDriverState *chr)
     int fd;
 
     if (s->fd_tag) {
-        g_source_remove(s->fd_tag);
+        io_remove_watch_poll(s->fd_tag);
         s->fd_tag = 0;
     }
     fd = g_io_channel_unix_get_fd(s->fd);
@@ -2252,7 +2277,7 @@  static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     s->bufptr = s->bufcnt;
     if (status != G_IO_STATUS_NORMAL) {
         if (s->tag) {
-            g_source_remove(s->tag);
+            io_remove_watch_poll(s->tag);
             s->tag = 0;
         }
         return FALSE;
@@ -2273,7 +2298,7 @@  static void udp_chr_update_read_handler(CharDriverState *chr)
     NetCharDriver *s = chr->opaque;
 
     if (s->tag) {
-        g_source_remove(s->tag);
+        io_remove_watch_poll(s->tag);
         s->tag = 0;
     }
 
@@ -2286,7 +2311,7 @@  static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
     if (s->tag) {
-        g_source_remove(s->tag);
+        io_remove_watch_poll(s->tag);
         s->tag = 0;
     }
     if (s->chan) {
@@ -2520,7 +2545,7 @@  static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
             s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
         }
         if (s->tag) {
-            g_source_remove(s->tag);
+            io_remove_watch_poll(s->tag);
             s->tag = 0;
         }
         g_io_channel_unref(s->chan);
@@ -2635,7 +2660,7 @@  static void tcp_chr_close(CharDriverState *chr)
     TCPCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
         if (s->tag) {
-            g_source_remove(s->tag);
+            io_remove_watch_poll(s->tag);
             s->tag = 0;
         }
         if (s->chan) {