Fix framebuffer layout handling, capturer lifecycle and macOS pointer state races

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

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)

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

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
 }

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/<pid>/cmdline (Linux) or the process name.
+// isOurProcess verifies the PID still belongs to a VNC-related process by
+// matching desc against /proc/<pid>/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 {