From f42fdb24b740f2b99526128dd6e78197e033be11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 15 Feb 2018 10:26:02 +0000 Subject: [PATCH 1/9] vnc: remove bogus object_unref on client socket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vnc_listen_io() does not own the reference on the 'cioc' parameter is it passed, so should not be unref'ing it. Fixes: 13e1d0e71e78a925848258391a6e616b6b5ae219 Reported-by: Bandan Das Signed-off-by: Daniel P. Berrangé Reviewed-by: Eric Blake Message-id: 20180215102602.10864-1-berrange@redhat.com Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ui/vnc.c b/ui/vnc.c index c715bae1cf..b97769aa9e 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -3152,7 +3152,6 @@ static void vnc_listen_io(QIONetListener *listener, isWebsock ? "vnc-ws-server" : "vnc-server"); qio_channel_set_delay(QIO_CHANNEL(cioc), false); vnc_connect(vd, cioc, false, isWebsock); - object_unref(OBJECT(cioc)); } static const DisplayChangeListenerOps dcl_ops = { From 577ce409acd439b7b5ad14935569cfb10bf261f3 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 13 Feb 2018 08:05:26 +0100 Subject: [PATCH 2/9] vnc: add qapi/error.h include to stubs Fixes --disable-vnc build failure. Signed-off-by: Gerd Hoffmann Reviewed-by: Eric Blake Message-id: 20180213070526.22475-1-kraxel@redhat.com --- ui/vnc-stubs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/vnc-stubs.c b/ui/vnc-stubs.c index f51280549a..06c4ac6296 100644 --- a/ui/vnc-stubs.c +++ b/ui/vnc-stubs.c @@ -1,5 +1,6 @@ #include "qemu/osdep.h" #include "ui/console.h" +#include "qapi/error.h" int vnc_display_password(const char *id, const char *password) { From d49b87f0d1e0520443a990fc610d0f02bc63c556 Mon Sep 17 00:00:00 2001 From: Klim Kireev Date: Wed, 7 Feb 2018 12:48:44 +0300 Subject: [PATCH 3/9] vnc: fix segfault in closed connection handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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= ... 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=, 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=) 11 main_loop_wait (nonblocking=nonblocking@entry=0) 12 main_loop () at vl.c:1909 13 main (argc=, argv=, ... 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 Reviewed-by: Daniel P. Berrangé Message-id: 20180207094844.21402-1-klim.kireev@virtuozzo.com Signed-off-by: Gerd Hoffmann --- 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 b97769aa9e..e371710f4f 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); } From 2ab858c6c38ee152299445f2810dbd4c3a0ac7ee Mon Sep 17 00:00:00 2001 From: Anatoly Trosinenko Date: Mon, 5 Feb 2018 16:32:28 +0300 Subject: [PATCH 4/9] sdl: restore optimized redraw The documentation on SDL_RenderPresent function states that "the backbuffer should be considered invalidated after each present", so copy the entire texture on each redraw. On the other hand, SDL_UpdateTexture function is described as "fairly slow function", so restrict it to just the changed pixels. Also added SDL_RenderClear call, as suggested in the documentation page on SDL_RenderPresent. Signed-off-by: Anatoly Trosinenko Message-id: 20180205133228.25082-1-anatoly.trosinenko@gmail.com Signed-off-by: Gerd Hoffmann --- ui/sdl2-2d.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c index 8ab68d67b9..1f34817bae 100644 --- a/ui/sdl2-2d.c +++ b/ui/sdl2-2d.c @@ -36,6 +36,8 @@ void sdl2_2d_update(DisplayChangeListener *dcl, struct sdl2_console *scon = container_of(dcl, struct sdl2_console, dcl); DisplaySurface *surf = qemu_console_surface(dcl->con); SDL_Rect rect; + size_t surface_data_offset = surface_bytes_per_pixel(surf) * x + + surface_stride(surf) * y; assert(!scon->opengl); @@ -46,27 +48,16 @@ void sdl2_2d_update(DisplayChangeListener *dcl, return; } - /* - * SDL2 seems to do some double-buffering, and trying to only - * update the changed areas results in only one of the two buffers - * being updated. Which flickers alot. So lets not try to be - * clever do a full update every time ... - */ -#if 0 rect.x = x; rect.y = y; rect.w = w; rect.h = h; -#else - rect.x = 0; - rect.y = 0; - rect.w = surface_width(surf); - rect.h = surface_height(surf); -#endif - SDL_UpdateTexture(scon->texture, NULL, surface_data(surf), + SDL_UpdateTexture(scon->texture, &rect, + surface_data(surf) + surface_data_offset, surface_stride(surf)); - SDL_RenderCopy(scon->real_renderer, scon->texture, &rect, &rect); + SDL_RenderClear(scon->real_renderer); + SDL_RenderCopy(scon->real_renderer, scon->texture, NULL, NULL); SDL_RenderPresent(scon->real_renderer); } From 8dfa3061ce56d871dc9df1e264f05e7ec2fb50c1 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Fri, 2 Feb 2018 13:08:03 +0100 Subject: [PATCH 5/9] sdl2: fix mouse grab When qemu mouse mode changes from relative to absolute we must turn off sdl relative mouse mode too. Fixes: https://bugs.launchpad.net/qemu/+bug/1703795 Signed-off-by: Gerd Hoffmann Message-Id: <20180202120803.11501-1-kraxel@redhat.com> --- ui/sdl2.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/sdl2.c b/ui/sdl2.c index 812c315891..858e04d7c0 100644 --- a/ui/sdl2.c +++ b/ui/sdl2.c @@ -249,6 +249,7 @@ static void sdl_mouse_mode_change(Notifier *notify, void *data) if (qemu_input_is_absolute()) { if (!absolute_enabled) { absolute_enabled = 1; + SDL_SetRelativeMouseMode(SDL_FALSE); absolute_mouse_grab(&sdl2_console[0]); } } else if (absolute_enabled) { From dffa1de071aa956308172170107b7b60d99bf34b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Feb 2018 11:49:35 +0000 Subject: [PATCH 6/9] ui: avoid risk of 32-bit int overflow in VNC buffer check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For very large framebuffers, it is theoretically possible for the result of 'vs->throttle_output_offset * VNC_THROTTLE_OUTPUT_LIMIT_SCALE' to exceed the size of a 32-bit int. For this to happen in practice, the video RAM would have to be set to a large enough value, which is not likely today. None the less we can be paranoid against future growth by using division instead of multiplication when checking the limits. Reported-by: Laszlo Ersek Signed-off-by: Daniel P. Berrangé Reviewed-by: Laszlo Ersek Reviewed-by: Philippe Mathieu-Daudé Message-id: 20180205114938.15784-2-berrange@redhat.com Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/vnc.c b/ui/vnc.c index e371710f4f..746293ddfa 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -1579,8 +1579,8 @@ void vnc_write(VncState *vs, const void *data, size_t len) * handshake, or from the job thread's VncState clone */ if (vs->throttle_output_offset != 0 && - vs->output.offset > (vs->throttle_output_offset * - VNC_THROTTLE_OUTPUT_LIMIT_SCALE)) { + (vs->output.offset / VNC_THROTTLE_OUTPUT_LIMIT_SCALE) > + vs->throttle_output_offset) { trace_vnc_client_output_limit(vs, vs->ioc, vs->output.offset, vs->throttle_output_offset); vnc_disconnect_start(vs); From 52c7c9d076dc64a6d3f1938b5a4994f84744c7fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Feb 2018 11:49:36 +0000 Subject: [PATCH 7/9] ui: avoid 'local_err' variable shadowing in VNC SASL auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The start_auth_sasl() method declares a 'Error *local_err' variable in an inner if () {...} scope, which shadows a variable of the same name declared at the start of the method. This is confusing for reviewers and may trigger compiler warnings. Reported-by: Laszlo Ersek Signed-off-by: Daniel P. Berrangé Reviewed-by: Laszlo Ersek Reviewed-by: Philippe Mathieu-Daudé Message-id: 20180205114938.15784-3-berrange@redhat.com Signed-off-by: Gerd Hoffmann --- ui/vnc-auth-sasl.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c index fbccca8c8a..8ebd0d3d00 100644 --- a/ui/vnc-auth-sasl.c +++ b/ui/vnc-auth-sasl.c @@ -556,7 +556,6 @@ void start_auth_sasl(VncState *vs) /* Inform SASL that we've got an external SSF layer from TLS/x509 */ if (vs->auth == VNC_AUTH_VENCRYPT && vs->subauth == VNC_AUTH_VENCRYPT_X509SASL) { - Error *local_err = NULL; int keysize; sasl_ssf_t ssf; @@ -565,7 +564,6 @@ void start_auth_sasl(VncState *vs) if (keysize < 0) { trace_vnc_auth_fail(vs, vs->auth, "cannot TLS get cipher size", error_get_pretty(local_err)); - error_free(local_err); sasl_dispose(&vs->sasl.conn); vs->sasl.conn = NULL; goto authabort; From cf0706581bc0c24ab2e9a81ff0fc3efa9482c812 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Feb 2018 11:49:37 +0000 Subject: [PATCH 8/9] ui: check VNC audio frequency limit at time of reading from client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'vs->as.freq' value is a signed integer, which is read from an unsigned 32-bit int field on the wire. There is thus a risk of overflow on 32-bit platforms. Move the frequency limit checking to be done at time of read before casting to a signed integer. Reported-by: Laszlo Ersek Signed-off-by: Daniel P. Berrangé Reviewed-by: Laszlo Ersek Reviewed-by: Philippe Mathieu-Daudé Message-id: 20180205114938.15784-4-berrange@redhat.com Signed-off-by: Gerd Hoffmann --- ui/vnc.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/ui/vnc.c b/ui/vnc.c index 746293ddfa..a77b568b57 100644 --- a/ui/vnc.c +++ b/ui/vnc.c @@ -982,14 +982,7 @@ static void vnc_update_throttle_offset(VncState *vs) vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel; if (vs->audio_cap) { - int freq = vs->as.freq; - /* We don't limit freq when reading settings from client, so - * it could be upto MAX_INT in size. 48khz is a sensible - * upper bound for trustworthy clients */ int bps; - if (freq > 48000) { - freq = 48000; - } switch (vs->as.fmt) { default: case AUD_FMT_U8: @@ -1005,7 +998,7 @@ static void vnc_update_throttle_offset(VncState *vs) bps = 4; break; } - offset += freq * bps * vs->as.nchannels; + offset += vs->as.freq * bps * vs->as.nchannels; } /* Put a floor of 1MB on offset, so that if we have a large pending @@ -2292,6 +2285,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len) { int i; uint16_t limit; + uint32_t freq; VncDisplay *vd = vs->vd; if (data[0] > 3) { @@ -2411,7 +2405,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len) vnc_client_error(vs); break; } - vs->as.freq = read_u32(data, 6); + freq = read_u32(data, 6); + /* No official limit for protocol, but 48khz is a sensible + * upper bound for trustworthy clients, and this limit + * protects calculations involving 'vs->as.freq' later. + */ + if (freq > 48000) { + VNC_DEBUG("Invalid audio frequency %u > 48000", freq); + vnc_client_error(vs); + break; + } + vs->as.freq = freq; break; default: VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4)); From d50f09ff23f5509c05e3883440849b27af051f08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 5 Feb 2018 11:49:38 +0000 Subject: [PATCH 9/9] ui: extend VNC trottling tracing to SASL codepaths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In previous commit: commit 6aa22a29187e1908f5db738d27c64a9efc8d0bfa Author: Daniel P. Berrange Date: Mon Dec 18 19:12:27 2017 +0000 ui: add trace events related to VNC client throttling trace points related to unthrottling client I/O were missed from the SASL codepaths. Reported-by: Laszlo Ersek Signed-off-by: Daniel P. Berrangé Reviewed-by: Laszlo Ersek Message-id: 20180205114938.15784-5-berrange@redhat.com Signed-off-by: Gerd Hoffmann --- ui/vnc-auth-sasl.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c index 8ebd0d3d00..3751a777a4 100644 --- a/ui/vnc-auth-sasl.c +++ b/ui/vnc-auth-sasl.c @@ -79,12 +79,23 @@ size_t vnc_client_write_sasl(VncState *vs) vs->sasl.encodedOffset += ret; if (vs->sasl.encodedOffset == vs->sasl.encodedLength) { + bool throttled = vs->force_update_offset != 0; + size_t offset; if (vs->sasl.encodedRawLength >= vs->force_update_offset) { vs->force_update_offset = 0; } else { vs->force_update_offset -= vs->sasl.encodedRawLength; } + if (throttled && vs->force_update_offset == 0) { + trace_vnc_client_unthrottle_forced(vs, vs->ioc); + } + offset = vs->output.offset; buffer_advance(&vs->output, vs->sasl.encodedRawLength); + if (offset >= vs->throttle_output_offset && + vs->output.offset < vs->throttle_output_offset) { + trace_vnc_client_unthrottle_incremental(vs, vs->ioc, + vs->output.offset); + } vs->sasl.encoded = NULL; vs->sasl.encodedOffset = vs->sasl.encodedLength = 0; }