From 6c6840e9281cf2fd3b29d77f45b18949d4a83944 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 11 Mar 2021 11:43:41 +0000 Subject: [PATCH 01/11] ui: introduce "password-secret" option for VNC servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é Message-Id: <20210311114343.439820-2-berrange@redhat.com> Signed-off-by: Gerd Hoffmann --- qemu-options.hx | 5 +++++ ui/vnc.c | 23 ++++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/qemu-options.hx b/qemu-options.hx index 622d3bfa5a..357fc4596e 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2165,6 +2165,11 @@ SRST time to allow password to expire immediately or never expire. + ``password-secret=`` + 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 diff --git a/ui/vnc.c b/ui/vnc.c index 310abc9378..e8e3426a65 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, From 99522f69d62216f5d9581f66f2c0edca6bd48f78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 11 Mar 2021 11:43:42 +0000 Subject: [PATCH 02/11] ui: introduce "password-secret" option for SPICE server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é Message-Id: <20210311114343.439820-3-berrange@redhat.com> Signed-off-by: Gerd Hoffmann --- qemu-options.hx | 9 +++++++-- ui/spice-core.c | 30 ++++++++++++++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/qemu-options.hx b/qemu-options.hx index 357fc4596e..a98f8e84a2 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1899,7 +1899,8 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice, " [,tls-ciphers=]\n" " [,tls-channel=[main|display|cursor|inputs|record|playback]]\n" " [,plaintext-channel=[main|display|cursor|inputs|record|playback]]\n" - " [,sasl=on|off][,password=][,disable-ticketing=on|off]\n" + " [,sasl=on|off][,disable-ticketing=on|off]\n" + " [,password=][,password-secret=]\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=`` + ``password=`` Set the password you need to authenticate. + ``password-secret=`` + 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 diff --git a/ui/spice-core.c b/ui/spice-core.c index cadec766fe..b9d8aced91 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)) { From c47c0bcb33e154b82b4f6b90984aba998fcc4f18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 11 Mar 2021 11:43:43 +0000 Subject: [PATCH 03/11] ui: deprecate "password" option for SPICE server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é Message-Id: <20210311114343.439820-4-berrange@redhat.com> Signed-off-by: Gerd Hoffmann --- docs/system/deprecated.rst | 8 ++++++++ qemu-options.hx | 4 ++++ ui/spice-core.c | 2 ++ 3 files changed, 14 insertions(+) diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst index 5e3a31c123..8cba672a7b 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 a98f8e84a2..4da3f4f48c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -1928,6 +1928,10 @@ SRST ``password=`` 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=`` Set the ID of the ``secret`` object containing the password you need to authenticate. diff --git a/ui/spice-core.c b/ui/spice-core.c index b9d8aced91..272d19b0c1 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); } } From 14c235eb40eb82e0d7e89601b1a47028fe24deca Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 19 Feb 2021 18:48:03 +0900 Subject: [PATCH 04/11] opengl: Do not convert format with glTexImage2D on OpenGL ES 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 Message-Id: <20210219094803.90860-1-akihiko.odaki@gmail.com> Signed-off-by: Gerd Hoffmann --- 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 0a6478161f..7c9894a51d 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); From 40c0193739eb08f76505f736c259928279d0376a Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 19 Feb 2021 20:16:52 +0900 Subject: [PATCH 05/11] ui/cocoa: Do not exit immediately after shutdown 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 Message-Id: <20210219111652.20623-1-akihiko.odaki@gmail.com> Signed-off-by: Gerd Hoffmann --- ui/cocoa.m | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index a7848ae0a3..9da0e884b7 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 From adc8fce871afd30b4bf13cf5440a96a3ffb486db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 11 Mar 2021 18:29:54 +0000 Subject: [PATCH 06/11] ui: add more trace points for VNC client/server messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This adds trace points for desktop size and audio related messages. Signed-off-by: Daniel P. Berrangé Reviewed-by: Marc-André Lureau Message-Id: <20210311182957.486939-2-berrange@redhat.com> Signed-off-by: Gerd Hoffmann --- ui/trace-events | 9 +++++++++ ui/vnc.c | 21 +++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/ui/trace-events b/ui/trace-events index 0ffcdb4408..bd8f8a9d18 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" diff --git a/ui/vnc.c b/ui/vnc.c index e8e3426a65..7291429c04 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 { From 55b400497cf9c79acbb5c01abc58737bc52c081c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 11 Mar 2021 18:29:55 +0000 Subject: [PATCH 07/11] ui: avoid sending framebuffer updates outside client desktop bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é Reviewed-by: Marc-André Lureau Message-Id: <20210311182957.486939-3-berrange@redhat.com> Signed-off-by: Gerd Hoffmann --- ui/trace-events | 5 +++++ ui/vnc-jobs.c | 44 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/ui/trace-events b/ui/trace-events index bd8f8a9d18..3838ae2d84 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" diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c index dbbfbefe56..4562bf8928 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 */ From 3d3a528da4215a55f6557ad0925507680da7ceb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 11 Mar 2021 18:29:56 +0000 Subject: [PATCH 08/11] ui: use client width/height in WMVi message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é Reviewed-by: Marc-André Lureau Message-Id: <20210311182957.486939-4-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 7291429c04..8c9890b3cd 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); From 69cc8db44bdf7c9289e1fd1f695e01ec6132bf2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 11 Mar 2021 18:29:57 +0000 Subject: [PATCH 09/11] ui: honour the actual guest display dimensions without rounding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A long time ago the VNC server code had some memory corruption fixes done in: commit bea60dd7679364493a0d7f5b54316c767cf894ef Author: Peter Lieven 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é Reviewed-by: Marc-André Lureau Message-Id: <20210311182957.486939-5-berrange@redhat.com> Signed-off-by: Gerd Hoffmann --- ui/trace-events | 2 ++ ui/vnc.c | 23 +++++++++++++++++++---- ui/vnc.h | 1 + 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/ui/trace-events b/ui/trace-events index 3838ae2d84..5d1da6f236 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" diff --git a/ui/vnc.c b/ui/vnc.c index 8c9890b3cd..9c004a11f4 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/vnc.h b/ui/vnc.h index 116463d5f0..d4f3e15558 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; From eb69442a06ea3be6af294c9db0e66e277a529a27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 12 Mar 2021 14:00:42 +0400 Subject: [PATCH 10/11] ui: fold qemu_alloc_display in only caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A minor code simplification. Signed-off-by: Marc-André Lureau Message-Id: <20210312100108.2706195-2-marcandre.lureau@redhat.com> Signed-off-by: Gerd Hoffmann --- ui/console.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/ui/console.c b/ui/console.c index c2fdf975b6..2de5f4105b 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) -{ - qemu_pixman_image_unref(surface->image); - surface->image = NULL; - - 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); + 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; + return surface; } From ad7f2f8ee9fbded410fbf77158b0065f8e2f08e3 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 12 Mar 2021 22:32:12 +0900 Subject: [PATCH 11/11] ui/cocoa: Comment about modifier key input quirks Based-on: <20210310042348.21931-1-akihiko.odaki@gmail.com> Signed-off-by: Akihiko Odaki Message-Id: <20210312133212.3131-1-akihiko.odaki@gmail.com> Signed-off-by: Gerd Hoffmann --- ui/cocoa.m | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/ui/cocoa.m b/ui/cocoa.m index 9da0e884b7..37e1fb52eb 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);