[Qemu-devel] [PULL 0/9] Ui 20180216 patches

[Qemu-devel] [PULL 0/9] Ui 20180216 patches

[Gerd Hoffmann]

The following changes since commit fb68096da3d35e64c88cd610c1fa42766c58e92a:

  Revert "tests: use memfd in vhost-user-test" (2018-02-13 09:51:52 +0000)

are available in the git repository at:

  git://git.kraxel.org/qemu tags/ui-20180216-pull-request

for you to fetch changes up to d50f09ff23f5509c05e3883440849b27af051f08:

  ui: extend VNC trottling tracing to SASL codepaths (2018-02-16 12:33:02 +0100)

----------------------------------------------------------------
bugfixes for vnc and sdl2

----------------------------------------------------------------

Anatoly Trosinenko (1):
  sdl: restore optimized redraw

Daniel P. Berrangé (5):
  vnc: remove bogus object_unref on client socket
  ui: avoid risk of 32-bit int overflow in VNC buffer check
  ui: avoid 'local_err' variable shadowing in VNC SASL auth
  ui: check VNC audio frequency limit at time of reading from client
  ui: extend VNC trottling tracing to SASL codepaths

Gerd Hoffmann (2):
  vnc: add qapi/error.h include to stubs
  sdl2: fix mouse grab

Klim Kireev (1):
  vnc: fix segfault in closed connection handling

 ui/sdl2-2d.c       | 21 ++++++---------------
 ui/sdl2.c          |  1 +
 ui/vnc-auth-sasl.c | 13 +++++++++++--
 ui/vnc-jobs.c      |  6 ++++--
 ui/vnc-stubs.c     |  1 +
 ui/vnc.c           | 42 +++++++++++++++++++++++++++++-------------
 6 files changed, 52 insertions(+), 32 deletions(-)

-- 
2.9.3

[Qemu-devel] [PULL 1/9] vnc: remove bogus object_unref on client socket

[Gerd Hoffmann]

From: Daniel P. Berrangé <berrange@redhat.com>

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 <bsd@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-id: 20180215102602.10864-1-berrange@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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 = {
-- 
2.9.3

[Qemu-devel] [PULL 2/9] vnc: add qapi/error.h include to stubs

[Gerd Hoffmann]

Fixes --disable-vnc build failure.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
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)
 {
-- 
2.9.3

[Qemu-devel] [PULL 3/9] vnc: fix segfault in closed connection handling

[Gerd Hoffmann]

From: Klim Kireev <klim.kireev@virtuozzo.com>

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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-id: 20180207094844.21402-1-klim.kireev@virtuozzo.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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);
 }
 
-- 
2.9.3

[Qemu-devel] [PULL 4/9] sdl: restore optimized redraw

[Gerd Hoffmann]

From: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>

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 <anatoly.trosinenko@gmail.com>
Message-id: 20180205133228.25082-1-anatoly.trosinenko@gmail.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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);
 }
 
-- 
2.9.3

Re: [Qemu-devel] [PULL 4/9] sdl: restore optimized redraw

[Peter Maydell]

On 16 February 2018 at 11:54, Gerd Hoffmann <kraxel@redhat.com> wrote:
> From: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
>
> 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 <anatoly.trosinenko@gmail.com>
> Message-id: 20180205133228.25082-1-anatoly.trosinenko@gmail.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  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);
>

Hi. Coverity points out (CID 1390598) that this change adds
a use of 'surf' before the "if (!surf) { return; }" check.
I suspect the calculation of surface_data_offset should be
done lower in the function, after all the early-exit checks.

thanks
-- PMM

[Qemu-devel] [PULL 5/9] sdl2: fix mouse grab

[Gerd Hoffmann]

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 <kraxel@redhat.com>
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) {
-- 
2.9.3

[Qemu-devel] [PULL 6/9] ui: avoid risk of 32-bit int overflow in VNC buffer check

[Gerd Hoffmann]

From: Daniel P. Berrangé <berrange@redhat.com>

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 <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 20180205114938.15784-2-berrange@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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);
-- 
2.9.3

[Qemu-devel] [PULL 7/9] ui: avoid 'local_err' variable shadowing in VNC SASL auth

[Gerd Hoffmann]

From: Daniel P. Berrangé <berrange@redhat.com>

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 <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 20180205114938.15784-3-berrange@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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;
-- 
2.9.3

[Qemu-devel] [PULL 8/9] ui: check VNC audio frequency limit at time of reading from client

[Gerd Hoffmann]

From: Daniel P. Berrangé <berrange@redhat.com>

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 <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-id: 20180205114938.15784-4-berrange@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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));
-- 
2.9.3

[Qemu-devel] [PULL 9/9] ui: extend VNC trottling tracing to SASL codepaths

[Gerd Hoffmann]

From: Daniel P. Berrangé <berrange@redhat.com>

In previous commit:

  commit 6aa22a29187e1908f5db738d27c64a9efc8d0bfa
  Author: Daniel P. Berrange <berrange@redhat.com>
  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 <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Message-id: 20180205114938.15784-5-berrange@redhat.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 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;
     }
-- 
2.9.3

Re: [Qemu-devel] [PULL 0/9] Ui 20180216 patches

[Peter Maydell]

On 16 February 2018 at 11:54, Gerd Hoffmann <kraxel@redhat.com> wrote:
> The following changes since commit fb68096da3d35e64c88cd610c1fa42766c58e92a:
>
>   Revert "tests: use memfd in vhost-user-test" (2018-02-13 09:51:52 +0000)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/ui-20180216-pull-request
>
> for you to fetch changes up to d50f09ff23f5509c05e3883440849b27af051f08:
>
>   ui: extend VNC trottling tracing to SASL codepaths (2018-02-16 12:33:02 +0100)
>
> ----------------------------------------------------------------
> bugfixes for vnc and sdl2
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM