vnc: fix deadlock + linter + bugbot review issues

Three issues from Cursor Bugbot's review on commit 1d41410, plus the
golangci-lint failures from the same CI run.

1. Deadlock in writeUpdate (HIGH severity).

   Conn.LockWrite acquired writeMu, then Conn.Flush at the end
   re-acquired the same non-reentrant mutex. Every VNC client hung on
   its first FramebufferUpdate; this is most likely the actual cause
   of the placeholder-forever symptom seen on-device.

   The intended pattern was a multi-call composite write with a
   private flushLocked variant — but in practice we have a strict
   single-writer model: serveClient drives the handshake on its own
   goroutine, then the dispatcher goroutine is the only writer
   afterward. The reader goroutine never writes. So the mutex adds
   no safety, only a deadlock. Strip writeMu, LockWrite, UnlockWrite,
   and flushLocked. Document the single-writer requirement on Conn.

2. SetEncodings allocated on the wire-supplied count (LOW severity).

   make([]EncodingType, count) was allocating up to 65 535 * 4 bytes
   regardless of the maxEncodings cap (256). Allocate based on the
   capped n; the loop still drains the full count from the wire so
   framing for the next message stays aligned.

3. Save button hidden when VNC was disabled in the UI (MEDIUM
   severity).

   The button lived inside the {settings.enabled && (...)} block, so
   unchecking the toggle hid the button — the disabled state could
   not be persisted. Move the button outside the conditional.

Linter follow-ups (golangci-lint v2.4 in CI):

- goimports re-aligned const blocks in keysym.go and placeholder.go.
- Removed the unused acquireVideoStream wrapper now that all callers
  pass an explicit codec.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: mcuelenaere

internal/rfb/conn.go

@@ -5,23 +5,21 @@ import (
 	"encoding/binary"
 	"io"
 	"net"
-	"sync"
 	"time"
 )
 
 // Conn wraps a net.Conn with framing buffers for the RFB protocol.
 //
-// Reads are not safe to issue from multiple goroutines, but writes are
-// serialized internally. The typical pattern is one goroutine reading
-// client messages and one writing server messages; the read side never
-// writes (except via the dedicated handshake helpers, which run before
-// either goroutine starts).
+// The Conn assumes a single-writer model: at most one goroutine
+// invokes write methods on the same Conn at a time. In typical use
+// the handshake runs sequentially on one goroutine and is then
+// followed by a per-connection dispatcher (also single goroutine)
+// while a separate reader goroutine only calls read methods. Do not
+// share writes between goroutines without external synchronization.
 type Conn struct {
 	nc net.Conn
 	r  *bufio.Reader
 	w  *bufio.Writer
-
-	writeMu sync.Mutex
 }
 
 // NewConn wraps a net.Conn with read/write buffers.
@@ -45,24 +43,8 @@ func (c *Conn) SetReadDeadline(t time.Time) error { return c.nc.SetReadDeadline(
 // SetWriteDeadline forwards to the underlying net.Conn.
 func (c *Conn) SetWriteDeadline(t time.Time) error { return c.nc.SetWriteDeadline(t) }
 
-// LockWrite acquires the write mutex. Callers MUST call UnlockWrite —
-// useful when emitting a multi-call composite message (e.g. a
-// FramebufferUpdate header followed by per-rectangle data).
-func (c *Conn) LockWrite() { c.writeMu.Lock() }
-
-// UnlockWrite releases the write mutex.
-func (c *Conn) UnlockWrite() { c.writeMu.Unlock() }
-
-// Flush flushes the write buffer to the underlying connection. It
-// acquires the write mutex internally.
-func (c *Conn) Flush() error {
-	c.writeMu.Lock()
-	defer c.writeMu.Unlock()
-	return c.w.Flush()
-}
-
-// flushLocked flushes assuming the write mutex is already held.
-func (c *Conn) flushLocked() error { return c.w.Flush() }
+// Flush flushes the write buffer to the underlying connection.
+func (c *Conn) Flush() error { return c.w.Flush() }
 
 // readByte reads one byte.
 func (c *Conn) readByte() (byte, error) { return c.r.ReadByte() }

internal/rfb/encodings.go

@@ -11,9 +11,9 @@ type Rect struct {
 }
 
 // BeginFramebufferUpdate writes the FramebufferUpdate message header
-// (RFC 6143 §7.6.1) for `n` rectangles. Caller MUST hold the write
-// mutex via Conn.LockWrite before calling and follow up with `n`
-// WriteRectHeader+payload pairs and then Conn.Flush+UnlockWrite.
+// (RFC 6143 §7.6.1) for `n` rectangles. The caller must follow up
+// with `n` WriteRectHeader+payload pairs and a final Conn.Flush.
+// The Conn type assumes a single-writer model — see Conn's doc.
 func (c *Conn) BeginFramebufferUpdate(n uint16) error {
 	if err := c.writeByte(byte(ServerMsgFramebufferUpdate)); err != nil {
 		return err

internal/rfb/encodings_test.go

@@ -13,13 +13,11 @@ func TestBeginFramebufferUpdate(t *testing.T) {
 
 	done := make(chan error, 1)
 	go func() {
-		c.LockWrite()
-		defer c.UnlockWrite()
 		if err := c.BeginFramebufferUpdate(3); err != nil {
 			done <- err
 			return
 		}
-		done <- c.flushLocked()
+		done <- c.Flush()
 	}()
 
 	if err := cliNC.SetReadDeadline(time.Now().Add(time.Second)); err != nil {
@@ -44,10 +42,8 @@ func TestWriteRectHeader(t *testing.T) {
 	c := NewConn(srvNC)
 
 	go func() {
-		c.LockWrite()
-		defer c.UnlockWrite()
 		_ = c.WriteRectHeader(Rect{X: 1, Y: 2, W: 1920, H: 1080, Encoding: EncodingOpenH264})
-		_ = c.flushLocked()
+		_ = c.Flush()
 	}()
 
 	out := make([]byte, 12)
@@ -75,14 +71,12 @@ func TestWriteOpenH264Rect(t *testing.T) {
 
 	nal := []byte{0x00, 0x00, 0x00, 0x01, 0x67, 0x42, 0x00, 0x1E} // dummy NAL
 	go func() {
-		c.LockWrite()
-		defer c.UnlockWrite()
 		_ = c.WriteOpenH264Rect(
 			Rect{X: 0, Y: 0, W: 1920, H: 1080, Encoding: EncodingOpenH264},
 			OpenH264FlagResetContext,
 			nal,
 		)
-		_ = c.flushLocked()
+		_ = c.Flush()
 	}()
 
 	out := make([]byte, 12+4+4+len(nal))
@@ -117,13 +111,11 @@ func TestWriteRawRect(t *testing.T) {
 
 	pixels := bytes.Repeat([]byte{0xAB}, 4*4*4) // 4x4 32bpp BGRA
 	go func() {
-		c.LockWrite()
-		defer c.UnlockWrite()
 		_ = c.WriteRawRect(
 			Rect{X: 0, Y: 0, W: 4, H: 4, Encoding: EncodingRaw},
 			pixels,
 		)
-		_ = c.flushLocked()
+		_ = c.Flush()
 	}()
 
 	out := make([]byte, 12+len(pixels))
@@ -147,10 +139,8 @@ func TestWriteDesktopSizeRect(t *testing.T) {
 	c := NewConn(srvNC)
 
 	go func() {
-		c.LockWrite()
-		defer c.UnlockWrite()
 		_ = c.WriteDesktopSizeRect(1920, 1080)
-		_ = c.flushLocked()
+		_ = c.Flush()
 	}()
 
 	out := make([]byte, 12)

internal/rfb/handshake.go

@@ -35,7 +35,7 @@ func (c *Conn) HandshakeServerVersion() (string, error) {
 	if err := c.writeRaw([]byte(ProtocolVersion38)); err != nil {
 		return "", fmt.Errorf("rfb: write server version: %w", err)
 	}
-	if err := c.flushLocked(); err != nil {
+	if err := c.Flush(); err != nil {
 		return "", fmt.Errorf("rfb: flush server version: %w", err)
 	}
 
@@ -71,7 +71,7 @@ func (c *Conn) OfferSecurityTypes(types []SecurityType) (SecurityType, error) {
 			return SecInvalid, err
 		}
 	}
-	if err := c.flushLocked(); err != nil {
+	if err := c.Flush(); err != nil {
 		return SecInvalid, err
 	}
 
@@ -102,15 +102,15 @@ func (c *Conn) SendSecurityFailure(reason string) error {
 	if err := c.writeRaw([]byte(reason)); err != nil {
 		return err
 	}
-	return c.flushLocked()
+	return c.Flush()
 }
 
 // SendSecurityResultOK signals successful authentication.
 func (c *Conn) SendSecurityResultOK() error {
 	if err := c.writeU32(SecResultOK); err != nil {
 		return err
 	}
-	return c.flushLocked()
+	return c.Flush()
 }
 
 // SendSecurityResultFailed signals failed authentication with an
@@ -126,7 +126,7 @@ func (c *Conn) SendSecurityResultFailed(reason string) error {
 	if err := c.writeRaw([]byte(reason)); err != nil {
 		return err
 	}
-	return c.flushLocked()
+	return c.Flush()
 }
 
 // ReadClientInit reads the 1-byte ClientInit (shared-flag).
@@ -157,5 +157,5 @@ func (c *Conn) SendServerInit(init ServerInit) error {
 	if err := c.writeRaw([]byte(init.Name)); err != nil {
 		return err
 	}
-	return c.flushLocked()
+	return c.Flush()
 }

internal/rfb/keysym.go

@@ -38,17 +38,17 @@ func IsModifierKeysym(keysym uint32) bool {
 // X11 keysym constants used by the table below. Source: X11/keysymdef.h.
 const (
 	// Whitespace / control
-	keysymBackSpace   uint32 = 0xff08
-	keysymTab         uint32 = 0xff09
-	keysymLinefeed    uint32 = 0xff0a
-	keysymClear       uint32 = 0xff0b
-	keysymReturn      uint32 = 0xff0d
-	keysymPause       uint32 = 0xff13
-	keysymScrollLock  uint32 = 0xff14
-	keysymSysReq      uint32 = 0xff15
-	keysymEscape      uint32 = 0xff1b
-	keysymDelete      uint32 = 0xffff
-	keysymPrint       uint32 = 0xff61
+	keysymBackSpace  uint32 = 0xff08
+	keysymTab        uint32 = 0xff09
+	keysymLinefeed   uint32 = 0xff0a
+	keysymClear      uint32 = 0xff0b
+	keysymReturn     uint32 = 0xff0d
+	keysymPause      uint32 = 0xff13
+	keysymScrollLock uint32 = 0xff14
+	keysymSysReq     uint32 = 0xff15
+	keysymEscape     uint32 = 0xff1b
+	keysymDelete     uint32 = 0xffff
+	keysymPrint      uint32 = 0xff61
 
 	// Cursor / nav
 	keysymHome     uint32 = 0xff50
@@ -114,51 +114,51 @@ const (
 // in [internal/usbgadget/hid_keyboard.go] for the modifiers; the rest
 // follow the standard 85/101/102-key layout.
 const (
-	hidA               byte = 0x04
-	hid1               byte = 0x1e
-	hidEnter           byte = 0x28
-	hidEscape          byte = 0x29
-	hidBackspace       byte = 0x2a
-	hidTab             byte = 0x2b
-	hidSpace           byte = 0x2c
-	hidMinus           byte = 0x2d
-	hidEqual           byte = 0x2e
-	hidLeftBracket     byte = 0x2f
-	hidRightBracket    byte = 0x30
-	hidBackslash       byte = 0x31
-	hidSemicolon       byte = 0x33
-	hidApostrophe      byte = 0x34
-	hidGrave           byte = 0x35
-	hidComma           byte = 0x36
-	hidPeriod          byte = 0x37
-	hidSlash           byte = 0x38
-	hidCapsLock        byte = 0x39
-	hidF1              byte = 0x3a
-	hidPrintScreen     byte = 0x46
-	hidScrollLock      byte = 0x47
-	hidPause           byte = 0x48
-	hidInsert          byte = 0x49
-	hidHome            byte = 0x4a
-	hidPageUp          byte = 0x4b
-	hidDelete          byte = 0x4c
-	hidEnd             byte = 0x4d
-	hidPageDown        byte = 0x4e
-	hidArrowRight      byte = 0x4f
-	hidArrowLeft       byte = 0x50
-	hidArrowDown       byte = 0x51
-	hidArrowUp         byte = 0x52
-	hidNumLock         byte = 0x53
-	hidKpDivide        byte = 0x54
-	hidKpMultiply      byte = 0x55
-	hidKpSubtract      byte = 0x56
-	hidKpAdd           byte = 0x57
-	hidKpEnter         byte = 0x58
-	hidKp1             byte = 0x59
-	hidKp0             byte = 0x62
-	hidKpDecimal       byte = 0x63
-	hidContextMenu     byte = 0x65
-	hidKpEqual         byte = 0x67
-	hidF13             byte = 0x68
+	hidA            byte = 0x04
+	hid1            byte = 0x1e
+	hidEnter        byte = 0x28
+	hidEscape       byte = 0x29
+	hidBackspace    byte = 0x2a
+	hidTab          byte = 0x2b
+	hidSpace        byte = 0x2c
+	hidMinus        byte = 0x2d
+	hidEqual        byte = 0x2e
+	hidLeftBracket  byte = 0x2f
+	hidRightBracket byte = 0x30
+	hidBackslash    byte = 0x31
+	hidSemicolon    byte = 0x33
+	hidApostrophe   byte = 0x34
+	hidGrave        byte = 0x35
+	hidComma        byte = 0x36
+	hidPeriod       byte = 0x37
+	hidSlash        byte = 0x38
+	hidCapsLock     byte = 0x39
+	hidF1           byte = 0x3a
+	hidPrintScreen  byte = 0x46
+	hidScrollLock   byte = 0x47
+	hidPause        byte = 0x48
+	hidInsert       byte = 0x49
+	hidHome         byte = 0x4a
+	hidPageUp       byte = 0x4b
+	hidDelete       byte = 0x4c
+	hidEnd          byte = 0x4d
+	hidPageDown     byte = 0x4e
+	hidArrowRight   byte = 0x4f
+	hidArrowLeft    byte = 0x50
+	hidArrowDown    byte = 0x51
+	hidArrowUp      byte = 0x52
+	hidNumLock      byte = 0x53
+	hidKpDivide     byte = 0x54
+	hidKpMultiply   byte = 0x55
+	hidKpSubtract   byte = 0x56
+	hidKpAdd        byte = 0x57
+	hidKpEnter      byte = 0x58
+	hidKp1          byte = 0x59
+	hidKp0          byte = 0x62
+	hidKpDecimal    byte = 0x63
+	hidContextMenu  byte = 0x65
+	hidKpEqual      byte = 0x67
+	hidF13          byte = 0x68
 
 	hidLeftControl  byte = 0xe0
 	hidLeftShift    byte = 0xe1

internal/rfb/messages.go

@@ -110,20 +110,20 @@ func (c *Conn) ReadClientMessage() (ClientMessage, error) {
 		if n > maxEncodings {
 			n = maxEncodings
 		}
-		encs := make([]EncodingType, count)
-		for i := uint16(0); i < count; i++ {
+		// Allocate based on the capped count, not the wire value, so
+		// a malicious client can't make us allocate up to 65535*4 bytes.
+		// We still drain the full wire stream below to keep framing
+		// aligned for the next message.
+		encs := make([]EncodingType, n)
+		for i := 0; i < int(count); i++ {
 			v, err := c.readS32()
 			if err != nil {
 				return nil, err
 			}
-			if int(i) < n {
+			if i < n {
 				encs[i] = EncodingType(v)
 			}
 		}
-		// If we capped, drop the trailing slots.
-		if int(count) > n {
-			encs = encs[:n]
-		}
 		return SetEncodingsMessage{Encodings: encs}, nil
 
 	case ClientMsgFramebufferUpdateRequest:

internal/rfb/placeholder.go

@@ -39,9 +39,9 @@ func PlaceholderImageWithMessage(width, height int, lines []string) []byte {
 	}
 
 	const (
-		bgR, bgG, bgB    uint8 = 0x10, 0x18, 0x40 // dark blue
-		fgR, fgG, fgB    uint8 = 0xFF, 0xFF, 0xFF
-		lineSpacingPx          = 18
+		bgR, bgG, bgB uint8 = 0x10, 0x18, 0x40 // dark blue
+		fgR, fgG, fgB uint8 = 0xFF, 0xFF, 0xFF
+		lineSpacingPx       = 18
 	)
 
 	rgba := image.NewRGBA(image.Rect(0, 0, width, height))

internal/rfb/security.go

@@ -26,7 +26,7 @@ func (c *Conn) PerformVNCAuth(password string) error {
 	if err := c.writeRaw(challenge[:]); err != nil {
 		return err
 	}
-	if err := c.flushLocked(); err != nil {
+	if err := c.Flush(); err != nil {
 		return err
 	}
 

ui/src/routes/devices.$id.settings.vnc.tsx

@@ -150,18 +150,20 @@ export default function SettingsVncRoute() {
             <div className="rounded-md border border-blue-200 bg-blue-50 p-3 text-xs text-blue-700 dark:border-blue-800 dark:bg-blue-900/20 dark:text-blue-300">
               {m.vnc_required_client_help()}
             </div>
-
-            <div className="flex items-center gap-x-2 pt-2">
-              <Button
-                size="SM"
-                theme="primary"
-                text={saving ? m.saving() : m.vnc_save_button()}
-                loading={saving}
-                onClick={handleSave}
-              />
-            </div>
           </>
         )}
+
+        {/* Save button is rendered unconditionally so users can also
+            persist the disabled state (unchecking the toggle). */}
+        <div className="flex items-center gap-x-2 pt-2">
+          <Button
+            size="SM"
+            theme="primary"
+            text={saving ? m.saving() : m.vnc_save_button()}
+            loading={saving}
+            onClick={handleSave}
+          />
+        </div>
       </div>
     </div>
   );