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

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

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")
 		})
 	}
 }

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.

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))
+}

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
 }
 

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])
+	}
+}

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")
+}