Lock pixel format to 32bpp little-endian truecolour and reject other formats

client/vnc/server/rfb.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"compress/zlib"
 	"encoding/binary"
+	"fmt"
 	"image"
 	"image/jpeg"
 	"unsafe"
@@ -80,24 +81,17 @@ const (
 	// Distinct-colour cap below which we still prefer Basic+zlib (text,
 	// UI). Sampled, not exhaustive: cheap to compute, good enough.
 	tightJPEGMinColors = 64
-
-	// Hextile subencoding flags (a bitmask in the first byte of each sub-tile).
-	hextileRaw                 = 0x01
-	hextileBackgroundSpecified = 0x02
-	hextileForegroundSpecified = 0x04
-	hextileAnySubrects         = 0x08
-	hextileSubrectsColoured    = 0x10
-
-	// Hextile sub-tile size per RFB spec.
-	hextileSubSize = 16
 )
 
-// serverPixelFormat is the default pixel format advertised by the server:
-// 32bpp RGBA, big-endian, true-colour, 8 bits per channel.
+// serverPixelFormat is the pixel format the server advertises and requires:
+// 32bpp RGBA, little-endian, true-colour, 8 bits per channel at standard
+// shifts (R=16, G=8, B=0). handleSetPixelFormat rejects any client that
+// negotiates a different format. Browser-side decoders are little-endian
+// natively, so advertising little-endian skips a byte-swap on every pixel.
 var serverPixelFormat = [16]byte{
 	32,     // bits-per-pixel
 	24,     // depth
-	1,      // big-endian-flag
+	0,      // big-endian-flag
 	1,      // true-colour-flag
 	0, 255, // red-max
 	0, 255, // green-max
@@ -108,42 +102,48 @@ var serverPixelFormat = [16]byte{
 	0, 0, 0, // padding
 }
 
-// clientPixelFormat holds the negotiated pixel format from the client.
+// clientPixelFormat holds the negotiated pixel format. Only RGB channel
+// shifts are tracked: every other field is constrained by the server to
+// the values in serverPixelFormat (32bpp / little-endian / truecolour /
+// 8-bit channels) and rejected at SetPixelFormat time if the client tries
+// to negotiate otherwise.
 type clientPixelFormat struct {
-	bpp       uint8
-	bigEndian uint8
-	rMax      uint16
-	gMax      uint16
-	bMax      uint16
-	rShift    uint8
-	gShift    uint8
-	bShift    uint8
+	rShift uint8
+	gShift uint8
+	bShift uint8
 }
 
 func defaultClientPixelFormat() clientPixelFormat {
 	return clientPixelFormat{
-		bpp:       serverPixelFormat[0],
-		bigEndian: serverPixelFormat[2],
-		rMax:      binary.BigEndian.Uint16(serverPixelFormat[4:6]),
-		gMax:      binary.BigEndian.Uint16(serverPixelFormat[6:8]),
-		bMax:      binary.BigEndian.Uint16(serverPixelFormat[8:10]),
-		rShift:    serverPixelFormat[10],
-		gShift:    serverPixelFormat[11],
-		bShift:    serverPixelFormat[12],
+		rShift: serverPixelFormat[10],
+		gShift: serverPixelFormat[11],
+		bShift: serverPixelFormat[12],
 	}
 }
 
-func parsePixelFormat(pf []byte) clientPixelFormat {
-	return clientPixelFormat{
-		bpp:       pf[0],
-		bigEndian: pf[2],
-		rMax:      binary.BigEndian.Uint16(pf[4:6]),
-		gMax:      binary.BigEndian.Uint16(pf[6:8]),
-		bMax:      binary.BigEndian.Uint16(pf[8:10]),
-		rShift:    pf[10],
-		gShift:    pf[11],
-		bShift:    pf[12],
+// parsePixelFormat returns the negotiated client pixel format, or an error
+// if the client tried to negotiate an unsupported format. The server only
+// supports 32bpp truecolour little-endian with 8-bit channels; arbitrary
+// shifts within that constraint are allowed because they are cheap to honour.
+func parsePixelFormat(pf []byte) (clientPixelFormat, error) {
+	bpp := pf[0]
+	bigEndian := pf[2]
+	trueColour := pf[3]
+	rMax := binary.BigEndian.Uint16(pf[4:6])
+	gMax := binary.BigEndian.Uint16(pf[6:8])
+	bMax := binary.BigEndian.Uint16(pf[8:10])
+	if bpp != 32 || bigEndian != 0 || trueColour != 1 ||
+		rMax != 255 || gMax != 255 || bMax != 255 {
+		return clientPixelFormat{}, fmt.Errorf(
+			"unsupported pixel format (bpp=%d be=%d tc=%d rgb-max=%d/%d/%d): "+
+				"server only supports 32bpp truecolour little-endian 8-bit channels",
+			bpp, bigEndian, trueColour, rMax, gMax, bMax)
 	}
+	return clientPixelFormat{
+		rShift: pf[10],
+		gShift: pf[11],
+		bShift: pf[12],
+	}, nil
 }
 
 // encodeCopyRectBody emits the per-rect payload for a CopyRect rectangle:
@@ -211,10 +211,7 @@ func encodeLastRectBody() []byte {
 // encodeRawRect encodes a framebuffer region as a raw RFB rectangle.
 // The returned buffer includes the FramebufferUpdate header (1 rectangle).
 func encodeRawRect(img *image.RGBA, pf clientPixelFormat, x, y, w, h int) []byte {
-	bytesPerPixel := max(int(pf.bpp)/8, 1)
-
-	pixelBytes := w * h * bytesPerPixel
-	buf := make([]byte, 4+12+pixelBytes)
+	buf := make([]byte, 4+12+w*h*4)
 
 	// FramebufferUpdate header.
 	buf[0] = serverFramebufferUpdate
@@ -228,26 +225,17 @@ func encodeRawRect(img *image.RGBA, pf clientPixelFormat, x, y, w, h int) []byte
 	binary.BigEndian.PutUint16(buf[10:12], uint16(h))
 	binary.BigEndian.PutUint32(buf[12:16], uint32(encRaw))
 
-	writePixels(buf[16:], img, pf, rect{x, y, w, h}, bytesPerPixel)
+	writePixels(buf[16:], img, pf, rect{x, y, w, h})
 	return buf
 }
 
-// writePixels writes a rectangle of img into dst in the client's requested
-// pixel format. It fast-paths the common case (32bpp, full 8-bit channels)
-// with a tight loop that skips the per-channel *max/255 arithmetic and emits
-// a single uint32 per pixel; the general path handles arbitrary formats.
-func writePixels(dst []byte, img *image.RGBA, pf clientPixelFormat, r rect, bytesPerPixel int) {
-	if bytesPerPixel == 4 && pf.rMax == 255 && pf.gMax == 255 && pf.bMax == 255 {
-		writePixelsFast32(dst, img, pf, r)
-		return
-	}
-	writePixelsGeneric(dst, img, pf, r, bytesPerPixel)
-}
-
-func writePixelsFast32(dst []byte, img *image.RGBA, pf clientPixelFormat, r rect) {
+// writePixels writes a rectangle of img into dst as 32bpp little-endian
+// pixels at the negotiated RGB shifts. The pixel format is constrained at
+// SetPixelFormat time so we can assume 4 bytes per pixel, 8-bit channels,
+// and little-endian byte order; arbitrary shifts (R/G/B order) are honoured.
+func writePixels(dst []byte, img *image.RGBA, pf clientPixelFormat, r rect) {
 	stride := img.Stride
 	rShift, gShift, bShift := pf.rShift, pf.gShift, pf.bShift
-	bigEndian := pf.bigEndian != 0
 	off := 0
 	for row := r.y; row < r.y+r.h; row++ {
 		p := row*stride + r.x*4
@@ -255,47 +243,13 @@ func writePixelsFast32(dst []byte, img *image.RGBA, pf clientPixelFormat, r rect
 			pixel := (uint32(img.Pix[p]) << rShift) |
 				(uint32(img.Pix[p+1]) << gShift) |
 				(uint32(img.Pix[p+2]) << bShift)
-			if bigEndian {
-				binary.BigEndian.PutUint32(dst[off:off+4], pixel)
-			} else {
-				binary.LittleEndian.PutUint32(dst[off:off+4], pixel)
-			}
+			binary.LittleEndian.PutUint32(dst[off:off+4], pixel)
 			p += 4
 			off += 4
 		}
 	}
 }
 
-func writePixelsGeneric(dst []byte, img *image.RGBA, pf clientPixelFormat, r rect, bytesPerPixel int) {
-	stride := img.Stride
-	off := 0
-	for row := r.y; row < r.y+r.h; row++ {
-		for col := r.x; col < r.x+r.w; col++ {
-			p := row*stride + col*4
-			rv := uint32(img.Pix[p]) * uint32(pf.rMax) / 255
-			gv := uint32(img.Pix[p+1]) * uint32(pf.gMax) / 255
-			bv := uint32(img.Pix[p+2]) * uint32(pf.bMax) / 255
-			pixel := (rv << pf.rShift) | (gv << pf.gShift) | (bv << pf.bShift)
-			emitPixelBytes(dst[off:off+bytesPerPixel], pixel, bytesPerPixel, pf.bigEndian != 0)
-			off += bytesPerPixel
-		}
-	}
-}
-
-func emitPixelBytes(dst []byte, pixel uint32, bytesPerPixel int, bigEndian bool) {
-	if bigEndian {
-		for i := range bytesPerPixel {
-			dst[i] = byte(pixel >> uint((bytesPerPixel-1-i)*8))
-		}
-		return
-	}
-	for i := range bytesPerPixel {
-		dst[i] = byte(pixel >> uint(i*8))
-	}
-}
-
-
-
 // diffTiles compares two RGBA images and returns a tile-ordered list of
 // dirty tiles, one entry per tile. Tile order is top-to-bottom, left-to-
 // right within each row. The caller decides whether to coalesce or hand
@@ -453,33 +407,6 @@ func tileIsUniform(img *image.RGBA, x, y, w, h int) (uint32, bool) {
 	return first, true
 }
 
-// encodePixel packs an RGBA byte triple into the client's requested pixel
-// format, honouring bpp, channel maxes, shifts and endianness. Returns the
-// number of bytes written to dst (1..4).
-func encodePixel(dst []byte, pf clientPixelFormat, r, g, b byte) int {
-	bytesPerPixel := max(int(pf.bpp)/8, 1)
-	var val uint32
-	if pf.rMax == 255 && pf.gMax == 255 && pf.bMax == 255 {
-		val = (uint32(r) << pf.rShift) | (uint32(g) << pf.gShift) | (uint32(b) << pf.bShift)
-	} else {
-		rv := uint32(r) * uint32(pf.rMax) / 255
-		gv := uint32(g) * uint32(pf.gMax) / 255
-		bv := uint32(b) * uint32(pf.bMax) / 255
-		val = (rv << pf.rShift) | (gv << pf.gShift) | (bv << pf.bShift)
-	}
-	if pf.bigEndian != 0 {
-		for i := range bytesPerPixel {
-			dst[i] = byte(val >> uint((bytesPerPixel-1-i)*8))
-		}
-	} else {
-		for i := range bytesPerPixel {
-			dst[i] = byte(val >> uint(i*8))
-		}
-	}
-	return bytesPerPixel
-}
-
-
 // tightState holds the per-session JPEG scratch buffer and reused encoders
 // so per-rect encoding stays alloc-free in the steady state.
 type tightState struct {

client/vnc/server/rfb_bench_test.go

@@ -84,26 +84,7 @@ func BenchmarkWritePixels(b *testing.B) {
 			b.SetBytes(int64(r.w * r.h * 4))
 			b.ReportAllocs()
 			for i := 0; i < b.N; i++ {
-				writePixels(dst, img, pf, rect{0, 0, r.w, r.h}, 4)
-			}
-		})
-	}
-}
-
-// BenchmarkWritePixelsScaled forces the general (non-fast) path by using a
-// pixel format with non-255 channel maxes.
-func BenchmarkWritePixelsScaled(b *testing.B) {
-	pf := defaultClientPixelFormat()
-	pf.rMax, pf.gMax, pf.bMax = 31, 63, 31 // 16bpp-ish; exercises the divide path
-	pf.bpp = 16
-	for _, r := range benchRects {
-		img := makeBenchImage(r.w, r.h, 1)
-		dst := make([]byte, r.w*r.h*2)
-		b.Run(r.name, func(b *testing.B) {
-			b.SetBytes(int64(r.w * r.h * 4))
-			b.ReportAllocs()
-			for i := 0; i < b.N; i++ {
-				writePixels(dst, img, pf, rect{0, 0, r.w, r.h}, 2)
+				writePixels(dst, img, pf, rect{0, 0, r.w, r.h})
 			}
 		})
 	}

client/vnc/server/session.go

@@ -260,7 +260,10 @@ func (s *session) handleSetPixelFormat() error {
 	if _, err := io.ReadFull(s.conn, buf[:]); err != nil {
 		return fmt.Errorf("read SetPixelFormat: %w", err)
 	}
-	pf := parsePixelFormat(buf[3:19])
+	pf, err := parsePixelFormat(buf[3:19])
+	if err != nil {
+		return err
+	}
 	s.encMu.Lock()
 	s.pf = pf
 	s.encMu.Unlock()
@@ -822,11 +825,8 @@ func drainRequests(ch chan fbRequest) int {
 }
 
 // pfIsTightCompatible reports whether the negotiated client pixel format
-// matches Tight's TPIXEL constraint: 32 bpp true colour with 8-bit RGB
-// channels at standard shifts (R=16, G=8, B=0). For anything else we fall
-// back to Zlib/Hextile/Raw which respect pf in full.
+// matches Tight's TPIXEL constraint: standard RGB shifts (R=16, G=8, B=0).
+// bpp/endianness/channel-max are already locked at SetPixelFormat time.
 func pfIsTightCompatible(pf clientPixelFormat) bool {
-	return pf.bpp == 32 &&
-		pf.rMax == 255 && pf.gMax == 255 && pf.bMax == 255 &&
-		pf.rShift == 16 && pf.gShift == 8 && pf.bShift == 0
+	return pf.rShift == 16 && pf.gShift == 8 && pf.bShift == 0
 }