char: fix segfault on chardev detach
cover char: fix segfault on chardev detach
This series fixes a segfault when a frontend is detached from a chardev, and the chardev had a pending callback registered. Further details in patch 1.
Please review,
Amit Shah (9):
char: remove watch callback on chardev detach from frontend
char: introduce tcp_chr_detach()
char: introduce fd_chr_detach()
char: introduce pty_chr_detach()
char: introduce udp_chr_detach()
char: use the new fd_chr_detach to dedup code
char: use the new pty_chr_detach to dedup code
char: use the new udp_chr_detach to dedup code
char: use the new tcp_chr_detach to dedup code
include/sysemu/char.h | 1 + qemu-char.c | 102 +++++++++++++++++++++++++++++--------------------- 2 files changed, 60 insertions(+), 43 deletions(-)
Signature · 1 line
1.8.3.1
PATCH 1/9 char: remove watch callback on chardev detach from frontend
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 NULL pointer dereference.
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
--- include/sysemu/char.h | 1 + qemu-char.c | 3 +++ 2 files changed, 4 insertions(+)
include/sysemu/char.h +1 -0
@@ -69,6 +69,7 @@ struct CharDriverState {void (*chr_accept_input)(struct CharDriverState *chr);void (*chr_set_echo)(struct CharDriverState *chr,bool echo);void (*chr_set_fe_open)(struct CharDriverState *chr,int fe_open); +void (*chr_detach)(struct CharDriverState *chr);void *opaque;char *label;char *filename;
qemu-char.c +3 -0
@@ -203,6 +203,9 @@ void qemu_chr_add_handlers(CharDriverState *s,if (!opaque && !fd_can_read && !fd_read && !fd_event) { fe_open =0 ; +if (s->handler_opaque && s->chr_detach) { + s->chr_detach (s); + } }else { fe_open =1 ; }
Signature · 1 line
1.8.3.1
PATCH 2/9 char: introduce tcp_chr_detach()
In reply to Amit ShahRemove any registered callbacks if a frontend is detached.
--- qemu-char.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
qemu-char.c +11 -0
@@ -2321,6 +2321,16 @@ typedef struct {int msgfd; } TCPCharDriver; +static void tcp_chr_detach (CharDriverState *chr) +{ +TCPCharDriver *s = chr->opaque; + +if (s->tag) { +io_remove_watch_poll (s->tag); + s->tag =0 ; + } +} +static gboolean tcp_chr_accept (GIOChannel *chan,GIOCondition cond,void *opaque);static int tcp_chr_write (CharDriverState *chr,const uint8_t *buf,int len)
@@ -2689,6 +2699,7 @@ static CharDriverState *qemu_chr_open_socket_fd(int fd, bool do_nodelay, chr->opaque = s; chr->chr_write = tcp_chr_write; chr->chr_close = tcp_chr_close; + chr->chr_detach = tcp_chr_detach; chr->get_msgfd = tcp_get_msgfd; chr->chr_add_client = tcp_chr_add_client; chr->chr_add_watch = tcp_chr_add_watch;
Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 2/9] char: introduce tcp_chr_detach()
Hi,
qemu-char.c typedef struct { · line 2332
Quoted 9 lines
qemu-char.c +9
-0+static void tcp_chr_detach (CharDriverState *chr) +{ +TCPCharDriver *s = chr->opaque; + +if (s->tag) { +io_remove_watch_poll (s->tag); + s->tag =0 ; + } +}
Lots of simliar functions in the other patches.
Doesn't it make sense to move the tag field from TCPCharDriver to CharDriverState instead, so we don't need a new callback in the first place?
cheers,
Gerd
Re: [Qemu-devel] [PATCH 2/9] char: introduce tcp_chr_detach()
Quoted 14 lines · Gerd Hoffmann
Hi,
qemu-char.c typedef struct { · line 2332
Quoted 9 lines
qemu-char.c +9
-0+static void tcp_chr_detach (CharDriverState *chr) +{ +TCPCharDriver *s = chr->opaque; + +if (s->tag) { +io_remove_watch_poll (s->tag); + s->tag =0 ; + } +}Lots of simliar functions in the other patches.
Quoted 3 lines
Doesn't it make sense to move the tag field from TCPCharDriver to CharDriverState instead, so we don't need a new callback in the first place?
Yep, I thought about it, but it might get tricky to handle it:
tcp needs two, one for listening sockets and one for connected ones.
We don't need to worry about the listening socket for this patchset, should we then just keep that in the tcp struct, and use the tag as the generic one in CharDriverState for all of the backends?
Amit
Re: [Qemu-devel] [PATCH 2/9] char: introduce tcp_chr_detach()
Quoted 4 lines
We don't need to worry about the listening socket for this patchset, should we then just keep that in the tcp struct, and use the tag as the generic one in CharDriverState for all of the backends?
Yes, I think that will simplify the series. And maybe name the one one in CharDriverState 'fd_in_tag' to make more clear what it is.
cheers,
Gerd
PATCH 3/9 char: introduce fd_chr_detach()
In reply to Amit ShahRemove any registered callbacks if a frontend is detached.
--- qemu-char.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
qemu-char.c +12 -0
@@ -806,6 +806,17 @@ typedef struct FDCharDriver {QTAILQ_ENTRY (FDCharDriver) node; } FDCharDriver; + +static void fd_chr_detach (struct CharDriverState *chr) +{ +FDCharDriver *s = chr->opaque; + +if (s->fd_in_tag) { +io_remove_watch_poll (s->fd_in_tag); + s->fd_in_tag =0 ; + } +} +static int fd_chr_write (CharDriverState *chr,const uint8_t *buf,int len) {FDCharDriver *s = chr->opaque;
@@ -913,6 +924,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out) chr->chr_write = fd_chr_write; chr->chr_update_read_handler = fd_chr_update_read_handler; chr->chr_close = fd_chr_close; + chr->chr_detach = fd_chr_detach;return chr; }
Signature · 1 line
1.8.3.1
PATCH 4/9 char: introduce pty_chr_detach()
In reply to Amit ShahRemove any registered callbacks if a frontend is detached.
--- qemu-char.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
qemu-char.c +11 -0
@@ -1036,6 +1036,16 @@ typedef struct {static void pty_chr_update_read_handler (CharDriverState *chr);static void pty_chr_state (CharDriverState *chr,int connected); +static void pty_chr_detach (struct CharDriverState *chr) +{ +PtyCharDriver *s = chr->opaque; + +if (s->fd_tag) { +io_remove_watch_poll (s->fd_tag); + s->fd_tag =0 ; + } +} +static gboolean pty_chr_timer (gpointer opaque) {struct CharDriverState *chr = opaque;
@@ -1215,6 +1225,7 @@ static CharDriverState *qemu_chr_open_pty(const char *id, chr->chr_update_read_handler = pty_chr_update_read_handler; chr->chr_close = pty_chr_close; chr->chr_add_watch = pty_chr_add_watch; + chr->chr_detach = pty_chr_detach; chr->explicit_be_open = true; s->fd =io_channel_from_fd (master_fd);
Signature · 1 line
1.8.3.1
PATCH 5/9 char: introduce udp_chr_detach()
In reply to Amit ShahRemove any registered callbacks if a frontend is detached.
--- qemu-char.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
qemu-char.c +11 -0
@@ -2198,6 +2198,16 @@ typedef struct {int max_size; } NetCharDriver; +static void udp_chr_detach (CharDriverState *chr) +{ +NetCharDriver *s = chr->opaque; + +if (s->tag) { +io_remove_watch_poll (s->tag); + s->tag =0 ; + } +} +static int udp_chr_write (CharDriverState *chr,const uint8_t *buf,int len) {NetCharDriver *s = chr->opaque;
@@ -2309,6 +2319,7 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd) chr->chr_write = udp_chr_write; chr->chr_update_read_handler = udp_chr_update_read_handler; chr->chr_close = udp_chr_close; + chr->chr_detach = udp_chr_detach;/* be isn't opened until we get a connection */ chr->explicit_be_open = true;return chr;
Signature · 1 line
1.8.3.1
PATCH 6/9 char: use the new fd_chr_detach to dedup code
In reply to Amit Shah--- qemu-char.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-)
qemu-char.c +3 -14
@@ -844,10 +844,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 (s->fd_in_tag) { -io_remove_watch_poll (s->fd_in_tag); - s->fd_in_tag =0 ; - } +fd_chr_detach (chr);qemu_chr_be_event (chr,CHR_EVENT_CLOSED );return FALSE; }
@@ -877,11 +874,7 @@ static void fd_chr_update_read_handler(CharDriverState *chr) {FDCharDriver *s = chr->opaque; -if (s->fd_in_tag) { -io_remove_watch_poll (s->fd_in_tag); - s->fd_in_tag =0 ; - } - +fd_chr_detach (chr);if (s->fd_in) { s->fd_in_tag =io_add_watch_poll (s->fd_in, fd_chr_read_poll, fd_chr_read, chr); }
@@ -891,11 +884,7 @@ static void fd_chr_close(struct CharDriverState *chr) {FDCharDriver *s = chr->opaque; -if (s->fd_in_tag) { -io_remove_watch_poll (s->fd_in_tag); - s->fd_in_tag =0 ; - } - +fd_chr_detach (chr);if (s->fd_in) {g_io_channel_unref (s->fd_in); }
Signature · 1 line
1.8.3.1
PATCH 7/9 char: use the new pty_chr_detach to dedup code
In reply to Amit Shah--- qemu-char.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
qemu-char.c +2 -8
@@ -1141,10 +1141,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)PtyCharDriver *s = chr->opaque;if (!connected) { -if (s->fd_tag) { -io_remove_watch_poll (s->fd_tag); - s->fd_tag =0 ; - } +pty_chr_detach (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
@@ -1169,10 +1166,7 @@ static void pty_chr_close(struct CharDriverState *chr)PtyCharDriver *s = chr->opaque;int fd; -if (s->fd_tag) { -io_remove_watch_poll (s->fd_tag); - s->fd_tag =0 ; - } +pty_chr_detach (chr); fd =g_io_channel_unix_get_fd (s->fd);g_io_channel_unref (s->fd);close (fd);
Signature · 1 line
1.8.3.1
PATCH 8/9 char: use the new udp_chr_detach to dedup code
In reply to Amit Shah--- qemu-char.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)
qemu-char.c +4 -13
@@ -2240,10 +2240,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 (s->tag) { -io_remove_watch_poll (s->tag); - s->tag =0 ; - } +udp_chr_detach (chr);return FALSE; }
@@ -2261,11 +2258,7 @@ static void udp_chr_update_read_handler(CharDriverState *chr) {NetCharDriver *s = chr->opaque; -if (s->tag) { -io_remove_watch_poll (s->tag); - s->tag =0 ; - } - +udp_chr_detach (chr);if (s->chan) { s->tag =io_add_watch_poll (s->chan, udp_chr_read_poll, udp_chr_read, chr); }
@@ -2274,10 +2267,8 @@ static void udp_chr_update_read_handler(CharDriverState *chr)static void udp_chr_close (CharDriverState *chr) {NetCharDriver *s = chr->opaque; -if (s->tag) { -io_remove_watch_poll (s->tag); - s->tag =0 ; - } + +udp_chr_detach (chr);if (s->chan) {g_io_channel_unref (s->chan);closesocket (s->fd);
Signature · 1 line
1.8.3.1
PATCH 9/9 char: use the new tcp_chr_detach to dedup code
In reply to Amit Shah--- qemu-char.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
qemu-char.c +2 -8
@@ -2514,10 +2514,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 (s->tag) { -io_remove_watch_poll (s->tag); - s->tag =0 ; - } +tcp_chr_detach (chr);g_io_channel_unref (s->chan); s->chan =NULL ;closesocket (s->fd);
@@ -2630,10 +2627,7 @@ static void tcp_chr_close(CharDriverState *chr) {TCPCharDriver *s = chr->opaque;if (s->fd >=0 ) { -if (s->tag) { -io_remove_watch_poll (s->tag); - s->tag =0 ; - } +tcp_chr_detach (chr);if (s->chan) {g_io_channel_unref (s->chan); }
Signature · 1 line
1.8.3.1