[PATCH v2 0/3] ui/vnc: fix some endian problems

[PATCH v2 0/3] ui/vnc: fix some endian problems

[Daniel P. Berrangé]

This fixes some edge cases in endian handling in the VNC server. These
bugs are rarely going to be visible by default, since most servers will
negotiate encoding formats / framebuffer formats that avoid hitting
the problem scenarios.

In v2:

 - Rename 'client_bo' to 'client_endian' for reviewer clarity
 - Rename 'native' to 'native_endian' for code consistency

Daniel P. Berrangé (3):
  ui/vnc.c: replace big endian flag with byte order value
  ui/vnc: take account of client byte order in pixman format
  ui/vnc: fix tight palette pixel encoding for 8/16-bpp formats

 include/ui/qemu-pixman.h |  4 ++--
 ui/qemu-pixman.c         | 15 ++++++++-------
 ui/vnc-enc-tight.c       | 16 ++++++++++------
 ui/vnc-enc-zrle.c        |  2 +-
 ui/vnc-jobs.c            |  2 +-
 ui/vnc.c                 |  9 +++++----
 ui/vnc.h                 |  2 +-
 7 files changed, 28 insertions(+), 22 deletions(-)

-- 
2.49.0

[PATCH v2 1/3] ui/vnc.c: replace big endian flag with byte order value

[Daniel P. Berrangé]

It will make it easier to do certain comparisons in future if we
store G_BIG_ENDIAN/G_LITTLE_ENDIAN directly, instead of a boolean
flag, as we can then compare directly to the G_BYTE_ORDER constant.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-enc-tight.c | 2 +-
 ui/vnc-enc-zrle.c  | 2 +-
 ui/vnc-jobs.c      | 2 +-
 ui/vnc.c           | 6 +++---
 ui/vnc.h           | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index 41f559eb83..f8aaa8f346 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -150,7 +150,7 @@ tight_detect_smooth_image24(VncState *vs, int w, int h)
      * If client is big-endian, color samples begin from the second
      * byte (offset 1) of a 32-bit pixel value.
      */
-    off = vs->client_be;
+    off = vs->client_endian == G_BIG_ENDIAN ? 1 : 0;
 
     memset(stats, 0, sizeof (stats));
 
diff --git a/ui/vnc-enc-zrle.c b/ui/vnc-enc-zrle.c
index bd33b89063..97ec6c7119 100644
--- a/ui/vnc-enc-zrle.c
+++ b/ui/vnc-enc-zrle.c
@@ -255,7 +255,7 @@ static void zrle_write_u8(VncState *vs, uint8_t value)
 static int zrle_send_framebuffer_update(VncState *vs, int x, int y,
                                         int w, int h)
 {
-    bool be = vs->client_be;
+    bool be = vs->client_endian == G_BIG_ENDIAN;
     size_t bytes;
     int zywrle_level;
 
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index fcca7ec632..d3486af9e2 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -188,7 +188,7 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
     local->lossy_rect = orig->lossy_rect;
     local->write_pixels = orig->write_pixels;
     local->client_pf = orig->client_pf;
-    local->client_be = orig->client_be;
+    local->client_endian = orig->client_endian;
     local->tight = orig->tight;
     local->zlib = orig->zlib;
     local->hextile = orig->hextile;
diff --git a/ui/vnc.c b/ui/vnc.c
index 9e097dc4b4..ab18172c4d 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -891,7 +891,7 @@ void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
         buf[0] = v;
         break;
     case 2:
-        if (vs->client_be) {
+        if (vs->client_endian == G_BIG_ENDIAN) {
             buf[0] = v >> 8;
             buf[1] = v;
         } else {
@@ -901,7 +901,7 @@ void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v)
         break;
     default:
     case 4:
-        if (vs->client_be) {
+        if (vs->client_endian == G_BIG_ENDIAN) {
             buf[0] = v >> 24;
             buf[1] = v >> 16;
             buf[2] = v >> 8;
@@ -2312,7 +2312,7 @@ static void set_pixel_format(VncState *vs, int bits_per_pixel,
     vs->client_pf.bits_per_pixel = 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_be = big_endian_flag;
+    vs->client_endian = big_endian_flag ? G_BIG_ENDIAN : G_LITTLE_ENDIAN;
 
     if (!true_color_flag) {
         send_color_map(vs);
diff --git a/ui/vnc.h b/ui/vnc.h
index acc53a2cc1..02613aa63a 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -323,7 +323,7 @@ struct VncState
     VncWritePixels *write_pixels;
     PixelFormat client_pf;
     pixman_format_code_t client_format;
-    bool client_be;
+    int client_endian; /* G_LITTLE_ENDIAN or G_BIG_ENDIAN */
 
     CaptureVoiceOut *audio_cap;
     struct audsettings as;
-- 
2.49.0

[PATCH v2 2/3] ui/vnc: take account of client byte order in pixman format

[Daniel P. Berrangé]

The set_pixel_conversion() method is responsible for determining whether
the VNC client pixel format matches the server format, and thus whether
we can use the fast path "copy" impl for sending pixels, or must use
the generic impl with bit swizzling.

The VNC server format is set at build time to VNC_SERVER_FB_FORMAT,
which corresponds to PIXMAN_x8r8g8b8.

The qemu_pixman_get_format() method is then responsible for converting
the VNC pixel format into a pixman format.

The VNC client pixel shifts are relative to the associated endianness.

The pixman formats are always relative to the host native endianness.

The qemu_pixman_get_format() method does not take into account the
VNC client endianness, and is thus returning a pixman format that is
only valid with the host endianness matches that of the VNC client.

This has been broken since pixman was introduced to the VNC server:

  commit 9f64916da20eea67121d544698676295bbb105a7
  Author: Gerd Hoffmann <kraxel@redhat.com>
  Date:   Wed Oct 10 13:29:43 2012 +0200

    pixman/vnc: use pixman images in vnc.

The flaw can be demonstrated using the Tigervnc client by using

   vncviewer -AutoSelect=0 -PreferredEncoding=raw server:display

connecting from a LE client to a QEMU on a BE server, or the
reverse.

The bug was masked, however, because almost all VNC clients will
advertize support for the "tight" encoding and the QEMU VNC server
will prefer "tight" if advertized.

The tight_pack24 method is responsible for taking a set of pixels
which have already been converted into client endianness and then
repacking them into the TPIXEL format which the RFB spec defines
as

  "TPIXEL is only 3 bytes long, where the first byte is the
   red component, the second byte is the green component,
   and the third byte is the blue component of the pixel
   color value"

IOW, the TPIXEL format is fixed on the wire, regardless of what
the VNC client declare as its endianness.

Since the VNC pixel encoding code was failing to honour the endian
flag of the client, the tight_pack24 method was always operating
on data in native endianness. Its impl cancelled out the VNC pixel
encoding bug.

With the VNC pixel encoding code now fixed, the tight_pack24 method
needs to take into account that it is operating on data in client
endianness, not native endianness. It thus may need to invert the
pixel shifts.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/ui/qemu-pixman.h |  4 ++--
 ui/qemu-pixman.c         | 15 ++++++++-------
 ui/vnc-enc-tight.c       |  2 +-
 ui/vnc.c                 |  3 ++-
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/ui/qemu-pixman.h b/include/ui/qemu-pixman.h
index 193bc046d1..2ca0ed7029 100644
--- a/include/ui/qemu-pixman.h
+++ b/include/ui/qemu-pixman.h
@@ -75,12 +75,12 @@ PixelFormat qemu_pixelformat_from_pixman(pixman_format_code_t format);
 pixman_format_code_t qemu_default_pixman_format(int bpp, bool native_endian);
 pixman_format_code_t qemu_drm_format_to_pixman(uint32_t drm_format);
 uint32_t qemu_pixman_to_drm_format(pixman_format_code_t pixman);
-int qemu_pixman_get_type(int rshift, int gshift, int bshift);
+int qemu_pixman_get_type(int rshift, int gshift, int bshift, int endian);
 bool qemu_pixman_check_format(DisplayChangeListener *dcl,
                               pixman_format_code_t format);
 
 #ifdef CONFIG_PIXMAN
-pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf);
+pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf, int endian);
 pixman_image_t *qemu_pixman_linebuf_create(pixman_format_code_t format,
                                            int width);
 void qemu_pixman_linebuf_fill(pixman_image_t *linebuf, pixman_image_t *fb,
diff --git a/ui/qemu-pixman.c b/ui/qemu-pixman.c
index 6ef4376f4e..ef4e71da11 100644
--- a/ui/qemu-pixman.c
+++ b/ui/qemu-pixman.c
@@ -126,33 +126,34 @@ uint32_t qemu_pixman_to_drm_format(pixman_format_code_t pixman_format)
     return 0;
 }
 
-int qemu_pixman_get_type(int rshift, int gshift, int bshift)
+int qemu_pixman_get_type(int rshift, int gshift, int bshift, int endian)
 {
     int type = PIXMAN_TYPE_OTHER;
+    bool native_endian = (endian == G_BYTE_ORDER);
 
     if (rshift > gshift && gshift > bshift) {
         if (bshift == 0) {
-            type = PIXMAN_TYPE_ARGB;
+            type = native_endian ? PIXMAN_TYPE_ARGB : PIXMAN_TYPE_BGRA;
         } else {
-            type = PIXMAN_TYPE_RGBA;
+            type = native_endian ? PIXMAN_TYPE_RGBA : PIXMAN_TYPE_ABGR;
         }
     } else if (rshift < gshift && gshift < bshift) {
         if (rshift == 0) {
-            type = PIXMAN_TYPE_ABGR;
+            type = native_endian ? PIXMAN_TYPE_ABGR : PIXMAN_TYPE_RGBA;
         } else {
-            type = PIXMAN_TYPE_BGRA;
+            type = native_endian ? PIXMAN_TYPE_BGRA : PIXMAN_TYPE_ARGB;
         }
     }
     return type;
 }
 
 #ifdef CONFIG_PIXMAN
-pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf)
+pixman_format_code_t qemu_pixman_get_format(PixelFormat *pf, int endian)
 {
     pixman_format_code_t format;
     int type;
 
-    type = qemu_pixman_get_type(pf->rshift, pf->gshift, pf->bshift);
+    type = qemu_pixman_get_type(pf->rshift, pf->gshift, pf->bshift, endian);
     format = PIXMAN_FORMAT(pf->bits_per_pixel, type,
                            pf->abits, pf->rbits, pf->gbits, pf->bbits);
     if (!pixman_format_supported_source(format)) {
diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index f8aaa8f346..a5bdc19ebb 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -891,7 +891,7 @@ static void tight_pack24(VncState *vs, uint8_t *buf, size_t count, size_t *ret)
 
     buf8 = buf;
 
-    if (1 /* FIXME */) {
+    if (vs->client_endian == G_BYTE_ORDER) {
         rshift = vs->client_pf.rshift;
         gshift = vs->client_pf.gshift;
         bshift = vs->client_pf.bshift;
diff --git a/ui/vnc.c b/ui/vnc.c
index ab18172c4d..d095cd7da3 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2240,7 +2240,8 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
 
 static void set_pixel_conversion(VncState *vs)
 {
-    pixman_format_code_t fmt = qemu_pixman_get_format(&vs->client_pf);
+    pixman_format_code_t fmt = qemu_pixman_get_format(&vs->client_pf,
+                                                      vs->client_endian);
 
     if (fmt == VNC_SERVER_FB_FORMAT) {
         vs->write_pixels = vnc_write_pixels_copy;
-- 
2.49.0

[PATCH v2 3/3] ui/vnc: fix tight palette pixel encoding for 8/16-bpp formats

[Daniel P. Berrangé]

When sending a tight rectangle with the palette filter, if the client
format was 8/16bpp, the colours on big endian hosts are not set as
we're sending the wrong bytes. We must first cast the 32-bit colour
to a 16/8-bit value, and then send the result.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 ui/vnc-enc-tight.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/ui/vnc-enc-tight.c b/ui/vnc-enc-tight.c
index a5bdc19ebb..705dcbb429 100644
--- a/ui/vnc-enc-tight.c
+++ b/ui/vnc-enc-tight.c
@@ -1001,13 +1001,17 @@ static int send_mono_rect(VncState *vs, int x, int y,
         break;
     }
     case 2:
-        vnc_write(vs, &bg, 2);
-        vnc_write(vs, &fg, 2);
+        uint16_t bg16 = bg;
+        uint16_t fg16 = fg;
+        vnc_write(vs, &bg16, 2);
+        vnc_write(vs, &fg16, 2);
         tight_encode_mono_rect16(vs->tight->tight.buffer, w, h, bg, fg);
         break;
     default:
-        vnc_write_u8(vs, bg);
-        vnc_write_u8(vs, fg);
+        uint8_t bg8 = bg;
+        uint8_t fg8 = fg;
+        vnc_write_u8(vs, bg8);
+        vnc_write_u8(vs, fg8);
         tight_encode_mono_rect8(vs->tight->tight.buffer, w, h, bg, fg);
         break;
     }
-- 
2.49.0

Re: [PATCH v2 1/3] ui/vnc.c: replace big endian flag with byte order value

[BALATON Zoltan]

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

On Wed, 14 May 2025, Daniel P. Berrangé wrote:
> It will make it easier to do certain comparisons in future if we
> store G_BIG_ENDIAN/G_LITTLE_ENDIAN directly, instead of a boolean
> flag, as we can then compare directly to the G_BYTE_ORDER constant.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Re: [PATCH v2 1/3] ui/vnc.c: replace big endian flag with byte order value

[Philippe Mathieu-Daudé]

On 14/5/25 13:19, Daniel P. Berrangé wrote:
> It will make it easier to do certain comparisons in future if we
> store G_BIG_ENDIAN/G_LITTLE_ENDIAN directly, instead of a boolean
> flag, as we can then compare directly to the G_BYTE_ORDER constant.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   ui/vnc-enc-tight.c | 2 +-
>   ui/vnc-enc-zrle.c  | 2 +-
>   ui/vnc-jobs.c      | 2 +-
>   ui/vnc.c           | 6 +++---
>   ui/vnc.h           | 2 +-
>   5 files changed, 7 insertions(+), 7 deletions(-)

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

Re: [PATCH v2 0/3] ui/vnc: fix some endian problems

[Michael Tokarev]

On 14.05.2025 14:19, Daniel P. Berrangé wrote:
> This fixes some edge cases in endian handling in the VNC server. These
> bugs are rarely going to be visible by default, since most servers will
> negotiate encoding formats / framebuffer formats that avoid hitting
> the problem scenarios.
> 
> In v2:
> 
>   - Rename 'client_bo' to 'client_endian' for reviewer clarity
>   - Rename 'native' to 'native_endian' for code consistency
> 
> Daniel P. Berrangé (3):
>    ui/vnc.c: replace big endian flag with byte order value
>    ui/vnc: take account of client byte order in pixman format
>    ui/vnc: fix tight palette pixel encoding for 8/16-bpp formats

Hi!

Is there anything in there which is relevant for qemu-stable?

Thanks,

/mjt

Re: [PATCH v2 0/3] ui/vnc: fix some endian problems

[Daniel P. Berrangé]

On Sat, May 24, 2025 at 01:32:45PM +0300, Michael Tokarev wrote:
> On 14.05.2025 14:19, Daniel P. Berrangé wrote:
> > This fixes some edge cases in endian handling in the VNC server. These
> > bugs are rarely going to be visible by default, since most servers will
> > negotiate encoding formats / framebuffer formats that avoid hitting
> > the problem scenarios.
> > 
> > In v2:
> > 
> >   - Rename 'client_bo' to 'client_endian' for reviewer clarity
> >   - Rename 'native' to 'native_endian' for code consistency
> > 
> > Daniel P. Berrangé (3):
> >    ui/vnc.c: replace big endian flag with byte order value
> >    ui/vnc: take account of client byte order in pixman format
> >    ui/vnc: fix tight palette pixel encoding for 8/16-bpp formats
> 
> Hi!
> 
> Is there anything in there which is relevant for qemu-stable?

I've not seen anyone raise these issues, since basically every serious
VNC client supports 'tight' encoding and it is very rare to run anything
other than true colour mode these days.

None the less these are clear bug fixes, so valid to pull all three
patches into stable if desired.


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 :|