From 913a1c6033d16e533fa8bb43a09f449a6309f24c Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Thu, 27 Aug 2026 21:08:20 +0200 Subject: [PATCH] Fix framebuffer layout handling, capturer lifecycle and macOS pointer state races --- client/vnc/server/agent_windows.go | 28 +++++++------- client/vnc/server/capture_fb_linux.go | 54 ++++++++++++++++++++++++++- client/vnc/server/capture_fb_unix.go | 14 ++++++- client/vnc/server/input_darwin.go | 10 +++++ client/vnc/server/shutdown_state.go | 6 ++- 5 files changed, 93 insertions(+), 19 deletions(-) diff --git a/client/vnc/server/agent_windows.go b/client/vnc/server/agent_windows.go index bab6035f5..bd5e8e35e 100644 --- a/client/vnc/server/agent_windows.go +++ b/client/vnc/server/agent_windows.go @@ -206,31 +206,31 @@ func getSystemTokenForSession(sessionID uint32) (windows.Token, error) { func injectEnvVar(envBlock uintptr, key, value string) []uint16 { entry := key + "=" + value - // Walk the existing block to find its total length. ptr := (*uint16)(unsafe.Pointer(envBlock)) + charAt := func(i int) uint16 { + return *(*uint16)(unsafe.Pointer(uintptr(unsafe.Pointer(ptr)) + uintptr(i)*2)) + } + + // Walk to the block's closing null, counting each entry and its own + // terminator. That offset is where the new entry goes, and it is 0 for an + // empty block, so nothing prepends an empty entry that would read to + // Windows as the end of the block. var totalChars int - for { - ch := *(*uint16)(unsafe.Pointer(uintptr(unsafe.Pointer(ptr)) + uintptr(totalChars)*2)) - if ch == 0 { - // Check for double-null terminator. - next := *(*uint16)(unsafe.Pointer(uintptr(unsafe.Pointer(ptr)) + uintptr(totalChars+1)*2)) - totalChars++ - if next == 0 { - // End of block (don't count the final null yet, we'll rebuild). - break - } - } else { + for charAt(totalChars) != 0 { + for charAt(totalChars) != 0 { totalChars++ } + totalChars++ } entryUTF16, _ := windows.UTF16FromString(entry) // New block: existing entries + new entry (null-terminated) + final null. newLen := totalChars + len(entryUTF16) + 1 newBlock := make([]uint16, newLen) - // Copy existing entries (up to but not including the final null). + // Copy existing entries, each with its own null, but not the block's + // closing null: that is re-added below, after the new entry. for i := range totalChars { - newBlock[i] = *(*uint16)(unsafe.Pointer(uintptr(unsafe.Pointer(ptr)) + uintptr(i)*2)) + newBlock[i] = charAt(i) } copy(newBlock[totalChars:], entryUTF16) newBlock[newLen-1] = 0 // final null terminator diff --git a/client/vnc/server/capture_fb_linux.go b/client/vnc/server/capture_fb_linux.go index f0f13e142..7fe6f3193 100644 --- a/client/vnc/server/capture_fb_linux.go +++ b/client/vnc/server/capture_fb_linux.go @@ -4,6 +4,7 @@ package server import ( "encoding/binary" + "errors" "fmt" "image" "sync" @@ -49,9 +50,14 @@ type fbVarScreenInfo struct { } // fbFixScreenInfo mirrors fb_fix_screeninfo. We only need LineLength. +// +// SmemStart and MmioStart are `unsigned long` in the kernel header, so they are +// pointer-sized: uintptr, not uint64. Spelling them uint64 would shift every +// field after SmemStart by four bytes on a 32-bit build and make LineLength +// read the wrong part of the ioctl's answer. type fbFixScreenInfo struct { IDStr [16]byte - SmemStart uint64 + SmemStart uintptr SmemLen uint32 Type uint32 TypeAux uint32 @@ -61,7 +67,7 @@ type fbFixScreenInfo struct { YWrapStep uint16 _pad0 uint16 LineLength uint32 - MmioStart uint64 + MmioStart uintptr MmioLen uint32 Accel uint32 Capabilities uint16 @@ -117,6 +123,10 @@ func NewFBCapturer(path string) (*FBCapturer, error) { unix.Close(fd) return nil, fmt.Errorf("unsupported framebuffer bpp: %d", bpp) } + if err := validateFBLayout(bpp, &vinfo); err != nil { + unix.Close(fd) + return nil, err + } size := int(finfo.LineLength) * int(vinfo.Yres) if size <= 0 { @@ -150,6 +160,40 @@ func NewFBCapturer(path string) (*FBCapturer, error) { return c, nil } +// validateFBLayout refuses a pixel layout the swizzlers do not implement. +// +// Each swizzler is written for one layout: 32bpp reads whole channels at the +// queried offsets, 24bpp reads packed B,G,R triplets, and 16bpp reads RGB565. +// A device reporting anything else (RGB888 at 24bpp, BGR565, a 10-bit channel) +// would be swizzled into wrong colours, which is worse than declining to +// capture at all, because nothing downstream can tell that it happened. +func validateFBLayout(bpp int, v *fbVarScreenInfo) error { + unsupported := func() error { + return fmt.Errorf("unsupported %dbpp framebuffer layout: r=%d/%d g=%d/%d b=%d/%d", + bpp, v.RedOffset, v.RedLen, v.GreenOffset, v.GreenLen, v.BlueOffset, v.BlueLen) + } + + switch bpp { + case 32: + // Offsets are honoured, channel widths are not. + if v.RedLen != 8 || v.GreenLen != 8 || v.BlueLen != 8 { + return unsupported() + } + case 24: + if v.RedLen != 8 || v.GreenLen != 8 || v.BlueLen != 8 || + v.BlueOffset != 0 || v.GreenOffset != 8 || v.RedOffset != 16 { + return unsupported() + } + case 16: + if v.RedOffset != 11 || v.RedLen != 5 || + v.GreenOffset != 5 || v.GreenLen != 6 || + v.BlueOffset != 0 || v.BlueLen != 5 { + return unsupported() + } + } + return nil +} + // Width returns the framebuffer width in pixels. func (c *FBCapturer) Width() int { return c.w } @@ -171,6 +215,12 @@ func (c *FBCapturer) CaptureInto(dst *image.RGBA) error { c.mu.Lock() defer c.mu.Unlock() + // Close unmaps but leaves w/h in place, so a capture arriving afterwards + // would pass the size check below and index into a nil mapping. + if c.mmap == nil { + return errors.New("framebuffer capturer is closed") + } + if dst.Rect.Dx() != c.w || dst.Rect.Dy() != c.h { return fmt.Errorf("dst size mismatch: dst=%dx%d fb=%dx%d", dst.Rect.Dx(), dst.Rect.Dy(), c.w, c.h) diff --git a/client/vnc/server/capture_fb_unix.go b/client/vnc/server/capture_fb_unix.go index 63351dcdb..faddefc9e 100644 --- a/client/vnc/server/capture_fb_unix.go +++ b/client/vnc/server/capture_fb_unix.go @@ -3,6 +3,7 @@ package server import ( + "errors" "image" "sync" ) @@ -18,8 +19,13 @@ type FBPoller struct { capturer *FBCapturer w, h int clients int32 + closed bool } +// errFBPollerClosed is what a call arriving after Close gets, instead of a +// freshly opened framebuffer. +var errFBPollerClosed = errors.New("framebuffer capturer closed") + // NewFBPoller returns a poller that opens path on first use. Empty path // defaults to /dev/fb0 on Linux and /dev/ttyv0 on FreeBSD. func NewFBPoller(path string) *FBPoller { @@ -86,10 +92,13 @@ func (p *FBPoller) CaptureInto(dst *image.RGBA) error { return p.capturer.CaptureInto(dst) } -// Close releases all framebuffer resources. +// Close releases all framebuffer resources. The poller stays closed: a later +// Capture or Width must not lazily reopen the device, the way X11Poller's done +// channel stops its own retry loop. func (p *FBPoller) Close() { p.mu.Lock() defer p.mu.Unlock() + p.closed = true if p.capturer != nil { p.capturer.Close() p.capturer = nil @@ -100,6 +109,9 @@ func (p *FBPoller) ensureCapturerLocked() error { if p.capturer != nil { return nil } + if p.closed { + return errFBPollerClosed + } c, err := NewFBCapturer(p.path) if err != nil { return err diff --git a/client/vnc/server/input_darwin.go b/client/vnc/server/input_darwin.go index 6bc30effd..68b3729e8 100644 --- a/client/vnc/server/input_darwin.go +++ b/client/vnc/server/input_darwin.go @@ -321,6 +321,10 @@ func ensureEventSource() uintptr { // MacInputInjector injects keyboard and mouse events via Core Graphics. type MacInputInjector struct { + // pointerMu serializes the pointer state below. One injector is shared by + // every attach-mode session, so two clients moving the mouse at once would + // otherwise interleave their button transitions and click counts. + pointerMu sync.Mutex lastButtons uint16 pbcopyPath string pbpastePath string @@ -673,6 +677,12 @@ func (m *MacInputInjector) InjectPointer(buttonMask uint16, px, py, serverW, ser return } x, y := scalePxToLogical(px, py, serverW, serverH) + + // Held across the dispatch: dispatchPointer derives each button transition + // from lastButtons and updates the click counters, so the read, the posted + // events and the write have to be one step. + m.pointerMu.Lock() + defer m.pointerMu.Unlock() m.dispatchPointer(src, buttonMask, x, y) m.lastButtons = buttonMask } diff --git a/client/vnc/server/shutdown_state.go b/client/vnc/server/shutdown_state.go index 9f3154e71..5250891ea 100644 --- a/client/vnc/server/shutdown_state.go +++ b/client/vnc/server/shutdown_state.go @@ -51,8 +51,10 @@ func (s *ShutdownState) Cleanup() error { return nil } -// isOurProcess verifies the PID still belongs to a VNC-related process -// by checking /proc//cmdline (Linux) or the process name. +// isOurProcess verifies the PID still belongs to a VNC-related process by +// matching desc against /proc//cmdline. A PID that no longer exists, or +// whose cmdline cannot be read, is treated as foreign and reported false, so +// cleanup never signals a process it cannot identify. func isOurProcess(pid int, desc string) bool { // Check if the process exists at all. if err := syscall.Kill(pid, 0); err != nil {