From d8236002c77c8a28edcf6b8fa01fa5e6e34f31d7 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Sat, 29 Aug 2026 12:40:14 +0200 Subject: [PATCH] Honour the negotiated pixel format for the cursor, enqueue key edges reliably, drop racy test writes --- client/vnc/server/input_windows.go | 9 ++- client/vnc/server/noise_auth_test.go | 8 +- client/vnc/server/security_hardening_test.go | 10 +-- client/vnc/server/server_test.go | 31 +++++--- client/vnc/server/session_cursor.go | 22 +++--- client/vnc/server/session_cursor_test.go | 78 +++++++++----------- 6 files changed, 86 insertions(+), 72 deletions(-) diff --git a/client/vnc/server/input_windows.go b/client/vnc/server/input_windows.go index c9479538a..cdf05632b 100644 --- a/client/vnc/server/input_windows.go +++ b/client/vnc/server/input_windows.go @@ -223,8 +223,13 @@ func (w *WindowsInputInjector) dispatch(cmd inputCmd) { } // InjectKey queues a key event for injection on the input desktop thread. +// +// Enqueued reliably, like pointer button transitions and for the same reason: +// every key event is an edge, and a dropped release leaves that key held down +// on the host with nothing to lift it. That covers the releases +// releaseStickyInput sends when a client disconnects mid-keystroke. func (w *WindowsInputInjector) InjectKey(keysym uint32, down bool) { - w.tryEnqueue(inputCmd{isKey: true, keysym: keysym, down: down}) + w.enqueueReliable(inputCmd{isKey: true, keysym: keysym, down: down}) } // InjectKeyScancode queues a raw-scancode key event. PC AT Set 1 maps @@ -237,7 +242,7 @@ func (w *WindowsInputInjector) InjectKeyScancode(scancode uint32, keysym uint32, w.InjectKey(keysym, down) return } - w.tryEnqueue(inputCmd{isScancode: true, scancode: scancode, keysym: keysym, down: down}) + w.enqueueReliable(inputCmd{isScancode: true, scancode: scancode, keysym: keysym, down: down}) } // InjectPointer queues a pointer event for injection on the input desktop diff --git a/client/vnc/server/noise_auth_test.go b/client/vnc/server/noise_auth_test.go index ec2396aeb..7eb862360 100644 --- a/client/vnc/server/noise_auth_test.go +++ b/client/vnc/server/noise_auth_test.go @@ -37,7 +37,9 @@ func noiseTestServer(t *testing.T) (net.Addr, *Server, []byte) { addr := netip.MustParseAddrPort("127.0.0.1:0") network := netip.MustParsePrefix("127.0.0.0/8") require.NoError(t, srv.Start(t.Context(), addr, network)) - srv.localAddr = netip.MustParseAddr("10.99.99.1") + // No local-address override: isAllowedSource short-circuits on + // loopback-to-loopback before the own-IP check, and writing srv.localAddr + // here would race the accept loop Start has already spawned. t.Cleanup(func() { _ = srv.Stop() }) return srv.listener.Addr(), srv, kp.Public @@ -365,7 +367,9 @@ func TestNoise_NoIdentityKey_FailsClosed(t *testing.T) { addr := netip.MustParseAddrPort("127.0.0.1:0") network := netip.MustParsePrefix("127.0.0.0/8") require.NoError(t, srv.Start(t.Context(), addr, network)) - srv.localAddr = netip.MustParseAddr("10.99.99.1") + // No local-address override: isAllowedSource short-circuits on + // loopback-to-loopback before the own-IP check, and writing srv.localAddr + // here would race the accept loop Start has already spawned. t.Cleanup(func() { _ = srv.Stop() }) clientKey, err := noise.DH25519.GenerateKeypair(nil) diff --git a/client/vnc/server/security_hardening_test.go b/client/vnc/server/security_hardening_test.go index 6c60c21f7..4b41f3982 100644 --- a/client/vnc/server/security_hardening_test.go +++ b/client/vnc/server/security_hardening_test.go @@ -114,18 +114,18 @@ func TestAppendTightLengthClampsInsteadOfPanicking(t *testing.T) { func TestEncodeCursorPseudoRectCapsDimensions(t *testing.T) { t.Run("oversized_rejected", func(t *testing.T) { img := image.NewRGBA(image.Rect(0, 0, maxCursorDim+1, 1)) - if buf := encodeCursorPseudoRect(img, 0, 0); buf != nil { + if buf := encodeCursorPseudoRect(img, 0, 0, defaultClientPixelFormat()); buf != nil { t.Fatalf("expected nil for oversized cursor, got %d bytes", len(buf)) } }) t.Run("nil_rejected", func(t *testing.T) { - if buf := encodeCursorPseudoRect(nil, 0, 0); buf != nil { + if buf := encodeCursorPseudoRect(nil, 0, 0, defaultClientPixelFormat()); buf != nil { t.Fatal("expected nil for nil image") } }) t.Run("zero_dims_rejected", func(t *testing.T) { img := image.NewRGBA(image.Rect(0, 0, 0, 0)) - if buf := encodeCursorPseudoRect(img, 0, 0); buf != nil { + if buf := encodeCursorPseudoRect(img, 0, 0, defaultClientPixelFormat()); buf != nil { t.Fatal("expected nil for zero-dim image") } }) @@ -135,7 +135,7 @@ func TestEncodeCursorPseudoRectCapsDimensions(t *testing.T) { for i := range img.Pix { img.Pix[i] = 0x80 } - buf := encodeCursorPseudoRect(img, 1, 2) + buf := encodeCursorPseudoRect(img, 1, 2, defaultClientPixelFormat()) if buf == nil { t.Fatal("expected encoded cursor, got nil") } @@ -151,7 +151,7 @@ func TestEncodeCursorPseudoRectCapsDimensions(t *testing.T) { // must allow exactly maxCursorDim×maxCursorDim through. func TestEncodeCursorPseudoRectAtMaxDim(t *testing.T) { img := image.NewRGBA(image.Rect(0, 0, maxCursorDim, maxCursorDim)) - if buf := encodeCursorPseudoRect(img, 0, 0); buf == nil { + if buf := encodeCursorPseudoRect(img, 0, 0, defaultClientPixelFormat()); buf == nil { t.Fatal("expected non-nil for max-dim cursor (boundary)") } } diff --git a/client/vnc/server/server_test.go b/client/vnc/server/server_test.go index 10b7e196a..e9229e20d 100644 --- a/client/vnc/server/server_test.go +++ b/client/vnc/server/server_test.go @@ -41,8 +41,10 @@ func startTestServer(t *testing.T, disableAuth bool) (net.Addr, *Server) { addr := netip.MustParseAddrPort("127.0.0.1:0") network := netip.MustParsePrefix("127.0.0.0/8") require.NoError(t, srv.Start(t.Context(), addr, network)) - // Override local address so source validation doesn't reject 127.0.0.1 as "own IP". - srv.localAddr = netip.MustParseAddr("10.99.99.1") + // No local-address override: isAllowedSource short-circuits on + // loopback-to-loopback before it reaches the own-IP check, so a 127.0.0.1 + // client is admitted as is. Writing the field here would race the accept + // loop Start has already spawned. t.Cleanup(func() { _ = srv.Stop() }) return srv.listener.Addr(), srv @@ -119,16 +121,24 @@ func TestAuthDisabled_AllowsConnection(t *testing.T) { // server must close immediately and the client must see EOF before any RFB // version greeting is written. func TestAuth_NoUnauthBytesPastHeader(t *testing.T) { + // The listener has to be loopback so the test can dial it, while the + // overlay has to exclude 127.0.0.0/8 and the local address has to be + // non-loopback, or isAllowedSource short-circuits and admits the client. + // Start cannot express that pair, so the listener is supplied ready-made: + // the pre-listener path leaves localAddr and network alone, which lets them + // be set here, before Start spawns the accept loop that reads them. + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + srv := New(Config{ Capturer: &testCapturer{}, Injector: &StubInputInjector{}, DisableAuth: true, + Listener: ln, }) - addr := netip.MustParseAddrPort("127.0.0.1:0") - // Tight overlay that excludes 127.0.0.0/8 and a non-loopback local IP, so - // the loopback short-circuit in isAllowedSource doesn't apply. - require.NoError(t, srv.Start(t.Context(), addr, netip.MustParsePrefix("10.99.0.0/16"))) srv.localAddr = netip.MustParseAddr("10.99.99.1") + srv.network = netip.MustParsePrefix("10.99.0.0/16") + require.NoError(t, srv.Start(t.Context(), netip.AddrPort{}, netip.Prefix{})) t.Cleanup(func() { _ = srv.Stop() }) conn, err := net.Dial("tcp", srv.listener.Addr().String()) @@ -270,7 +280,9 @@ func TestAgentToken_MismatchClosesConnection(t *testing.T) { addr := netip.MustParseAddrPort("127.0.0.1:0") network := netip.MustParsePrefix("127.0.0.0/8") require.NoError(t, srv.Start(t.Context(), addr, network)) - srv.localAddr = netip.MustParseAddr("10.99.99.1") + // No local-address override: isAllowedSource short-circuits on + // loopback-to-loopback before the own-IP check, and writing srv.localAddr + // here would race the accept loop Start has already spawned. t.Cleanup(func() { _ = srv.Stop() }) conn, err := net.Dial("tcp", srv.listener.Addr().String()) @@ -304,7 +316,9 @@ func TestAgentToken_MatchAllowsHandshake(t *testing.T) { addr := netip.MustParseAddrPort("127.0.0.1:0") network := netip.MustParsePrefix("127.0.0.0/8") require.NoError(t, srv.Start(t.Context(), addr, network)) - srv.localAddr = netip.MustParseAddr("10.99.99.1") + // No local-address override: isAllowedSource short-circuits on + // loopback-to-loopback before the own-IP check, and writing srv.localAddr + // here would race the accept loop Start has already spawned. t.Cleanup(func() { _ = srv.Stop() }) conn, err := net.Dial("tcp", srv.listener.Addr().String()) @@ -340,7 +354,6 @@ func TestSessionMode_RejectedWhenNoVMGR(t *testing.T) { addr := netip.MustParseAddrPort("127.0.0.1:0") network := netip.MustParsePrefix("127.0.0.0/8") require.NoError(t, srv.Start(t.Context(), addr, network)) - srv.localAddr = netip.MustParseAddr("10.99.99.1") // Force vmgr to nil regardless of platform so the test is deterministic. srv.vmgr = nil t.Cleanup(func() { _ = srv.Stop() }) diff --git a/client/vnc/server/session_cursor.go b/client/vnc/server/session_cursor.go index c6a9a44eb..cc059fb97 100644 --- a/client/vnc/server/session_cursor.go +++ b/client/vnc/server/session_cursor.go @@ -17,6 +17,7 @@ func (s *session) pendingCursorRect() []byte { failed := s.cursorSourceFailed composite := s.showRemoteCursor lastSerial := s.lastCursorSerial + pf := s.pf s.encMu.RUnlock() if !supported || failed || composite { return nil @@ -36,7 +37,7 @@ func (s *session) pendingCursorRect() []byte { if img == nil || serial == lastSerial { return nil } - buf := encodeCursorPseudoRect(img, hotX, hotY) + buf := encodeCursorPseudoRect(img, hotX, hotY, pf) if buf == nil { return nil } @@ -70,11 +71,12 @@ const maxCursorDim = 256 // encodeCursorPseudoRect packs the cursor sprite into a Cursor pseudo // rectangle (RFB 7.7.4, pseudo-encoding -239). Layout: 12-byte rect header -// followed by w*h*4 BGRX pixel bytes and a 1-bit mask of (w+7)/8 bytes per -// row, MSB-first, with each row independently padded. Returns nil when +// followed by w*h*4 pixel bytes at pf's negotiated channel shifts, then a +// 1-bit mask of (w+7)/8 bytes per row, MSB-first, with each row independently +// padded. Returns nil when // the source image's dimensions are non-positive or exceed maxCursorDim; // callers treat nil as "skip the cursor rect this frame." -func encodeCursorPseudoRect(img *image.RGBA, hotX, hotY int) []byte { +func encodeCursorPseudoRect(img *image.RGBA, hotX, hotY int, pf clientPixelFormat) []byte { if img == nil { return nil } @@ -104,6 +106,11 @@ func encodeCursorPseudoRect(img *image.RGBA, hotX, hotY int) []byte { mask := buf[12+pixelBytes:] src := img.Pix stride := img.Stride + // Packed at the negotiated shifts, the same way writePixels packs the + // framebuffer. Hard-coding BGRX here would leave a client that asked for + // another channel order with a correctly coloured desktop and a cursor + // with its red and blue swapped. + rShift, gShift, bShift := pf.rShift, pf.gShift, pf.bShift for y := 0; y < h; y++ { row := y * stride dstRow := y * w * 4 @@ -113,11 +120,8 @@ func encodeCursorPseudoRect(img *image.RGBA, hotX, hotY int) []byte { g := src[row+x*4+1] b := src[row+x*4+2] a := src[row+x*4+3] - off := dstRow + x*4 - pix[off+0] = b - pix[off+1] = g - pix[off+2] = r - pix[off+3] = 0 + pixel := (uint32(r) << rShift) | (uint32(g) << gShift) | (uint32(b) << bShift) + binary.LittleEndian.PutUint32(pix[dstRow+x*4:dstRow+x*4+4], pixel) if a >= 0x80 { mask[maskRow+x/8] |= 0x80 >> (x % 8) } diff --git a/client/vnc/server/session_cursor_test.go b/client/vnc/server/session_cursor_test.go index 9d72d6553..7bcc1dc89 100644 --- a/client/vnc/server/session_cursor_test.go +++ b/client/vnc/server/session_cursor_test.go @@ -11,70 +11,58 @@ import ( "github.com/stretchr/testify/require" ) -// fakeCursorCapturer plays back a scripted sequence of cursor sprites, each -// with the serial its platform would report. -type fakeCursorCapturer struct { - sprites []fakeSprite - next int -} - -type fakeSprite struct { +// stubCursorSource returns a scripted sequence of cursors, standing in for a +// platform cursor source. +type stubCursorSource struct { + img *image.RGBA serial uint64 - err error } -func (f *fakeCursorCapturer) Width() int { return 100 } -func (f *fakeCursorCapturer) Height() int { return 100 } +func (s *stubCursorSource) Width() int { return 64 } +func (s *stubCursorSource) Height() int { return 64 } -func (f *fakeCursorCapturer) Capture() (*image.RGBA, error) { - return image.NewRGBA(image.Rect(0, 0, 100, 100)), nil +func (s *stubCursorSource) Capture() (*image.RGBA, error) { + return image.NewRGBA(image.Rect(0, 0, 64, 64)), nil } -func (f *fakeCursorCapturer) Cursor() (*image.RGBA, int, int, uint64, error) { - s := f.sprites[min(f.next, len(f.sprites)-1)] - f.next++ - if s.err != nil { - return nil, 0, 0, 0, s.err - } - return image.NewRGBA(image.Rect(0, 0, 16, 16)), 0, 0, s.serial, nil +func (s *stubCursorSource) Cursor() (*image.RGBA, int, int, uint64, error) { + return s.img, 0, 0, s.serial, nil } -func newCursorSession(t *testing.T, cap ScreenCapturer) *session { - t.Helper() +func newCursorSession(src *stubCursorSource) *session { return &session{ - capturer: cap, + capturer: src, clientSupportsCursor: true, - log: log.WithField("test", t.Name()), + log: log.WithField("test", "cursor"), } } -// X11 reports the XFixes cursor-serial, which names the cursor rather than -// counting upwards: switching back to a cursor shown earlier yields a lower -// value. Ordering the serials treated that as stale and left the client stuck -// on whichever cursor had the highest one, typically the I-beam. -func TestPendingCursorRect_SerialGoingBackwardsStillUpdates(t *testing.T) { - cap := &fakeCursorCapturer{sprites: []fakeSprite{ - {serial: 100}, // arrow - {serial: 250}, // I-beam over a text field - {serial: 100}, // back to the arrow - }} - s := newCursorSession(t, cap) - - require.NotNil(t, s.pendingCursorRect(), "the first cursor must be sent") - assert.Equal(t, uint64(100), s.lastCursorSerial) +// X11 passes through the XFixes cursor-serial, which names the cursor rather +// than counting upwards: going back to a cursor shown earlier reports a lower +// value. An ordering comparison discarded that update and left the client stuck +// on whichever cursor had the highest serial, in practice the I-beam. +func TestPendingCursorRect_SwitchingBackToALowerSerial(t *testing.T) { + sprite := image.NewRGBA(image.Rect(0, 0, 16, 16)) + src := &stubCursorSource{img: sprite, serial: 100} + s := newCursorSession(src) + // The arrow, then an I-beam the X server happens to number higher. + require.NotNil(t, s.pendingCursorRect(), "first cursor must be sent") + src.serial = 250 require.NotNil(t, s.pendingCursorRect(), "a different cursor must be sent") - assert.Equal(t, uint64(250), s.lastCursorSerial) - require.NotNil(t, s.pendingCursorRect(), "returning to an earlier cursor must be sent too") - assert.Equal(t, uint64(100), s.lastCursorSerial) + // Back to the arrow: a lower serial, and still a real change. + src.serial = 100 + assert.NotNil(t, s.pendingCursorRect(), "returning to an earlier cursor must be sent, not dropped as stale") } -// The same serial twice in a row is the same cursor and carries no update. +// An unchanged serial is still the one case that must not produce a rect, +// otherwise every framebuffer update would carry a redundant cursor. func TestPendingCursorRect_UnchangedSerialIsSkipped(t *testing.T) { - cap := &fakeCursorCapturer{sprites: []fakeSprite{{serial: 7}, {serial: 7}}} - s := newCursorSession(t, cap) + sprite := image.NewRGBA(image.Rect(0, 0, 16, 16)) + src := &stubCursorSource{img: sprite, serial: 7} + s := newCursorSession(src) require.NotNil(t, s.pendingCursorRect()) - assert.Nil(t, s.pendingCursorRect(), "an unchanged serial must not re-send the sprite") + assert.Nil(t, s.pendingCursorRect(), "the same cursor must not be re-sent") }