[Qemu-devel] [PULL 2/5] qemu-char: fix deadlock with "-monitor pty"

[Paolo Bonzini]

qemu_chr_be_generic_open cannot be called with the write lock taken,
because it calls client code that may call qemu_chr_fe_write.  This
actually happens for the monitor:

    0x00007ffff27dbf79 in __GI_raise (address@hidden)
    0x00007ffff27df388 in __GI_abort ()
    0x00005555555ef489 in error_exit (err=<optimized out>, address@hidden 
<__func__.5959> "qemu_mutex_lock")
    0x00005555558f9080 in qemu_mutex_lock (address@hidden)
    0x0000555555713936 in qemu_chr_fe_write (s=0x555556248a30, address@hidden 
"QEMU 2.0.90 monitor - type 'help' for more information\r\n", len=56)
    0x00005555556217fd in monitor_flush_locked (address@hidden)
    0x0000555555621a12 in monitor_flush_locked (mon=0x555556251fd0)
    monitor_puts (address@hidden, str=0x55555634bfa7 "", address@hidden "QEMU 
2.0.90 monitor - type 'help' for more information\n")
    0x0000555555624359 in monitor_vprintf (mon=0x555556251fd0, fmt=<optimized 
out>, ap=<optimized out>)
    0x0000555555624414 in monitor_printf (mon=<optimized out>, address@hidden 
"QEMU %s monitor - type 'help' for more information\n")
    0x0000555555629806 in monitor_event (opaque=0x555556251fd0, 
event=<optimized out>)
    0x000055555571343c in qemu_chr_be_generic_open (s=0x555556248a30)

To avoid this, defer the call to an idle callback, which will be
called as soon as the main loop is re-entered.  In order to simplify
the cleanup and do it in one place only, change pty_chr_close to
call pty_chr_state.

To reproduce, run with "-monitor pty", then try to read from the
slave /dev/pts/FOO that it creates.

Fixes: 9005b2a7589540a3733b3abdcfbccfe7746cd1a1
Reported-by: Li Liang <address@hidden>
Reviewed-by: Fam Zheng <address@hidden>
Signed-off-by: Paolo Bonzini <address@hidden>
---
 qemu-char.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 55e372c..7acc03f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1089,6 +1089,7 @@ typedef struct {
     /* Protected by the CharDriverState chr_write_lock.  */
     int connected;
     guint timer_tag;
+    guint open_tag;
 } PtyCharDriver;
 
 static void pty_chr_update_read_handler_locked(CharDriverState *chr);
@@ -1101,6 +1102,7 @@ static gboolean pty_chr_timer(gpointer opaque)
 
     qemu_mutex_lock(&chr->chr_write_lock);
     s->timer_tag = 0;
+    s->open_tag = 0;
     if (!s->connected) {
         /* Next poll ... */
         pty_chr_update_read_handler_locked(chr);
@@ -1203,12 +1205,26 @@ static gboolean pty_chr_read(GIOChannel *chan, 
GIOCondition cond, void *opaque)
     return TRUE;
 }
 
+static gboolean qemu_chr_be_generic_open_func(gpointer opaque)
+{
+    CharDriverState *chr = opaque;
+    PtyCharDriver *s = chr->opaque;
+
+    s->open_tag = 0;
+    qemu_chr_be_generic_open(chr);
+    return FALSE;
+}
+
 /* Called with chr_write_lock held.  */
 static void pty_chr_state(CharDriverState *chr, int connected)
 {
     PtyCharDriver *s = chr->opaque;
 
     if (!connected) {
+        if (s->open_tag) {
+            g_source_remove(s->open_tag);
+            s->open_tag = 0;
+        }
         remove_fd_in_watch(chr);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
@@ -1221,8 +1237,9 @@ static void pty_chr_state(CharDriverState *chr, int 
connected)
             s->timer_tag = 0;
         }
         if (!s->connected) {
+            g_assert(s->open_tag == 0);
             s->connected = 1;
-            qemu_chr_be_generic_open(chr);
+            s->open_tag = g_idle_add(qemu_chr_be_generic_open_func, chr);
         }
         if (!chr->fd_in_tag) {
             chr->fd_in_tag = io_add_watch_poll(s->fd, pty_chr_read_poll,
@@ -1236,7 +1253,8 @@ static void pty_chr_close(struct CharDriverState *chr)
     PtyCharDriver *s = chr->opaque;
     int fd;
 
-    remove_fd_in_watch(chr);
+    qemu_mutex_lock(&chr->chr_write_lock);
+    pty_chr_state(chr, 0);
     fd = g_io_channel_unix_get_fd(s->fd);
     g_io_channel_unref(s->fd);
     close(fd);
@@ -1244,6 +1262,7 @@ static void pty_chr_close(struct CharDriverState *chr)
         g_source_remove(s->timer_tag);
         s->timer_tag = 0;
     }
+    qemu_mutex_unlock(&chr->chr_write_lock);
     g_free(s);
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
-- 
1.9.3