Skip to contentSkip to conversation

char: fix segfault on chardev detach

PATCH Superseded Amit Shah 2 participants 6 messages Aug 28, 2013

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Amit Shah

author
#0 lore raw

v2 cover char: fix segfault on chardev detach

This series fixes a segfault when a frontend is detached from a chardev while the chardev had a pending callback registered. Further details in patch 3.

Please review.

v2:
* Move tag property to CharDriverState to simplify everything (Gerd)

Amit Shah (3):
char: move backends' io watch tag to CharDriverState
char: use common function to disable callbacks on chardev close
char: remove watch callback on chardev detach from frontend

 include/sysemu/char.h |  1 +
 qemu-char.c           | 82 +++++++++++++++++++--------------------------------
 2 files changed, 32 insertions(+), 51 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH v2 1/3 char: move backends' io watch tag to CharDriverState

All the backends implement an io watcher tag for callbacks. Move it to CharDriverState from each backend's struct to make accessing the tag from backend-neutral functions easier.

This will be used later to cancel a callback on chardev detach from a frontend.

CC <[email protected]>
Signed-off-by Amit Shah <[email protected]>
---
 include/sysemu/char.h |  1 +
 qemu-char.c           | 77 ++++++++++++++++++++++++++-------------------------
 2 files changed, 40 insertions(+), 38 deletions(-)
include/sysemu/char.h +1 -0
index 8053130..ad101d9 100644
@@ -78,6 +78,7 @@ struct CharDriverState {
     int explicit_be_open;
     int avail_connections;
     int is_mux;
+    guint fd_in_tag;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
 };
qemu-char.c +39 -38

179 lines not on this page · raw message

Signature · 1 line
1.8.3.1
author
#2 lore raw

PATCH v2 2/3 char: use common function to disable callbacks on chardev close

In reply to Amit Shah

This deduplicates code used a lot of times.

CC <[email protected]>
Signed-off-by Amit Shah <[email protected]>
---
 qemu-char.c | 62 +++++++++++++++++++------------------------------------------
 1 file changed, 19 insertions(+), 43 deletions(-)
qemu-char.c +19 -43
index cb4a311..44d993a 100644
@@ -725,6 +725,14 @@ static void io_remove_watch_poll(guint tag)
     g_source_destroy(&iwp->parent);
 }
 
+static void remove_fd_in_watch(CharDriverState *chr)
+{
+    if (chr->fd_in_tag) {
+        io_remove_watch_poll(chr->fd_in_tag);
+        chr->fd_in_tag = 0;
+    }
+}
+
 #ifndef _WIN32
 static GIOChannel *io_channel_from_fd(int fd)
 {

@@ -829,10 +837,7 @@ static gboolean fd_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     status = g_io_channel_read_chars(chan, (gchar *)buf,
                                      len, &bytes_read, NULL);
     if (status == G_IO_STATUS_EOF) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return FALSE;
     }

@@ -862,11 +867,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
-
+    remove_fd_in_watch(chr);
     if (s->fd_in) {
         chr->fd_in_tag = io_add_watch_poll(s->fd_in, fd_chr_read_poll, fd_chr_read,
                                      chr);

@@ -877,11 +878,7 @@ static void fd_chr_close(struct CharDriverState *chr)
 {
     FDCharDriver *s = chr->opaque;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
-
+    remove_fd_in_watch(chr);
     if (s->fd_in) {
         g_io_channel_unref(s->fd_in);
     }

@@ -1126,10 +1123,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
     PtyCharDriver *s = chr->opaque;
 
     if (!connected) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
          * We check more frequently in case the guests sends data to

@@ -1155,10 +1149,7 @@ static void pty_chr_close(struct CharDriverState *chr)
     PtyCharDriver *s = chr->opaque;
     int fd;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
+    remove_fd_in_watch(chr);
     fd = g_io_channel_unix_get_fd(s->fd);
     g_io_channel_unref(s->fd);
     close(fd);

@@ -2220,10 +2211,7 @@ static gboolean udp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
     s->bufcnt = bytes_read;
     s->bufptr = s->bufcnt;
     if (status != G_IO_STATUS_NORMAL) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         return FALSE;
     }
 

@@ -2241,11 +2229,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
 
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
-
+    remove_fd_in_watch(chr);
     if (s->chan) {
         chr->fd_in_tag = io_add_watch_poll(s->chan, udp_chr_read_poll, udp_chr_read,
                                      chr);

@@ -2255,10 +2239,8 @@ static void udp_chr_update_read_handler(CharDriverState *chr)
 static void udp_chr_close(CharDriverState *chr)
 {
     NetCharDriver *s = chr->opaque;
-    if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
-        chr->fd_in_tag = 0;
-    }
+
+    remove_fd_in_watch(chr);
     if (s->chan) {
         g_io_channel_unref(s->chan);
         closesocket(s->fd);

@@ -2493,10 +2475,7 @@ static gboolean tcp_chr_read(GIOChannel *chan, GIOCondition cond, void *opaque)
         if (s->listen_chan) {
             s->listen_tag = g_io_add_watch(s->listen_chan, G_IO_IN, tcp_chr_accept, chr);
         }
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         g_io_channel_unref(s->chan);
         s->chan = NULL;
         closesocket(s->fd);

@@ -2610,10 +2589,7 @@ static void tcp_chr_close(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
     if (s->fd >= 0) {
-        if (chr->fd_in_tag) {
-            io_remove_watch_poll(chr->fd_in_tag);
-            chr->fd_in_tag = 0;
-        }
+        remove_fd_in_watch(chr);
         if (s->chan) {
             g_io_channel_unref(s->chan);
         }
Signature · 1 line
1.8.3.1
author
#3 lore raw

PATCH v2 3/3 char: remove watch callback on chardev detach from frontend

In reply to Amit Shah

If a frontend device releases the chardev (via unplug), the chr handlers are set to NULL via qdev's exit callbacks invoking qemu_chr_add_handlers(). If the chardev had a pending operation, a callback will be invoked, which will try to access data in the just-released frontend, causing a segfault.

Ensure the callbacks are disabled when frontends release chardevs.

This was seen when a virtio-serial port was unplugged when heavy guest->host IO was in progress (causing a callback to be registered). In the window in which the throttling was active, unplugging ports caused a qemu segfault.

https://bugzilla.redhat.com/show_bug.cgi?id=985205

CC <[email protected]>
Reported-by Sibiao Luo <[email protected]>
Signed-off-by Amit Shah <[email protected]>
---
 qemu-char.c | 3 +++
 1 file changed, 3 insertions(+)
qemu-char.c +3 -0
index 44d993a..0cba7d4 100644
@@ -193,6 +193,8 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
     va_end(ap);
 }
 
+static void remove_fd_in_watch(CharDriverState *chr);
+
 void qemu_chr_add_handlers(CharDriverState *s,
                            IOCanReadHandler *fd_can_read,
                            IOReadHandler *fd_read,

@@ -203,6 +205,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
 
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
+        remove_fd_in_watch(s);
     } else {
         fe_open = 1;
     }
Signature · 1 line
1.8.3.1
author
#5 lore raw
Quoted 8 lines · Gerd Hoffmann
Quoted 3 lines · Amit Shah

This series fixes a segfault when a frontend is detached from a chardev while the chardev had a pending callback registered. Further details in patch 3.

Nice.

Reviewed-by Gerd Hoffmann <[email protected]>

Thanks!

(Sorry, had to do a v3 to fix whitespace)

Amit