[PULL 00/11] Ui 20210316 patches

[PULL 00/11] Ui 20210316 patches

[Gerd Hoffmann]

The following changes since commit 6157b0e19721aadb4c7fdcfe57b2924af6144b14:

  Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.0-pull-=
request' into staging (2021-03-14 17:47:49 +0000)

are available in the Git repository at:

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

for you to fetch changes up to ad7f2f8ee9fbded410fbf77158b0065f8e2f08e3:

  ui/cocoa: Comment about modifier key input quirks (2021-03-16 06:36:45 +010=
0)

----------------------------------------------------------------
vnc+spice: password-secret option.
bugfixes for cocoa, vnc, opengl.

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

Akihiko Odaki (3):
  opengl: Do not convert format with glTexImage2D on OpenGL ES
  ui/cocoa: Do not exit immediately after shutdown
  ui/cocoa: Comment about modifier key input quirks

Daniel P. Berrang=C3=A9 (7):
  ui: introduce "password-secret" option for VNC servers
  ui: introduce "password-secret" option for SPICE server
  ui: deprecate "password" option for SPICE server
  ui: add more trace points for VNC client/server messages
  ui: avoid sending framebuffer updates outside client desktop bounds
  ui: use client width/height in WMVi message
  ui: honour the actual guest display dimensions without rounding

Marc-Andr=C3=A9 Lureau (1):
  ui: fold qemu_alloc_display in only caller

 ui/vnc.h                   |  1 +
 ui/console-gl.c            | 19 +++++++---
 ui/console.c               | 14 ++------
 ui/spice-core.c            | 32 +++++++++++++++--
 ui/vnc-jobs.c              | 44 ++++++++++++++++++++---
 ui/vnc.c                   | 71 +++++++++++++++++++++++++++++++++-----
 docs/system/deprecated.rst |  8 +++++
 qemu-options.hx            | 18 ++++++++--
 ui/cocoa.m                 | 46 ++++++++++++++++++++++--
 ui/trace-events            | 16 +++++++++
 10 files changed, 234 insertions(+), 35 deletions(-)

--=20
2.29.2

[PULL 01/11] ui: introduce "password-secret" option for VNC servers

[Gerd Hoffmann]

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

Currently when using VNC the "password" flag turns on password based
authentication. The actual password has to be provided separately via
the monitor.

This introduces a "password-secret" option which lets the password be
provided up front.

  $QEMU --object secret,id=vncsec0,file=passwd.txt \
        --vnc localhost:0,password-secret=vncsec0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20210311114343.439820-2-berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c        | 23 ++++++++++++++++++++++-
 qemu-options.hx |  5 +++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 310abc937812..e8e3426a65c4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -48,6 +48,7 @@
 #include "crypto/tlscredsanon.h"
 #include "crypto/tlscredsx509.h"
 #include "crypto/random.h"
+#include "crypto/secret_common.h"
 #include "qom/object_interfaces.h"
 #include "qemu/cutils.h"
 #include "qemu/help_option.h"
@@ -3459,6 +3460,9 @@ static QemuOptsList qemu_vnc_opts = {
         },{
             .name = "password",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "reverse",
             .type = QEMU_OPT_BOOL,
@@ -3931,6 +3935,7 @@ void vnc_display_open(const char *id, Error **errp)
     int lock_key_sync = 1;
     int key_delay_ms;
     const char *audiodev;
+    const char *passwordSecret;
 
     if (!vd) {
         error_setg(errp, "VNC display not active");
@@ -3948,7 +3953,23 @@ void vnc_display_open(const char *id, Error **errp)
         goto fail;
     }
 
-    password = qemu_opt_get_bool(opts, "password", false);
+
+    passwordSecret = qemu_opt_get(opts, "password-secret");
+    if (passwordSecret) {
+        if (qemu_opt_get(opts, "password")) {
+            error_setg(errp,
+                       "'password' flag is redundant with 'password-secret'");
+            goto fail;
+        }
+        vd->password = qcrypto_secret_lookup_as_utf8(passwordSecret,
+                                                     errp);
+        if (!vd->password) {
+            goto fail;
+        }
+        password = true;
+    } else {
+        password = qemu_opt_get_bool(opts, "password", false);
+    }
     if (password) {
         if (fips_get_state()) {
             error_setg(errp,
diff --git a/qemu-options.hx b/qemu-options.hx
index 622d3bfa5a7d..357fc4596ecc 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2165,6 +2165,11 @@ SRST
         time to allow <protocol> password to expire immediately or never
         expire.
 
+    ``password-secret=<secret-id>``
+        Require that password based authentication is used for client
+        connections, using the password provided by the ``secret``
+        object identified by ``secret-id``.
+
     ``tls-creds=ID``
         Provides the ID of a set of TLS credentials to use to secure the
         VNC server. They will apply to both the normal VNC server socket
-- 
2.29.2

[PULL 02/11] ui: introduce "password-secret" option for SPICE server

[Gerd Hoffmann]

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

Currently when using SPICE the "password" option provides the password
in plain text on the command line. This is insecure as it is visible
to all processes on the host. As an alternative, the password can be
provided separately via the monitor.

This introduces a "password-secret" option which lets the password be
provided up front.

  $QEMU --object secret,id=vncsec0,file=passwd.txt \
        --spice port=5901,password-secret=vncsec0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20210311114343.439820-3-berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-core.c | 30 ++++++++++++++++++++++++++++--
 qemu-options.hx |  9 +++++++--
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index cadec766fe3a..b9d8aced91df 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -34,6 +34,7 @@
 #include "qapi/qapi-events-ui.h"
 #include "qemu/notify.h"
 #include "qemu/option.h"
+#include "crypto/secret_common.h"
 #include "migration/misc.h"
 #include "hw/pci/pci_bus.h"
 #include "ui/spice-display.h"
@@ -415,6 +416,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "password",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "disable-ticketing",
             .type = QEMU_OPT_BOOL,
@@ -636,7 +640,9 @@ void qemu_spice_display_init_done(void)
 static void qemu_spice_init(void)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
-    const char *password, *str, *x509_dir, *addr,
+    char *password = NULL;
+    const char *passwordSecret;
+    const char *str, *x509_dir, *addr,
         *x509_key_password = NULL,
         *x509_dh_file = NULL,
         *tls_ciphers = NULL;
@@ -663,7 +669,26 @@ static void qemu_spice_init(void)
         error_report("spice tls-port is out of range");
         exit(1);
     }
-    password = qemu_opt_get(opts, "password");
+    passwordSecret = qemu_opt_get(opts, "password-secret");
+    if (passwordSecret) {
+        Error *local_err = NULL;
+        if (qemu_opt_get(opts, "password")) {
+            error_report("'password' option is mutually exclusive with "
+                         "'password-secret'");
+            exit(1);
+        }
+        password = qcrypto_secret_lookup_as_utf8(passwordSecret,
+                                                 &local_err);
+        if (!password) {
+            error_report_err(local_err);
+            exit(1);
+        }
+    } else {
+        str = qemu_opt_get(opts, "password");
+        if (str) {
+            password = g_strdup(str);
+        }
+    }
 
     if (tls_port) {
         x509_dir = qemu_opt_get(opts, "x509-dir");
@@ -809,6 +834,7 @@ static void qemu_spice_init(void)
     g_free(x509_key_file);
     g_free(x509_cert_file);
     g_free(x509_cacert_file);
+    g_free(password);
 
 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
diff --git a/qemu-options.hx b/qemu-options.hx
index 357fc4596ecc..a98f8e84a29d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1899,7 +1899,8 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,tls-ciphers=<list>]\n"
     "       [,tls-channel=[main|display|cursor|inputs|record|playback]]\n"
     "       [,plaintext-channel=[main|display|cursor|inputs|record|playback]]\n"
-    "       [,sasl=on|off][,password=<secret>][,disable-ticketing=on|off]\n"
+    "       [,sasl=on|off][,disable-ticketing=on|off]\n"
+    "       [,password=<string>][,password-secret=<secret-id>]\n"
     "       [,image-compression=[auto_glz|auto_lz|quic|glz|lz|off]]\n"
     "       [,jpeg-wan-compression=[auto|never|always]]\n"
     "       [,zlib-glz-wan-compression=[auto|never|always]]\n"
@@ -1924,9 +1925,13 @@ SRST
     ``ipv4=on|off``; \ ``ipv6=on|off``; \ ``unix=on|off``
         Force using the specified IP version.
 
-    ``password=<secret>``
+    ``password=<string>``
         Set the password you need to authenticate.
 
+    ``password-secret=<secret-id>``
+        Set the ID of the ``secret`` object containing the password
+        you need to authenticate.
+
     ``sasl=on|off``
         Require that the client use SASL to authenticate with the spice.
         The exact choice of authentication method used is controlled
-- 
2.29.2

[PULL 03/11] ui: deprecate "password" option for SPICE server

[Gerd Hoffmann]

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

With the new "password-secret" option, there is no reason to use the old
inecure "password" option with -spice, so it can be deprecated.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20210311114343.439820-4-berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/spice-core.c            | 2 ++
 docs/system/deprecated.rst | 8 ++++++++
 qemu-options.hx            | 4 ++++
 3 files changed, 14 insertions(+)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index b9d8aced91df..272d19b0c152 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -686,6 +686,8 @@ static void qemu_spice_init(void)
     } else {
         str = qemu_opt_get(opts, "password");
         if (str) {
+            warn_report("'password' option is deprecated and insecure, "
+                        "use 'password-secret' instead");
             password = g_strdup(str);
         }
     }
diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 5e3a31c12361..8cba672a7b4a 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -174,6 +174,14 @@ Input parameters that take a size value should only use a size suffix
 the value is hexadecimal.  That is, '0x20M' is deprecated, and should
 be written either as '32M' or as '0x2000000'.
 
+``-spice password=string`` (since 6.0)
+''''''''''''''''''''''''''''''''''''''
+
+This option is insecure because the SPICE password remains visible in
+the process listing. This is replaced by the new ``password-secret``
+option which lets the password be securely provided on the command
+line using a ``secret`` object instance.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/qemu-options.hx b/qemu-options.hx
index a98f8e84a29d..4da3f4f48c03 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1928,6 +1928,10 @@ SRST
     ``password=<string>``
         Set the password you need to authenticate.
 
+        This option is deprecated and insecure because it leaves the
+        password visible in the process listing. Use ``password-secret``
+        instead.
+
     ``password-secret=<secret-id>``
         Set the ID of the ``secret`` object containing the password
         you need to authenticate.
-- 
2.29.2

[PULL 04/11] opengl: Do not convert format with glTexImage2D on OpenGL ES

[Gerd Hoffmann]

From: Akihiko Odaki <akihiko.odaki@gmail.com>

OpenGL ES does not support conversion from the given data format
to the internal format with glTexImage2D.

Use the given data format as the internal format, and ignore
the given alpha channels with GL_TEXTURE_SWIZZLE_A in case the
format contains alpha channels.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Message-Id: <20210219094803.90860-1-akihiko.odaki@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console-gl.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/ui/console-gl.c b/ui/console-gl.c
index 0a6478161fed..7c9894a51d99 100644
--- a/ui/console-gl.c
+++ b/ui/console-gl.c
@@ -73,11 +73,20 @@ void surface_gl_create_texture(QemuGLShader *gls,
     glBindTexture(GL_TEXTURE_2D, surface->texture);
     glPixelStorei(GL_UNPACK_ROW_LENGTH_EXT,
                   surface_stride(surface) / surface_bytes_per_pixel(surface));
-    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
-                 surface_width(surface),
-                 surface_height(surface),
-                 0, surface->glformat, surface->gltype,
-                 surface_data(surface));
+    if (epoxy_is_desktop_gl()) {
+        glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB,
+                     surface_width(surface),
+                     surface_height(surface),
+                     0, surface->glformat, surface->gltype,
+                     surface_data(surface));
+    } else {
+        glTexImage2D(GL_TEXTURE_2D, 0, surface->glformat,
+                     surface_width(surface),
+                     surface_height(surface),
+                     0, surface->glformat, surface->gltype,
+                     surface_data(surface));
+        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_SWIZZLE_A, GL_ONE);
+    }
 
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
     glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
-- 
2.29.2

[PULL 05/11] ui/cocoa: Do not exit immediately after shutdown

[Gerd Hoffmann]

From: Akihiko Odaki <akihiko.odaki@gmail.com>

ui/cocoa used to call exit immediately after calling
qemu_system_shutdown_request, which prevents QEMU from actually
perfoming system shutdown. Just sleep forever, and wait QEMU to call
exit and kill the Cocoa thread.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Message-Id: <20210219111652.20623-1-akihiko.odaki@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/cocoa.m | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index a7848ae0a30e..9da0e884b712 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -1115,7 +1115,13 @@ QemuCocoaView *cocoaView;
     COCOA_DEBUG("QemuCocoaAppController: applicationWillTerminate\n");
 
     qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_UI);
-    exit(0);
+
+    /*
+     * Sleep here, because returning will cause OSX to kill us
+     * immediately; the QEMU main loop will handle the shutdown
+     * request and terminate the process.
+     */
+    [NSThread sleepForTimeInterval:INFINITY];
 }
 
 - (BOOL)applicationShouldTerminateAfterLastWindowClosed:(NSApplication *)theApplication
-- 
2.29.2

[PULL 06/11] ui: add more trace points for VNC client/server messages

[Gerd Hoffmann]

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

This adds trace points for desktop size and audio related messages.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20210311182957.486939-2-berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c        | 21 +++++++++++++++++++--
 ui/trace-events |  9 +++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index e8e3426a65c4..7291429c04cf 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -659,6 +659,9 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
 
 static void vnc_desktop_resize_ext(VncState *vs, int reject_reason)
 {
+    trace_vnc_msg_server_ext_desktop_resize(
+        vs, vs->ioc, vs->client_width, vs->client_height, reject_reason);
+
     vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
@@ -705,6 +708,9 @@ static void vnc_desktop_resize(VncState *vs)
         return;
     }
 
+    trace_vnc_msg_server_desktop_resize(
+        vs, vs->ioc, vs->client_width, vs->client_height);
+
     vnc_lock_output(vs);
     vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE);
     vnc_write_u8(vs, 0);
@@ -1182,6 +1188,7 @@ static void audio_capture_notify(void *opaque, audcnotification_e cmd)
     assert(vs->magic == VNC_MAGIC);
     switch (cmd) {
     case AUD_CNOTIFY_DISABLE:
+        trace_vnc_msg_server_audio_end(vs, vs->ioc);
         vnc_lock_output(vs);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
@@ -1191,6 +1198,7 @@ static void audio_capture_notify(void *opaque, audcnotification_e cmd)
         break;
 
     case AUD_CNOTIFY_ENABLE:
+        trace_vnc_msg_server_audio_begin(vs, vs->ioc);
         vnc_lock_output(vs);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
@@ -1210,6 +1218,7 @@ static void audio_capture(void *opaque, const void *buf, int size)
     VncState *vs = opaque;
 
     assert(vs->magic == VNC_MAGIC);
+    trace_vnc_msg_server_audio_data(vs, vs->ioc, buf, size);
     vnc_lock_output(vs);
     if (vs->output.offset < vs->throttle_output_offset) {
         vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
@@ -2454,9 +2463,11 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
 
             switch (read_u16 (data, 2)) {
             case VNC_MSG_CLIENT_QEMU_AUDIO_ENABLE:
+                trace_vnc_msg_client_audio_enable(vs, vs->ioc);
                 audio_add(vs);
                 break;
             case VNC_MSG_CLIENT_QEMU_AUDIO_DISABLE:
+                trace_vnc_msg_client_audio_disable(vs, vs->ioc);
                 audio_del(vs);
                 break;
             case VNC_MSG_CLIENT_QEMU_AUDIO_SET_FORMAT:
@@ -2492,6 +2503,8 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
                     break;
                 }
                 vs->as.freq = freq;
+                trace_vnc_msg_client_audio_format(
+                    vs, vs->ioc, vs->as.fmt, vs->as.nchannels, vs->as.freq);
                 break;
             default:
                 VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
@@ -2510,6 +2523,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
     {
         size_t size;
         uint8_t screens;
+        int w, h;
 
         if (len < 8) {
             return 8;
@@ -2520,12 +2534,15 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
         if (len < size) {
             return size;
         }
+        w = read_u16(data, 2);
+        h = read_u16(data, 4);
 
+        trace_vnc_msg_client_set_desktop_size(vs, vs->ioc, w, h, screens);
         if (dpy_ui_info_supported(vs->vd->dcl.con)) {
             QemuUIInfo info;
             memset(&info, 0, sizeof(info));
-            info.width = read_u16(data, 2);
-            info.height = read_u16(data, 4);
+            info.width = w;
+            info.height = h;
             dpy_set_ui_info(vs->vd->dcl.con, &info);
             vnc_desktop_resize_ext(vs, 4 /* Request forwarded */);
         } else {
diff --git a/ui/trace-events b/ui/trace-events
index 0ffcdb4408a6..bd8f8a9d186a 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -37,6 +37,15 @@ vnc_key_event_ext(bool down, int sym, int keycode, const char *name) "down %d, s
 vnc_key_event_map(bool down, int sym, int keycode, const char *name) "down %d, sym 0x%x -> keycode 0x%x [%s]"
 vnc_key_sync_numlock(bool on) "%d"
 vnc_key_sync_capslock(bool on) "%d"
+vnc_msg_server_audio_begin(void *state, void *ioc) "VNC server msg audio begin state=%p ioc=%p"
+vnc_msg_server_audio_end(void *state, void *ioc) "VNC server msg audio end state=%p ioc=%p"
+vnc_msg_server_audio_data(void *state, void *ioc, const void *buf, size_t len) "VNC server msg audio data state=%p ioc=%p buf=%p len=%zd"
+vnc_msg_server_desktop_resize(void *state, void *ioc, int width, int height) "VNC server msg ext resize state=%p ioc=%p size=%dx%d"
+vnc_msg_server_ext_desktop_resize(void *state, void *ioc, int width, int height, int reason) "VNC server msg ext resize state=%p ioc=%p size=%dx%d reason=%d"
+vnc_msg_client_audio_enable(void *state, void *ioc) "VNC client msg audio enable state=%p ioc=%p"
+vnc_msg_client_audio_disable(void *state, void *ioc) "VNC client msg audio disable state=%p ioc=%p"
+vnc_msg_client_audio_format(void *state, void *ioc, int fmt, int channels, int freq) "VNC client msg audio format state=%p ioc=%p fmt=%d channels=%d freq=%d"
+vnc_msg_client_set_desktop_size(void *state, void *ioc, int width, int height, int screens) "VNC client msg set desktop size  state=%p ioc=%p size=%dx%d screens=%d"
 vnc_client_eof(void *state, void *ioc) "VNC client EOF state=%p ioc=%p"
 vnc_client_io_error(void *state, void *ioc, const char *msg) "VNC client I/O error state=%p ioc=%p errmsg=%s"
 vnc_client_connect(void *state, void *ioc) "VNC client connect state=%p ioc=%p"
-- 
2.29.2

[PULL 07/11] ui: avoid sending framebuffer updates outside client desktop bounds

[Gerd Hoffmann]

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

We plan framebuffer update rects based on the VNC server surface. If the
client doesn't support desktop resize, then the client bounds may differ
from the server surface bounds. VNC clients may become upset if we then
send an update message outside the bounds of the client desktop.

This takes the approach of clamping the rectangles from the worker
thread immediately before sending them. This may sometimes results in
sending a framebuffer update message with zero rectangles.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20210311182957.486939-3-berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc-jobs.c   | 44 ++++++++++++++++++++++++++++++++++++++++----
 ui/trace-events |  5 +++++
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index dbbfbefe5619..4562bf89288e 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -32,6 +32,7 @@
 #include "qemu/sockets.h"
 #include "qemu/main-loop.h"
 #include "block/aio.h"
+#include "trace.h"
 
 /*
  * Locking:
@@ -94,6 +95,8 @@ int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h)
 {
     VncRectEntry *entry = g_new0(VncRectEntry, 1);
 
+    trace_vnc_job_add_rect(job->vs, job, x, y, w, h);
+
     entry->rect.x = x;
     entry->rect.y = y;
     entry->rect.w = w;
@@ -190,6 +193,8 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
     local->zlib = orig->zlib;
     local->hextile = orig->hextile;
     local->zrle = orig->zrle;
+    local->client_width = orig->client_width;
+    local->client_height = orig->client_height;
 }
 
 static void vnc_async_encoding_end(VncState *orig, VncState *local)
@@ -202,6 +207,34 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local)
     orig->lossy_rect = local->lossy_rect;
 }
 
+static bool vnc_worker_clamp_rect(VncState *vs, VncJob *job, VncRect *rect)
+{
+    trace_vnc_job_clamp_rect(vs, job, rect->x, rect->y, rect->w, rect->h);
+
+    if (rect->x >= vs->client_width) {
+        goto discard;
+    }
+    rect->w = MIN(vs->client_width - rect->x, rect->w);
+    if (rect->w == 0) {
+        goto discard;
+    }
+
+    if (rect->y >= vs->client_height) {
+        goto discard;
+    }
+    rect->h = MIN(vs->client_height - rect->y, rect->h);
+    if (rect->h == 0) {
+        goto discard;
+    }
+
+    trace_vnc_job_clamped_rect(vs, job, rect->x, rect->y, rect->w, rect->h);
+    return true;
+
+ discard:
+    trace_vnc_job_discard_rect(vs, job, rect->x, rect->y, rect->w, rect->h);
+    return false;
+}
+
 static int vnc_worker_thread_loop(VncJobQueue *queue)
 {
     VncJob *job;
@@ -260,14 +293,17 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
             goto disconnected;
         }
 
-        n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
-                                        entry->rect.w, entry->rect.h);
+        if (vnc_worker_clamp_rect(&vs, job, &entry->rect)) {
+            n = vnc_send_framebuffer_update(&vs, entry->rect.x, entry->rect.y,
+                                            entry->rect.w, entry->rect.h);
 
-        if (n >= 0) {
-            n_rectangles += n;
+            if (n >= 0) {
+                n_rectangles += n;
+            }
         }
         g_free(entry);
     }
+    trace_vnc_job_nrects(&vs, job, n_rectangles);
     vnc_unlock_display(job->vs->vd);
 
     /* Put n_rectangles at the beginning of the message */
diff --git a/ui/trace-events b/ui/trace-events
index bd8f8a9d186a..3838ae2d849a 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -59,6 +59,11 @@ vnc_client_throttle_audio(void *state, void *ioc, size_t offset) "VNC client thr
 vnc_client_unthrottle_forced(void *state, void *ioc) "VNC client unthrottle forced offset state=%p ioc=%p"
 vnc_client_unthrottle_incremental(void *state, void *ioc, size_t offset) "VNC client unthrottle incremental state=%p ioc=%p offset=%zu"
 vnc_client_output_limit(void *state, void *ioc, size_t offset, size_t threshold) "VNC client output limit state=%p ioc=%p offset=%zu threshold=%zu"
+vnc_job_add_rect(void *state, void *job, int x, int y, int w, int h) "VNC add rect state=%p job=%p offset=%d,%d size=%dx%d"
+vnc_job_discard_rect(void *state, void *job, int x, int y, int w, int h) "VNC job discard rect state=%p job=%p offset=%d,%d size=%dx%d"
+vnc_job_clamp_rect(void *state, void *job, int x, int y, int w, int h) "VNC job clamp rect state=%p job=%p offset=%d,%d size=%dx%d"
+vnc_job_clamped_rect(void *state, void *job, int x, int y, int w, int h) "VNC job clamp rect state=%p job=%p offset=%d,%d size=%dx%d"
+vnc_job_nrects(void *state, void *job, int nrects) "VNC job state=%p job=%p nrects=%d"
 vnc_auth_init(void *display, int websock, int auth, int subauth) "VNC auth init state=%p websock=%d auth=%d subauth=%d"
 vnc_auth_start(void *state, int method) "VNC client auth start state=%p method=%d"
 vnc_auth_pass(void *state, int method) "VNC client auth passed state=%p method=%d"
-- 
2.29.2

[PULL 08/11] ui: use client width/height in WMVi message

[Gerd Hoffmann]

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

The WMVi message is supposed to provide the same width/height
information as the regular desktop resize and extended desktop
resize messages. There can be times where the client width and
height are different from the pixman surface dimensions.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20210311182957.486939-4-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 7291429c04cf..8c9890b3cdc4 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2319,8 +2319,8 @@ static void vnc_colordepth(VncState *vs)
         vnc_write_u8(vs, 0);
         vnc_write_u16(vs, 1); /* number of rects */
         vnc_framebuffer_update(vs, 0, 0,
-                               pixman_image_get_width(vs->vd->server),
-                               pixman_image_get_height(vs->vd->server),
+                               vs->client_width,
+                               vs->client_height,
                                VNC_ENCODING_WMVi);
         pixel_format_message(vs);
         vnc_unlock_output(vs);
-- 
2.29.2

[PULL 09/11] ui: honour the actual guest display dimensions without rounding

[Gerd Hoffmann]

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

A long time ago the VNC server code had some memory corruption
fixes done in:

  commit bea60dd7679364493a0d7f5b54316c767cf894ef
  Author: Peter Lieven <pl@kamp.de>
  Date:   Mon Jun 30 10:57:51 2014 +0200

    ui/vnc: fix potential memory corruption issues

One of the implications of the fix was that the VNC server would have a
thin black bad down the right hand side if the guest desktop width was
not a multiple of 16. In practice this was a non-issue since the VNC
server was always honouring a guest specified resolution and guests
essentially always pick from a small set of sane resolutions likely in
real world hardware.

We recently introduced support for the extended desktop resize extension
and as a result the VNC client has ability to specify an arbitrary
desktop size and the guest OS may well honour it exactly. As a result we
no longer have any guarantee that the width will be a multiple of 16,
and so when resizing the desktop we have a 93% chance of getting the
black bar on the right hand size.

The VNC server maintains three different desktop dimensions

 1. The guest surface
 2. The server surface
 3. The client desktop

The requirement for the width to be a multiple of 16 only applies to
item 2, the server surface, for the purpose of doing dirty bitmap
tracking.

Normally we will set the client desktop size to always match the server
surface size, but that's not a strict requirement. In order to cope with
clients that don't support the desktop size encoding, we already allow
for the client desktop to be a different size that the server surface.

Thus we can trivially eliminate the black bar, but setting the client
desktop size to be the un-rounded server surface size - the so called
"true width".

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20210311182957.486939-5-berrange@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.h        |  1 +
 ui/vnc.c        | 23 +++++++++++++++++++----
 ui/trace-events |  2 ++
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index 116463d5f099..d4f3e1555809 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -164,6 +164,7 @@ struct VncDisplay
 
     struct VncSurface guest;   /* guest visible surface (aka ds->surface) */
     pixman_image_t *server;    /* vnc server surface */
+    int true_width; /* server surface width before rounding up */
 
     const char *id;
     QTAILQ_ENTRY(VncDisplay) next;
diff --git a/ui/vnc.c b/ui/vnc.c
index 8c9890b3cdc4..9c004a11f495 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -608,6 +608,11 @@ static int vnc_width(VncDisplay *vd)
                                        VNC_DIRTY_PIXELS_PER_BIT));
 }
 
+static int vnc_true_width(VncDisplay *vd)
+{
+    return MIN(VNC_MAX_WIDTH, surface_width(vd->ds));
+}
+
 static int vnc_height(VncDisplay *vd)
 {
     return MIN(VNC_MAX_HEIGHT, surface_height(vd->ds));
@@ -691,16 +696,16 @@ static void vnc_desktop_resize(VncState *vs)
                             !vnc_has_feature(vs, VNC_FEATURE_RESIZE_EXT))) {
         return;
     }
-    if (vs->client_width == pixman_image_get_width(vs->vd->server) &&
+    if (vs->client_width == vs->vd->true_width &&
         vs->client_height == pixman_image_get_height(vs->vd->server)) {
         return;
     }
 
-    assert(pixman_image_get_width(vs->vd->server) < 65536 &&
-           pixman_image_get_width(vs->vd->server) >= 0);
+    assert(vs->vd->true_width < 65536 &&
+           vs->vd->true_width >= 0);
     assert(pixman_image_get_height(vs->vd->server) < 65536 &&
            pixman_image_get_height(vs->vd->server) >= 0);
-    vs->client_width = pixman_image_get_width(vs->vd->server);
+    vs->client_width = vs->vd->true_width;
     vs->client_height = pixman_image_get_height(vs->vd->server);
 
     if (vnc_has_feature(vs, VNC_FEATURE_RESIZE_EXT)) {
@@ -774,6 +779,7 @@ static void vnc_update_server_surface(VncDisplay *vd)
 
     width = vnc_width(vd);
     height = vnc_height(vd);
+    vd->true_width = vnc_true_width(vd);
     vd->server = pixman_image_create_bits(VNC_SERVER_FB_FORMAT,
                                           width, height,
                                           NULL, 0);
@@ -809,13 +815,22 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
     vd->guest.fb = pixman_image_ref(surface->image);
     vd->guest.format = surface->format;
 
+
     if (pageflip) {
+        trace_vnc_server_dpy_pageflip(vd,
+                                      surface_width(surface),
+                                      surface_height(surface),
+                                      surface_format(surface));
         vnc_set_area_dirty(vd->guest.dirty, vd, 0, 0,
                            surface_width(surface),
                            surface_height(surface));
         return;
     }
 
+    trace_vnc_server_dpy_recreate(vd,
+                                  surface_width(surface),
+                                  surface_height(surface),
+                                  surface_format(surface));
     /* server surface */
     vnc_update_server_surface(vd);
 
diff --git a/ui/trace-events b/ui/trace-events
index 3838ae2d849a..5d1da6f23668 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -59,6 +59,8 @@ vnc_client_throttle_audio(void *state, void *ioc, size_t offset) "VNC client thr
 vnc_client_unthrottle_forced(void *state, void *ioc) "VNC client unthrottle forced offset state=%p ioc=%p"
 vnc_client_unthrottle_incremental(void *state, void *ioc, size_t offset) "VNC client unthrottle incremental state=%p ioc=%p offset=%zu"
 vnc_client_output_limit(void *state, void *ioc, size_t offset, size_t threshold) "VNC client output limit state=%p ioc=%p offset=%zu threshold=%zu"
+vnc_server_dpy_pageflip(void *dpy, int w, int h, int fmt) "VNC server dpy pageflip dpy=%p size=%dx%d fmt=%d"
+vnc_server_dpy_recreate(void *dpy, int w, int h, int fmt) "VNC server dpy recreate dpy=%p size=%dx%d fmt=%d"
 vnc_job_add_rect(void *state, void *job, int x, int y, int w, int h) "VNC add rect state=%p job=%p offset=%d,%d size=%dx%d"
 vnc_job_discard_rect(void *state, void *job, int x, int y, int w, int h) "VNC job discard rect state=%p job=%p offset=%d,%d size=%dx%d"
 vnc_job_clamp_rect(void *state, void *job, int x, int y, int w, int h) "VNC job clamp rect state=%p job=%p offset=%d,%d size=%dx%d"
-- 
2.29.2

[PULL 10/11] ui: fold qemu_alloc_display in only caller

[Gerd Hoffmann]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

A minor code simplification.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-Id: <20210312100108.2706195-2-marcandre.lureau@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/console.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index c2fdf975b6b3..2de5f4105b59 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1386,26 +1386,18 @@ static QemuConsole *new_console(DisplayState *ds, console_type_t console_type,
     return s;
 }
 
-static void qemu_alloc_display(DisplaySurface *surface, int width, int height)
+DisplaySurface *qemu_create_displaysurface(int width, int height)
 {
-    qemu_pixman_image_unref(surface->image);
-    surface->image = NULL;
+    DisplaySurface *surface = g_new0(DisplaySurface, 1);
 
+    trace_displaysurface_create(surface, width, height);
     surface->format = PIXMAN_x8r8g8b8;
     surface->image = pixman_image_create_bits(surface->format,
                                               width, height,
                                               NULL, width * 4);
     assert(surface->image != NULL);
-
     surface->flags = QEMU_ALLOCATED_FLAG;
-}
 
-DisplaySurface *qemu_create_displaysurface(int width, int height)
-{
-    DisplaySurface *surface = g_new0(DisplaySurface, 1);
-
-    trace_displaysurface_create(surface, width, height);
-    qemu_alloc_display(surface, width, height);
     return surface;
 }
 
-- 
2.29.2

[PULL 11/11] ui/cocoa: Comment about modifier key input quirks

[Gerd Hoffmann]

From: Akihiko Odaki <akihiko.odaki@gmail.com>

Based-on: <20210310042348.21931-1-akihiko.odaki@gmail.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
Message-Id: <20210312133212.3131-1-akihiko.odaki@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/cocoa.m | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index 9da0e884b712..37e1fb52eb4d 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -690,7 +690,43 @@ QemuCocoaView *cocoaView;
     NSPoint p = [self screenLocationOfEvent:event];
     NSUInteger modifiers = [event modifierFlags];
 
-    // emulate caps lock keydown and keyup
+    /*
+     * Check -[NSEvent modifierFlags] here.
+     *
+     * There is a NSEventType for an event notifying the change of
+     * -[NSEvent modifierFlags], NSEventTypeFlagsChanged but these operations
+     * are performed for any events because a modifier state may change while
+     * the application is inactive (i.e. no events fire) and we don't want to
+     * wait for another modifier state change to detect such a change.
+     *
+     * NSEventModifierFlagCapsLock requires a special treatment. The other flags
+     * are handled in similar manners.
+     *
+     * NSEventModifierFlagCapsLock
+     * ---------------------------
+     *
+     * If CapsLock state is changed, "up" and "down" events will be fired in
+     * sequence, effectively updates CapsLock state on the guest.
+     *
+     * The other flags
+     * ---------------
+     *
+     * If a flag is not set, fire "up" events for all keys which correspond to
+     * the flag. Note that "down" events are not fired here because the flags
+     * checked here do not tell what exact keys are down.
+     *
+     * If one of the keys corresponding to a flag is down, we rely on
+     * -[NSEvent keyCode] of an event whose -[NSEvent type] is
+     * NSEventTypeFlagsChanged to know the exact key which is down, which has
+     * the following two downsides:
+     * - It does not work when the application is inactive as described above.
+     * - It malfactions *after* the modifier state is changed while the
+     *   application is inactive. It is because -[NSEvent keyCode] does not tell
+     *   if the key is up or down, and requires to infer the current state from
+     *   the previous state. It is still possible to fix such a malfanction by
+     *   completely leaving your hands from the keyboard, which hopefully makes
+     *   this implementation usable enough.
+     */
     if (!!(modifiers & NSEventModifierFlagCapsLock) !=
         qkbd_state_modifier_get(kbd, QKBD_MOD_CAPSLOCK)) {
         qkbd_state_key_event(kbd, Q_KEY_CODE_CAPS_LOCK, true);
-- 
2.29.2

Re: [PULL 00/11] Ui 20210316 patches

[Peter Maydell]

On Tue, 16 Mar 2021 at 05:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> The following changes since commit 6157b0e19721aadb4c7fdcfe57b2924af6144b14:
>
>   Merge remote-tracking branch 'remotes/vivier2/tags/linux-user-for-6.0-pull-=
> request' into staging (2021-03-14 17:47:49 +0000)
>
> are available in the Git repository at:
>
>   git://git.kraxel.org/qemu tags/ui-20210316-pull-request
>
> for you to fetch changes up to ad7f2f8ee9fbded410fbf77158b0065f8e2f08e3:
>
>   ui/cocoa: Comment about modifier key input quirks (2021-03-16 06:36:45 +010=
> 0)
>
> ----------------------------------------------------------------
> vnc+spice: password-secret option.
> bugfixes for cocoa, vnc, opengl.
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/6.0
for any user-visible changes.

-- PMM