[PATCH 0/2] ui: fix VNC endian regression & improve tracing

[PATCH 0/2] ui: fix VNC endian regression & improve tracing

[Daniel P. Berrangé]

Daniel P. Berrangé (2):
  ui: fix setting client_endian field defaults
  ui: add trace events for all client messages

 ui/trace-events | 14 +++++++++++++
 ui/vnc.c        | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

-- 
2.49.0

[PATCH 1/2] ui: fix setting client_endian field defaults

[Daniel P. Berrangé]

When a VNC client sends a "set pixel format" message, the
'client_endian' field will get initialized, however, it is
valid to omit this message if the client wants to use the
server's native pixel format. In the latter scenario nothing
is initializing the 'client_endian' field, so it remains set
to 0, matching neither G_LITTLE_ENDIAN nor G_BIG_ENDIAN. This
then results in pixel format conversion routines taking the
wrong code paths.

This problem existed before the 'client_be' flag was changed
into the 'client_endian' value, but the lack of initialization
meant it semantically defaulted to little endian, so only big
endian systems would potentially be exposed to incorrect pixel
translation.

The 'virt-viewer' / 'remote-viewer' apps always send a "set
pixel format" message so aren't exposed to any problems, but
the classical 'vncviewer' app will show the problem easily.

Fixes: 7ed96710e82c385c6cfc3d064eec7dde20f0f3fd
Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ui/vnc.c b/ui/vnc.c
index d095cd7da3..51872d1838 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2329,6 +2329,7 @@ static void pixel_format_message (VncState *vs) {
     char pad[3] = { 0, 0, 0 };
 
     vs->client_pf = qemu_default_pixelformat(32);
+    vs->client_endian = G_BYTE_ORDER;
 
     vnc_write_u8(vs, vs->client_pf.bits_per_pixel); /* bits-per-pixel */
     vnc_write_u8(vs, vs->client_pf.depth); /* depth */
-- 
2.49.0

[PATCH 2/2] ui: add trace events for all client messages

[Daniel P. Berrangé]

This lets us see the full flow of RFB messages received from the
client.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/trace-events | 14 +++++++++++++
 ui/vnc.c        | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/ui/trace-events b/ui/trace-events
index 3da0d5e280..3eba9ca3a8 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -48,13 +48,27 @@ vnc_msg_server_ext_desktop_resize(void *state, void *ioc, int width, int height,
 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_cut_text(void *state, void *ioc, int len) "VNC client msg cut text state=%p ioc=%p len=%u"
+vnc_msg_client_cut_text_ext(void *state, void *ioc, int len, int flags) "VNC client msg cut text state=%p ioc=%p len=%u flags=%u"
+vnc_msg_client_ext_key_event(void *state, void *ioc, int down, int sym, int keycode) "VNC client msg ext key event state=%p ioc=%p down=%u sym=%u keycode=%u"
+vnc_msg_client_framebuffer_update_request(void *state, void *ioc, int incremental, int x, int y, int w, int h) "VNC client msg framebuffer update request state=%p ioc=%p incremental=%u x=%u y=%u w=%u h=%u"
+vnc_msg_client_key_event(void *state, void *ioc, int down, int sym) "VNC client msg key event state=%p ioc=%p down=%u sym=%u"
+vnc_msg_client_pointer_event(void *state, void *ioc, int button_mask, int x, int y) "VNC client msg pointer event state=%p ioc=%p button_mask=%u x=%u y=%u"
 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_msg_client_set_encodings(void *state, void *ioc, int limit) "VNC client msg set encodings state=%p ioc=%p limit=%u"
+vnc_msg_client_set_pixel_format(void *state, void *ioc, int bpp, int big_endian, int true_color) "VNC client msg set pixel format state=%p ioc=%p bpp=%u big_endian=%u true_color=%u"
+vnc_msg_client_set_pixel_format_rgb(void *state, void *ioc, int red_max, int green_max, int blue_max, int red_shift, int green_shift, int blue_shift) "VNC client msg set pixel format RGB state=%p ioc=%p red_max=%u green_max=%u blue_max=%u red_shift=%u green_shift=%u blue_shift=%u"
+vnc_msg_client_xvp(void *state, void *ioc, int version, int action) "VNC client msg XVP state=%p ioc=%p version=%u action=%u"
 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"
 vnc_client_disconnect_start(void *state, void *ioc) "VNC client disconnect start state=%p ioc=%p"
 vnc_client_disconnect_finish(void *state, void *ioc) "VNC client disconnect finish state=%p ioc=%p"
 vnc_client_io_wrap(void *state, void *ioc, const char *type) "VNC client I/O wrap state=%p ioc=%p type=%s"
+vnc_client_pixel_format(void *state, void *ioc, int bpp, int depth, int endian) "VNC client pixel format state=%p ioc=%p bpp=%u depth=%u endian=%u"
+vnc_client_pixel_format_red(void *state, void *ioc, int max, int bits, int shift, int mask) "VNC client pixel format red state=%p ioc=%p max=%u bits=%u shift=%u mask=%u"
+vnc_client_pixel_format_green(void *state, void *ioc, int max, int bits, int shift, int mask) "VNC client pixel format green state=%p ioc=%p max=%u bits=%u shift=%u mask=%u"
+vnc_client_pixel_format_blue(void *state, void *ioc, int max, int bits, int shift, int mask) "VNC client pixel format blue state=%p ioc=%p max=%u bits=%u shift=%u mask=%u"
 vnc_client_throttle_threshold(void *state, void *ioc, size_t oldoffset, size_t offset, int client_width, int client_height, int bytes_per_pixel, void *audio_cap) "VNC client throttle threshold state=%p ioc=%p oldoffset=%zu newoffset=%zu width=%d height=%d bpp=%d audio=%p"
 vnc_client_throttle_incremental(void *state, void *ioc, int job_update, size_t offset) "VNC client throttle incremental state=%p ioc=%p job-update=%d offset=%zu"
 vnc_client_throttle_forced(void *state, void *ioc, int job_update, size_t offset) "VNC client throttle forced state=%p ioc=%p job-update=%d offset=%zu"
diff --git a/ui/vnc.c b/ui/vnc.c
index 51872d1838..af173b8033 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2314,6 +2314,25 @@ static void set_pixel_format(VncState *vs, int bits_per_pixel,
     vs->client_pf.bytes_per_pixel = bits_per_pixel / 8;
     vs->client_pf.depth = bits_per_pixel == 32 ? 24 : bits_per_pixel;
     vs->client_endian = big_endian_flag ? G_BIG_ENDIAN : G_LITTLE_ENDIAN;
+    trace_vnc_client_pixel_format(vs, vs->ioc,
+                                  vs->client_pf.bits_per_pixel,
+                                  vs->client_pf.depth,
+                                  vs->client_endian);
+    trace_vnc_client_pixel_format_red(vs, vs->ioc,
+                                      vs->client_pf.rmax,
+                                      vs->client_pf.rbits,
+                                      vs->client_pf.rshift,
+                                      vs->client_pf.rmask);
+    trace_vnc_client_pixel_format_green(vs, vs->ioc,
+                                        vs->client_pf.gmax,
+                                        vs->client_pf.gbits,
+                                        vs->client_pf.gshift,
+                                        vs->client_pf.gmask);
+    trace_vnc_client_pixel_format_blue(vs, vs->ioc,
+                                       vs->client_pf.bmax,
+                                       vs->client_pf.bbits,
+                                       vs->client_pf.bshift,
+                                       vs->client_pf.bmask);
 
     if (!true_color_flag) {
         send_color_map(vs);
@@ -2388,6 +2407,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
         if (len == 1)
             return 20;
 
+        trace_vnc_msg_client_set_pixel_format(vs, vs->ioc,
+                                              read_u8(data, 4),
+                                              read_u8(data, 6),
+                                              read_u8(data, 7));
+        trace_vnc_msg_client_set_pixel_format_rgb(vs, vs->ioc,
+                                                  read_u16(data, 8),
+                                                  read_u16(data, 10),
+                                                  read_u16(data, 12),
+                                                  read_u8(data, 14),
+                                                  read_u8(data, 15),
+                                                  read_u8(data, 16));
         set_pixel_format(vs, read_u8(data, 4),
                          read_u8(data, 6), read_u8(data, 7),
                          read_u16(data, 8), read_u16(data, 10),
@@ -2410,12 +2440,19 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
             memcpy(data + 4 + (i * 4), &val, sizeof(val));
         }
 
+        trace_vnc_msg_client_set_encodings(vs, vs->ioc, limit);
         set_encodings(vs, (int32_t *)(data + 4), limit);
         break;
     case VNC_MSG_CLIENT_FRAMEBUFFER_UPDATE_REQUEST:
         if (len == 1)
             return 10;
 
+        trace_vnc_msg_client_framebuffer_update_request(vs, vs->ioc,
+                                                        read_u8(data, 1),
+                                                        read_u16(data, 2),
+                                                        read_u16(data, 4),
+                                                        read_u16(data, 6),
+                                                        read_u16(data, 8));
         framebuffer_update_request(vs,
                                    read_u8(data, 1), read_u16(data, 2), read_u16(data, 4),
                                    read_u16(data, 6), read_u16(data, 8));
@@ -2424,12 +2461,19 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
         if (len == 1)
             return 8;
 
+        trace_vnc_msg_client_key_event(vs, vs->ioc,
+                                       read_u8(data, 1),
+                                       read_u32(data, 4));
         key_event(vs, read_u8(data, 1), read_u32(data, 4));
         break;
     case VNC_MSG_CLIENT_POINTER_EVENT:
         if (len == 1)
             return 6;
 
+        trace_vnc_msg_client_pointer_event(vs, vs->ioc,
+                                           read_u8(data, 1),
+                                           read_u16(data, 2),
+                                           read_u16(data, 4));
         pointer_event(vs, read_u8(data, 1), read_u16(data, 2), read_u16(data, 4));
         break;
     case VNC_MSG_CLIENT_CUT_TEXT:
@@ -2461,9 +2505,12 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
                 vnc_client_error(vs);
                 break;
             }
+            trace_vnc_msg_client_cut_text_ext(vs, vs->ioc,
+                                              dlen, read_u32(data, 8));
             vnc_client_cut_text_ext(vs, dlen, read_u32(data, 8), data + 12);
             break;
         }
+        trace_vnc_msg_client_cut_text(vs, vs->ioc, read_u32(data, 4));
         vnc_client_cut_text(vs, read_u32(data, 4), data + 8);
         break;
     case VNC_MSG_CLIENT_XVP:
@@ -2478,6 +2525,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
         if (len == 4) {
             uint8_t version = read_u8(data, 2);
             uint8_t action = read_u8(data, 3);
+            trace_vnc_msg_client_xvp(vs, vs->ioc, version, action);
 
             if (version != 1) {
                 error_report("vnc: xvp client message version %d != 1",
@@ -2511,6 +2559,10 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
             if (len == 2)
                 return 12;
 
+            trace_vnc_msg_client_ext_key_event(vs, vs->ioc,
+                                               read_u16(data, 2),
+                                               read_u32(data, 4),
+                                               read_u32(data, 8));
             ext_key_event(vs, read_u16(data, 2),
                           read_u32(data, 4), read_u32(data, 8));
             break;
-- 
2.49.0

Re: [PATCH 0/2] ui: fix VNC endian regression & improve tracing

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]

On Wed, Jun 4, 2025 at 8:22 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

>
>
> Daniel P. Berrangé (2):
>   ui: fix setting client_endian field defaults
>   ui: add trace events for all client messages
>
>  ui/trace-events | 14 +++++++++++++
>  ui/vnc.c        | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
>

Series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 1070 bytes --]

Re: [PATCH 0/2] ui: fix VNC endian regression & improve tracing

[Philippe Mathieu-Daudé]

On 4/6/25 18:22, Daniel P. Berrangé wrote:

> Daniel P. Berrangé (2):
>    ui: fix setting client_endian field defaults
>    ui: add trace events for all client messages

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/2] ui: fix VNC endian regression & improve tracing

[Michael Tokarev]

On 04.06.2025 19:22, Daniel P. Berrangé wrote:
> 
> Daniel P. Berrangé (2):
>    ui: fix setting client_endian field defaults
>    ui: add trace events for all client messages
> 
>   ui/trace-events | 14 +++++++++++++
>   ui/vnc.c        | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 67 insertions(+)

Hi!

Has this series been forgotten?  It seems it should be in 10.1
and in 10.0.3.   It has 2 R-Bs.  Is it time to send a PullReq?

Thanks,

/mjt

Re: [PATCH 0/2] ui: fix VNC endian regression & improve tracing

[Daniel P. Berrangé]

On Sat, Jul 12, 2025 at 11:26:29AM +0300, Michael Tokarev wrote:
> On 04.06.2025 19:22, Daniel P. Berrangé wrote:
> > 
> > Daniel P. Berrangé (2):
> >    ui: fix setting client_endian field defaults
> >    ui: add trace events for all client messages
> > 
> >   ui/trace-events | 14 +++++++++++++
> >   ui/vnc.c        | 53 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 67 insertions(+)
> 
> Hi!
> 
> Has this series been forgotten?  It seems it should be in 10.1
> and in 10.0.3.   It has 2 R-Bs.  Is it time to send a PullReq?

I have it pending a PR

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|