* [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
* Re: [Qemu-devel] [PATCH v3] vnc: fix segfault in closed connection handling
2018-02-07 9:48 [Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling Klim Kireev
@ 2018-02-14 14:43 ` Klim Kireev
2018-02-16 11:25 ` Gerd Hoffmann
2018-02-14 15:21 ` [Qemu-devel] [PATCH] " Daniel P. Berrangé
1 sibling, 1 reply; 7+ messages in thread
From: Klim Kireev @ 2018-02-14 14:43 UTC (permalink / raw)
To: qemu-devel; +Cc: den, kraxel
ping
On 02/07/2018 12:48 PM, Klim Kireev wrote:
> 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);
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling
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-14 15:21 ` Daniel P. Berrangé
1 sibling, 0 replies; 7+ messages in thread
From: Daniel P. Berrangé @ 2018-02-14 15:21 UTC (permalink / raw)
To: Klim Kireev; +Cc: qemu-devel, den, kraxel
On Wed, Feb 07, 2018 at 12:48:44PM +0300, Klim Kireev wrote:
> 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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> 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
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH v3] vnc: fix segfault in closed connection handling
2018-02-14 14:43 ` [Qemu-devel] [PATCH v3] " Klim Kireev
@ 2018-02-16 11:25 ` Gerd Hoffmann
0 siblings, 0 replies; 7+ messages in thread
From: Gerd Hoffmann @ 2018-02-16 11:25 UTC (permalink / raw)
To: Klim Kireev; +Cc: qemu-devel, den
On Wed, Feb 14, 2018 at 05:43:19PM +0300, Klim Kireev wrote:
> ping
Queued now. Was lingering in my inbox, waiting for me to find the time
for the next ui pull request.
cheers,
Gerd
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] vnc: fix segfault in closed connection handling
2018-01-31 13:16 ` Marc-André Lureau
@ 2018-01-31 13:18 ` klim
0 siblings, 0 replies; 7+ messages in thread
From: klim @ 2018-01-31 13:18 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Gerd Hoffmann
On 01/31/2018 04:16 PM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jan 31, 2018 at 2:06 PM, Klim Kireev <klim.kireev@virtuozzo.com> wrote:
>> On one of our client's node, due to trying to read from closed ioc,
>> a segmentation fault occured. Corresponding backtrace:
>>
> Oops, you probably forgot an extra space before the # interpreted as comment.
Thanks, fixed
>
> Do you have a reproducer?
Unfortunately, no
>> 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 [flat|nested] 7+ messages in thread
* Re: [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
2018-01-31 13:18 ` klim
0 siblings, 1 reply; 7+ messages in thread
From: Marc-André Lureau @ 2018-01-31 13:16 UTC (permalink / raw)
To: Klim Kireev; +Cc: QEMU, Gerd Hoffmann
Hi
On Wed, Jan 31, 2018 at 2:06 PM, Klim Kireev <klim.kireev@virtuozzo.com> wrote:
> On one of our client's node, due to trying to read from closed ioc,
> a segmentation fault occured. Corresponding backtrace:
>
Oops, you probably forgot an extra space before the # interpreted as comment.
Do you have a reproducer?
> 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
>
>
--
Marc-André Lureau
^ permalink raw reply [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.