From 3cc3da41c1d672d9e8847b142d5fe6b6ece9e520 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 23 Sep 2026 08:16:15 +0200 Subject: [PATCH] Open the FreeBSD framebuffer read-only and decode depth-24 as 32-bit storage, validate X11 byte order and visual masks, honour destination stride, and drop the cached frame on close --- client/vnc/server/capture_fb_freebsd.go | 21 +++++- client/vnc/server/capture_fb_freebsd_test.go | 61 +++++----------- client/vnc/server/capture_x11.go | 49 +++++++++++-- client/vnc/server/capture_x11_format_test.go | 73 ++++++++++++++++++++ client/vnc/server/capture_x11_shm_linux.go | 2 +- client/vnc/server/swizzle.go | 18 +++++ client/vnc/server/swizzle_test.go | 22 ++++++ 7 files changed, 194 insertions(+), 52 deletions(-) create mode 100644 client/vnc/server/capture_x11_format_test.go diff --git a/client/vnc/server/capture_fb_freebsd.go b/client/vnc/server/capture_fb_freebsd.go index e0be37be2..efbca40ec 100644 --- a/client/vnc/server/capture_fb_freebsd.go +++ b/client/vnc/server/capture_fb_freebsd.go @@ -54,7 +54,10 @@ func NewFBCapturer(path string) (*FBCapturer, error) { if path == "" { path = defaultFBPath() } - fd, err := unix.Open(path, unix.O_RDWR, 0) + // Read-only is all capture needs: FBIOGTYPE and a PROT_READ mapping both + // work on it, and asking for write access fails on a deployment that only + // granted the service read access to the console device. + fd, err := unix.Open(path, unix.O_RDONLY, 0) if err != nil { return nil, fmt.Errorf("open %s: %w", path, err) } @@ -79,13 +82,13 @@ func NewFBCapturer(path string) (*FBCapturer, error) { return nil, fmt.Errorf("mmap %s: %w (vt may not support mmap on this driver, e.g. virtio_gpu)", path, err) } - bpp := int(fbt.FbDepth) stride, err := freebsdFBStride(fbt) if err != nil { _ = unix.Munmap(mm) unix.Close(fd) return nil, err } + bpp := freebsdStorageBits(fbt, stride) c := &FBCapturer{ path: path, fd: fd, // valid fd >= 0; we use -1 as the closed sentinel @@ -99,6 +102,20 @@ func NewFBCapturer(path string) (*FBCapturer, error) { return c, nil } +// freebsdStorageBits returns how many bits each pixel occupies in memory, +// which is not what fb_depth reports. fb_depth is the colour depth: a KMS +// framebuffer is depth 24 stored as 32-bit XRGB, and decoding that with the +// packed 24-bit path reads every row at three-quarters of its real width and +// corrupts the whole screen. fbtype carries no storage field, so the row pitch +// decides: a depth-24 row wide enough for four bytes per pixel is 32-bit +// storage, anything narrower is packed. +func freebsdStorageBits(fbt fbType, stride int) int { + if fbt.FbDepth == 24 && stride >= int(fbt.FbWidth)*4 { + return 32 + } + return int(fbt.FbDepth) +} + // freebsdFBStride returns the framebuffer's row pitch in bytes. // // Not width*bpp/8: a KMS-backed framebuffer commonly pads each row up to an diff --git a/client/vnc/server/capture_fb_freebsd_test.go b/client/vnc/server/capture_fb_freebsd_test.go index 6b1d6cbd0..85e188044 100644 --- a/client/vnc/server/capture_fb_freebsd_test.go +++ b/client/vnc/server/capture_fb_freebsd_test.go @@ -6,54 +6,29 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -// The row pitch decides where every row after the first begins, so getting it -// from the reported width instead of the mapping shears the whole image on any -// device that pads its rows. -func TestFreebsdFBStride(t *testing.T) { +// fb_depth is colour depth, not storage: a KMS console is depth 24 stored as +// 32-bit XRGB, and decoding it as packed 24-bit reads every row at the wrong +// width. The row pitch decides which it is. +func TestFreebsdStorageBits(t *testing.T) { tests := []struct { - name string - fbt fbType - want int - wantErr bool + name string + depth int32 + width int32 + stride int + want int }{ - { - name: "unpadded 32bpp", - fbt: fbType{FbWidth: 1920, FbHeight: 1080, FbDepth: 32, FbSize: 1920 * 4 * 1080}, - want: 1920 * 4, - }, - { - name: "row padded up to an alignment", - fbt: fbType{FbWidth: 1366, FbHeight: 768, FbDepth: 32, FbSize: 5504 * 768}, - want: 5504, // 1366*4 = 5464, padded to 5504 - }, - { - name: "unpadded 16bpp", - fbt: fbType{FbWidth: 800, FbHeight: 600, FbDepth: 16, FbSize: 800 * 2 * 600}, - want: 800 * 2, - }, - { - // A mapping too small for the geometry it reports: reading rows at - // the reported width would run off the end of it. - name: "size cannot hold the geometry", - fbt: fbType{FbWidth: 1920, FbHeight: 1080, FbDepth: 32, FbSize: 1920 * 4 * 500}, - wantErr: true, - }, + {"depth 24 in 32-bit pixels", 24, 1024, 4096, 32}, + {"depth 24 in 32-bit pixels with padding", 24, 1000, 4096, 32}, + {"packed depth 24", 24, 1024, 3072, 24}, + {"depth 32", 32, 1024, 4096, 32}, + {"depth 16", 16, 1024, 2048, 16}, } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := freebsdFBStride(tt.fbt) - if tt.wantErr { - require.Error(t, err) - return - } - require.NoError(t, err) - assert.Equal(t, tt.want, got) - assert.GreaterOrEqual(t, got, int(tt.fbt.FbWidth)*(int(tt.fbt.FbDepth)/8), - "the pitch can pad a row but never truncate it") + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := freebsdStorageBits(fbType{FbDepth: tc.depth, FbWidth: tc.width}, tc.stride) + assert.Equal(t, tc.want, got, "storage bits per pixel") }) } } diff --git a/client/vnc/server/capture_x11.go b/client/vnc/server/capture_x11.go index 866326d94..5c0d019b1 100644 --- a/client/vnc/server/capture_x11.go +++ b/client/vnc/server/capture_x11.go @@ -435,7 +435,7 @@ func NewX11Capturer(display, cookieHex string) (*X11Capturer, error) { } screen := setup.Roots[0] - if err := checkPixmapFormat(setup, screen.RootDepth); err != nil { + if err := checkPixmapFormat(setup, &screen); err != nil { conn.Close() return nil, err } @@ -457,13 +457,29 @@ func NewX11Capturer(display, cookieHex string) (*X11Capturer, error) { // checkPixmapFormat rejects a screen whose pixels this capturer cannot decode. // GetImage returns ZPixmap data in the server's pixmap format for the screen's -// depth, and both the SHM and GetImage paths read it as 8-bit-per-channel BGRA. -// A 16-bpp screen, a packed 24-bpp one, or a 30-bit deep-colour one would -// otherwise pass startup and fail on every frame instead. -func checkPixmapFormat(setup *xproto.SetupInfo, depth byte) error { +// depth, and both the SHM and GetImage paths read it as 8-bit-per-channel BGRA: +// blue in the first byte of each pixel. That needs three things to hold, and a +// screen failing any of them would pass startup and then deliver every frame +// with wrong colours or garbage instead: 32 bits per pixel at depth 24 or 32, +// least-significant-byte-first image order, and a root visual whose masks put +// red, green and blue in the second, third and fourth bytes from the top. +func checkPixmapFormat(setup *xproto.SetupInfo, screen *xproto.ScreenInfo) error { + depth := screen.RootDepth if depth != 24 && depth != 32 { return fmt.Errorf("unsupported X11 root depth %d, need 24 or 32", depth) } + if err := checkPixmapBitsPerPixel(setup, depth); err != nil { + return err + } + if setup.ImageByteOrder != xproto.ImageOrderLSBFirst { + return fmt.Errorf("unsupported X11 image byte order %d, need LSB first", setup.ImageByteOrder) + } + return checkRootVisualMasks(screen) +} + +// checkPixmapBitsPerPixel requires the pixmap format for depth to store 32 +// bits per pixel. +func checkPixmapBitsPerPixel(setup *xproto.SetupInfo, depth byte) error { for _, f := range setup.PixmapFormats { if f.Depth != depth { continue @@ -477,6 +493,24 @@ func checkPixmapFormat(setup *xproto.SetupInfo, depth byte) error { return fmt.Errorf("no X11 pixmap format for root depth %d", depth) } +// checkRootVisualMasks requires the root visual to be 0xRRGGBB, which in an +// LSB-first 32-bit pixel is the B, G, R byte order the decoder reads. +func checkRootVisualMasks(screen *xproto.ScreenInfo) error { + for _, d := range screen.AllowedDepths { + for _, v := range d.Visuals { + if v.VisualId != screen.RootVisual { + continue + } + if v.RedMask != 0xff0000 || v.GreenMask != 0x00ff00 || v.BlueMask != 0x0000ff { + return fmt.Errorf("unsupported X11 root visual masks r=%#x g=%#x b=%#x, need 0xff0000/0xff00/0xff", + v.RedMask, v.GreenMask, v.BlueMask) + } + return nil + } + } + return fmt.Errorf("X11 root visual %d not found among the screen's visuals", screen.RootVisual) +} + // initSHM is implemented in capture_x11_shm_linux.go (requires SysV SHM). // On platforms without SysV SHM (FreeBSD), a stub returns an error and // the capturer falls back to GetImage. @@ -527,7 +561,7 @@ func (c *X11Capturer) captureGetImageInto(dst *image.RGBA) error { if len(reply.Data) < n { return fmt.Errorf("GetImage returned %d bytes, expected %d", len(reply.Data), n) } - swizzleBGRAtoRGBA(dst.Pix, reply.Data) + swizzleBGRAIntoImage(dst, reply.Data, c.w, c.h) return nil } @@ -657,6 +691,9 @@ func (p *X11Poller) Close() { p.capturer.Close() p.capturer = nil } + // The cache would otherwise keep answering Capture for freshWindow after + // shutdown, with a picture of a desktop this poller no longer watches. + p.lastFrame = nil } // Width returns the screen width. Triggers lazy init if needed. diff --git a/client/vnc/server/capture_x11_format_test.go b/client/vnc/server/capture_x11_format_test.go new file mode 100644 index 000000000..43248ea87 --- /dev/null +++ b/client/vnc/server/capture_x11_format_test.go @@ -0,0 +1,73 @@ +//go:build (linux && !android) || freebsd + +package server + +import ( + "testing" + + "github.com/jezek/xgb/xproto" + "github.com/stretchr/testify/assert" +) + +// The capturer decodes every frame as 32-bit BGRA. A screen that is not laid +// out that way has to be refused at startup: accepted, it produces a +// successful-looking session whose every frame has swapped or garbage colours. +func TestCheckPixmapFormat(t *testing.T) { + const rootVisual xproto.Visualid = 0x21 + + build := func(depth, bpp byte, order byte, r, g, b uint32) (*xproto.SetupInfo, *xproto.ScreenInfo) { + setup := &xproto.SetupInfo{ + ImageByteOrder: order, + PixmapFormats: []xproto.Format{{Depth: depth, BitsPerPixel: bpp}}, + } + screen := &xproto.ScreenInfo{ + RootDepth: depth, + RootVisual: rootVisual, + AllowedDepths: []xproto.DepthInfo{{ + Depth: depth, + Visuals: []xproto.VisualInfo{{ + VisualId: rootVisual, RedMask: r, GreenMask: g, BlueMask: b, + }}, + }}, + } + return setup, screen + } + + tests := []struct { + name string + depth byte + bpp byte + order byte + r, g, b uint32 + wantErr bool + }{ + {"depth 24 in 32 bits, LSB first, RGB masks", 24, 32, xproto.ImageOrderLSBFirst, 0xff0000, 0xff00, 0xff, false}, + {"depth 32", 32, 32, xproto.ImageOrderLSBFirst, 0xff0000, 0xff00, 0xff, false}, + {"16-bit screen", 16, 16, xproto.ImageOrderLSBFirst, 0xf800, 0x7e0, 0x1f, true}, + {"packed 24 bits per pixel", 24, 24, xproto.ImageOrderLSBFirst, 0xff0000, 0xff00, 0xff, true}, + {"MSB-first byte order", 24, 32, xproto.ImageOrderMSBFirst, 0xff0000, 0xff00, 0xff, true}, + {"BGR visual", 24, 32, xproto.ImageOrderLSBFirst, 0xff, 0xff00, 0xff0000, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + setup, screen := build(tc.depth, tc.bpp, tc.order, tc.r, tc.g, tc.b) + err := checkPixmapFormat(setup, screen) + if tc.wantErr { + assert.Error(t, err, "a layout the BGRA decoder cannot read must be refused") + } else { + assert.NoError(t, err, "the standard 32-bit BGRA layout must be accepted") + } + }) + } +} + +// A root visual that the screen does not list cannot be checked, so it is +// refused rather than assumed. +func TestCheckPixmapFormat_UnknownRootVisual(t *testing.T) { + setup := &xproto.SetupInfo{ + ImageByteOrder: xproto.ImageOrderLSBFirst, + PixmapFormats: []xproto.Format{{Depth: 24, BitsPerPixel: 32}}, + } + screen := &xproto.ScreenInfo{RootDepth: 24, RootVisual: 0x99} + assert.Error(t, checkPixmapFormat(setup, screen)) +} diff --git a/client/vnc/server/capture_x11_shm_linux.go b/client/vnc/server/capture_x11_shm_linux.go index 466318fa5..6e5bb9194 100644 --- a/client/vnc/server/capture_x11_shm_linux.go +++ b/client/vnc/server/capture_x11_shm_linux.go @@ -86,7 +86,7 @@ func (c *X11Capturer) captureSHMInto(dst *image.RGBA) error { if err := c.fillSHM(); err != nil { return err } - swizzleBGRAtoRGBA(dst.Pix, c.shmAddr[:c.w*c.h*4]) + swizzleBGRAIntoImage(dst, c.shmAddr[:c.w*c.h*4], c.w, c.h) return nil } diff --git a/client/vnc/server/swizzle.go b/client/vnc/server/swizzle.go index 1930831c1..da382874f 100644 --- a/client/vnc/server/swizzle.go +++ b/client/vnc/server/swizzle.go @@ -4,6 +4,7 @@ package server import ( "encoding/binary" + "image" "unsafe" ) @@ -52,3 +53,20 @@ func swizzleBGRAtoRGBABytes(dst, src []byte) { d[0], d[1], d[2], d[3] = s[2], s[1], s[0], 0xFF } } + +// swizzleBGRAIntoImage converts a tightly packed w x h BGRA source into dst. +// dst may be any image.RGBA of that size, including one whose rows are padded +// (a SubImage, or a buffer allocated with a wider stride): swizzleBGRAtoRGBA on +// dst.Pix as a whole would then write pixel data into the padding and shift +// every row after the first. A packed dst still takes the single-pass path. +func swizzleBGRAIntoImage(dst *image.RGBA, src []byte, w, h int) { + rowBytes := w * 4 + if dst.Stride == rowBytes { + swizzleBGRAtoRGBA(dst.Pix[:rowBytes*h], src[:rowBytes*h]) + return + } + for y := 0; y < h; y++ { + d := dst.Pix[y*dst.Stride : y*dst.Stride+rowBytes] + swizzleBGRAtoRGBA(d, src[y*rowBytes:(y+1)*rowBytes]) + } +} diff --git a/client/vnc/server/swizzle_test.go b/client/vnc/server/swizzle_test.go index 8d70d3085..9b97c92e0 100644 --- a/client/vnc/server/swizzle_test.go +++ b/client/vnc/server/swizzle_test.go @@ -3,6 +3,7 @@ package server import ( + "image" "testing" "github.com/stretchr/testify/assert" @@ -35,3 +36,24 @@ func TestSwizzleBGRAtoRGBA_InPlace(t *testing.T) { swizzleBGRAtoRGBA(buf, buf) assert.Equal(t, []byte{0x33, 0x22, 0x11, 0xFF}, buf) } + +// A destination with padded rows must get each row at its own offset, with the +// padding left alone, rather than having the pixels run on into it. +func TestSwizzleBGRAIntoImage_PaddedStride(t *testing.T) { + const w, h = 2, 2 + src := []byte{ + 0x01, 0x02, 0x03, 0x00, 0x04, 0x05, 0x06, 0x00, + 0x07, 0x08, 0x09, 0x00, 0x0a, 0x0b, 0x0c, 0x00, + } + // Four bytes of padding per row, filled with a marker that must survive. + dst := &image.RGBA{Pix: make([]byte, 2*(w*4+4)), Stride: w*4 + 4, Rect: image.Rect(0, 0, w, h)} + for i := range dst.Pix { + dst.Pix[i] = 0xee + } + + swizzleBGRAIntoImage(dst, src, w, h) + + assert.Equal(t, []byte{0x03, 0x02, 0x01, 0xff, 0x06, 0x05, 0x04, 0xff}, dst.Pix[0:8], "row 0 pixels") + assert.Equal(t, []byte{0xee, 0xee, 0xee, 0xee}, dst.Pix[8:12], "row 0 padding untouched") + assert.Equal(t, []byte{0x09, 0x08, 0x07, 0xff, 0x0c, 0x0b, 0x0a, 0xff}, dst.Pix[12:20], "row 1 pixels") +}