qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling
@ 2018-02-07  9:48 Klim Kireev
  2018-02-14 14:43 ` [Qemu-devel] [PATCH v3] " Klim Kireev
  2018-02-14 15:21 ` [Qemu-devel] [PATCH] " Daniel P. Berrangé
  0 siblings, 2 replies; 7+ messages in thread
From: Klim Kireev @ 2018-02-07  9:48 UTC (permalink / raw)
  To: qemu-devel; +Cc: den, berrange, kraxel

On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:

0  object_get_class (obj=obj@entry=0x0)
1  qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
2  qio_channel_read (ioc=<optimized out> ...
3  vnc_client_read_buf (vs=vs@entry=0x55625f3c6000, ...
4  vnc_client_read_plain (vs=0x55625f3c6000)
5  vnc_client_read (vs=0x55625f3c6000)
6  vnc_client_io (ioc=<optimized out>, condition=G_IO_IN, ...
7  g_main_dispatch (context=0x556251568a50)
8  g_main_context_dispatch (context=context@entry=0x556251568a50)
9  glib_pollfds_poll ()
10 os_host_main_loop_wait (timeout=<optimized out>)
11 main_loop_wait (nonblocking=nonblocking@entry=0)
12 main_loop () at vl.c:1909
13 main (argc=<optimized out>, argv=<optimized out>, ...

Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.

The patch checks vs->disconnecting in places where we call
qio_channel_add_watch and resets handler if disconnecting == TRUE
to prevent such an occurrence.

Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com>
---
Changelog:
v2: Attach the backtrace

v3: Change checks

 ui/vnc-jobs.c |  6 ++++--
 ui/vnc.c      | 15 ++++++++++++++-
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index e326679dd0..868dddef4b 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -148,8 +148,10 @@ void vnc_jobs_consume_buffer(VncState *vs)
             if (vs->ioc_tag) {
                 g_source_remove(vs->ioc_tag);
             }
-            vs->ioc_tag = qio_channel_add_watch(
-                vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
+            if (vs->disconnecting == FALSE) {
+                vs->ioc_tag = qio_channel_add_watch(
+                    vs->ioc, G_IO_IN | G_IO_OUT, vnc_client_io, vs, NULL);
+            }
         }
         buffer_move(&vs->output, &vs->jobs_buffer);
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 93731accb6..67ccc8160f 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1536,12 +1536,19 @@ gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED,
     VncState *vs = opaque;
     if (condition & G_IO_IN) {
         if (vnc_client_read(vs) < 0) {
-            return TRUE;
+            goto end;
         }
     }
     if (condition & G_IO_OUT) {
         vnc_client_write(vs);
     }
+end:
+    if (vs->disconnecting) {
+        if (vs->ioc_tag != 0) {
+            g_source_remove(vs->ioc_tag);
+        }
+        vs->ioc_tag = 0;
+    }
     return TRUE;
 }
 
@@ -1630,6 +1637,12 @@ void vnc_flush(VncState *vs)
     if (vs->ioc != NULL && vs->output.offset) {
         vnc_client_write_locked(vs);
     }
+    if (vs->disconnecting) {
+        if (vs->ioc_tag != 0) {
+            g_source_remove(vs->ioc_tag);
+        }
+        vs->ioc_tag = 0;
+    }
     vnc_unlock_output(vs);
 }
 
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling
@ 2018-01-31 13:06 Klim Kireev
  2018-01-31 13:16 ` Marc-André Lureau
  0 siblings, 1 reply; 7+ messages in thread
From: Klim Kireev @ 2018-01-31 13:06 UTC (permalink / raw)
  To: qemu-devel; +Cc: kraxel

On one of our client's node, due to trying to read from closed ioc,
a segmentation fault occured. Corresponding backtrace:

Having analyzed the coredump, I understood that the reason is that
ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
in vnc_disconnect_finish. Between these two events due to some
reasons the ioc_tag was set again and after vnc_disconnect_finish
the handler is running with freed ioc,
which led to the segmentation fault.

I suggest to check ioc_tag in vnc_disconnect_finish to prevent such
an occurrence.

Signed-off-by: Klim Kireev <klim.kireev@virtuozzo.com>
---
 ui/vnc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index 33b087221f..b8bf0180cb 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1270,6 +1270,10 @@ void vnc_disconnect_finish(VncState *vs)
     }
     g_free(vs->lossy_rect);
 
+    if (vs->ioc_tag) {
+        g_source_remove(vs->ioc_tag);
+        vs->ioc_tag = 0;
+    }
     object_unref(OBJECT(vs->ioc));
     vs->ioc = NULL;
     object_unref(OBJECT(vs->sioc));
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-02-16 11:25 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-07  9:48 [Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling Klim Kireev
2018-02-14 14:43 ` [Qemu-devel] [PATCH v3] " Klim Kireev
2018-02-16 11:25   ` Gerd Hoffmann
2018-02-14 15:21 ` [Qemu-devel] [PATCH] " Daniel P. Berrangé
  -- strict thread matches above, loose matches on Subject: below --
2018-01-31 13:06 Klim Kireev
2018-01-31 13:16 ` Marc-André Lureau
2018-01-31 13:18   ` klim

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).