From 17359cdc1e41c404f98f1a2debcbfb0461d334c2 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 20 May 2026 16:34:29 +0200 Subject: [PATCH] Fix VNC lint, 386 atomic alignment, and Sonar code smells --- client/vnc/server/agent_ipc.go | 2 +- client/vnc/server/cursor_windows.go | 57 ++++++++++--------- client/vnc/server/metrics_conn.go | 70 +++++++++++------------ client/vnc/server/session.go | 86 +++++++++++++++++------------ 4 files changed, 120 insertions(+), 95 deletions(-) diff --git a/client/vnc/server/agent_ipc.go b/client/vnc/server/agent_ipc.go index 8cf0ea8a6..1842653bf 100644 --- a/client/vnc/server/agent_ipc.go +++ b/client/vnc/server/agent_ipc.go @@ -1,4 +1,4 @@ -//go:build !js && !ios && !android +//go:build darwin || windows package server diff --git a/client/vnc/server/cursor_windows.go b/client/vnc/server/cursor_windows.go index 9e4c98554..491af2865 100644 --- a/client/vnc/server/cursor_windows.go +++ b/client/vnc/server/cursor_windows.go @@ -259,43 +259,50 @@ func decodeColorCursor(hbmColor, hbmMask windows.Handle) (*image.RGBA, error) { if hbmMask != 0 { mask, _ = dibCopy(hbmMask, w, h) } + hasAlpha := colorHasAlpha(color) img := image.NewRGBA(image.Rect(0, 0, int(w), int(h))) - hasAlpha := false - for i := 0; i < len(color); i += 4 { - if color[i+3] != 0 { - hasAlpha = true - break - } - } for y := int32(0); y < h; y++ { for x := int32(0); x < w; x++ { si := (y*w + x) * 4 - di := (y*w + x) * 4 b := color[si] g := color[si+1] r := color[si+2] - a := color[si+3] - if !hasAlpha { - a = 255 - if mask != nil { - // AND mask: 1 = transparent, 0 = opaque. The DIB - // representation we requested is 32bpp so each "bit" - // is a 4-byte entry; we use the first byte as the - // effective AND value. - if mask[si] != 0 { - a = 0 - } - } - } - img.Pix[di+0] = r - img.Pix[di+1] = g - img.Pix[di+2] = b - img.Pix[di+3] = a + a := pixelAlpha(color[si+3], si, mask, hasAlpha) + img.Pix[si+0] = r + img.Pix[si+1] = g + img.Pix[si+2] = b + img.Pix[si+3] = a } } return img, nil } +// colorHasAlpha reports whether any pixel of a 32bpp BGRA buffer has a +// non-zero alpha. Cursors authored without alpha leave the channel at 0 +// and rely on hbmMask for transparency. +func colorHasAlpha(color []byte) bool { + for i := 0; i < len(color); i += 4 { + if color[i+3] != 0 { + return true + } + } + return false +} + +// pixelAlpha returns the effective alpha for a colour-cursor pixel. When +// the source bitmap already has alpha we trust it; otherwise the AND mask +// decides (1 = transparent, 0 = opaque). The 32bpp DIB stores each AND +// bit as a 4-byte entry; the first byte carries the effective value. +func pixelAlpha(colorA byte, si int32, mask []byte, hasAlpha bool) byte { + if hasAlpha { + return colorA + } + if mask != nil && mask[si] != 0 { + return 0 + } + return 255 +} + // decodeMonoCursor handles legacy 1bpp cursors where hbmMask is twice as // tall as the visible sprite: rows [0..h) are the AND mask and rows [h..2h) // are the XOR mask. We render the visible half into RGBA, treating diff --git a/client/vnc/server/metrics_conn.go b/client/vnc/server/metrics_conn.go index 60f31101e..75749629a 100644 --- a/client/vnc/server/metrics_conn.go +++ b/client/vnc/server/metrics_conn.go @@ -41,15 +41,15 @@ type metricsConn struct { recorder func(SessionTick) - bytesOut uint64 - writes uint64 - writeNanos uint64 - largestPkt uint64 - fbus uint64 - fbuBytes uint64 - fbuRects uint64 - maxFBUBytes uint64 - maxFBURects uint64 + bytesOut atomic.Uint64 + writes atomic.Uint64 + writeNanos atomic.Uint64 + largestPkt atomic.Uint64 + fbus atomic.Uint64 + fbuBytes atomic.Uint64 + fbuRects atomic.Uint64 + maxFBUBytes atomic.Uint64 + maxFBURects atomic.Uint64 tickMu sync.Mutex tickStart time.Time @@ -104,10 +104,10 @@ func (m *metricsConn) flushTick(final bool) { m.tickMu.Lock() defer m.tickMu.Unlock() - b := atomic.LoadUint64(&m.bytesOut) - w := atomic.LoadUint64(&m.writes) - f := atomic.LoadUint64(&m.fbus) - ns := atomic.LoadUint64(&m.writeNanos) + b := m.bytesOut.Load() + w := m.writes.Load() + f := m.fbus.Load() + ns := m.writeNanos.Load() db := b - m.tickPrevB dw := w - m.tickPrevW @@ -115,9 +115,9 @@ func (m *metricsConn) flushTick(final bool) { dns := ns - m.tickPrevNS m.tickPrevB, m.tickPrevW, m.tickPrevF, m.tickPrevNS = b, w, f, ns - maxFBU := atomic.SwapUint64(&m.maxFBUBytes, 0) - maxRects := atomic.SwapUint64(&m.maxFBURects, 0) - maxPkt := atomic.SwapUint64(&m.largestPkt, 0) + maxFBU := m.maxFBUBytes.Swap(0) + maxRects := m.maxFBURects.Swap(0) + maxPkt := m.largestPkt.Swap(0) period := time.Since(m.tickStart) m.tickStart = time.Now() @@ -144,7 +144,7 @@ func (m *metricsConn) flushTick(final bool) { // throttle JPEG quality or skip frames in response. func (m *metricsConn) BusyFraction() float64 { now := time.Now() - ns := atomic.LoadUint64(&m.writeNanos) + ns := m.writeNanos.Load() m.busyMu.Lock() defer m.busyMu.Unlock() @@ -179,30 +179,30 @@ func isFBUHeader(p []byte) bool { func (m *metricsConn) Write(p []byte) (int, error) { if isFBUHeader(p) { - if b := atomic.SwapUint64(&m.fbuBytes, 0); b > 0 { - if b > atomic.LoadUint64(&m.maxFBUBytes) { - atomic.StoreUint64(&m.maxFBUBytes, b) + if b := m.fbuBytes.Swap(0); b > 0 { + if b > m.maxFBUBytes.Load() { + m.maxFBUBytes.Store(b) } } - if r := atomic.SwapUint64(&m.fbuRects, 0); r > 0 { - if r > atomic.LoadUint64(&m.maxFBURects) { - atomic.StoreUint64(&m.maxFBURects, r) + if r := m.fbuRects.Swap(0); r > 0 { + if r > m.maxFBURects.Load() { + m.maxFBURects.Store(r) } } - atomic.AddUint64(&m.fbus, 1) + m.fbus.Add(1) } t0 := time.Now() n, err := m.Conn.Write(p) - atomic.AddUint64(&m.writeNanos, uint64(time.Since(t0).Nanoseconds())) - atomic.AddUint64(&m.bytesOut, uint64(n)) - atomic.AddUint64(&m.writes, 1) + m.writeNanos.Add(uint64(time.Since(t0).Nanoseconds())) + m.bytesOut.Add(uint64(n)) + m.writes.Add(1) if !isFBUHeader(p) { - atomic.AddUint64(&m.fbuBytes, uint64(n)) - atomic.AddUint64(&m.fbuRects, 1) + m.fbuBytes.Add(uint64(n)) + m.fbuRects.Add(1) } - if uint64(n) > atomic.LoadUint64(&m.largestPkt) { - atomic.StoreUint64(&m.largestPkt, uint64(n)) + if uint64(n) > m.largestPkt.Load() { + m.largestPkt.Store(uint64(n)) } return n, err } @@ -213,11 +213,11 @@ func (m *metricsConn) Close() error { if m.recorder == nil { return } - if b := atomic.SwapUint64(&m.fbuBytes, 0); b > atomic.LoadUint64(&m.maxFBUBytes) { - atomic.StoreUint64(&m.maxFBUBytes, b) + if b := m.fbuBytes.Swap(0); b > m.maxFBUBytes.Load() { + m.maxFBUBytes.Store(b) } - if r := atomic.SwapUint64(&m.fbuRects, 0); r > atomic.LoadUint64(&m.maxFBURects) { - atomic.StoreUint64(&m.maxFBURects, r) + if r := m.fbuRects.Swap(0); r > m.maxFBURects.Load() { + m.maxFBURects.Store(r) } m.flushTick(true) }) diff --git a/client/vnc/server/session.go b/client/vnc/server/session.go index 11a177f7a..bd355a780 100644 --- a/client/vnc/server/session.go +++ b/client/vnc/server/session.go @@ -342,40 +342,7 @@ func (s *session) handleSetEncodings() error { return err } - var encs []string - s.encMu.Lock() - // Per RFC 6143 §7.5.3 each SetEncodings replaces the previous list, so - // reset all flags before re-applying. extClipCapsSent stays sticky so - // we don't re-emit Caps every refresh. - s.resetEncodingCaps() - for i := range int(numEnc) { - enc := int32(binary.BigEndian.Uint32(buf[i*4 : i*4+4])) - if name := s.applyEncoding(enc); name != "" { - encs = append(encs, name) - } - } - if s.useTight && (s.tight == nil || - s.tight.qualityLevel != s.clientJPEGQuality || - s.tight.compressLevel != s.clientZlibLevel) { - // When we replace an in-use tightState the client's stream-0 - // inflater carries dictionary state from the old deflater. Carry - // the pending-reset flag so the next Basic rect tells the client - // to reset its inflater before decoding. - replacing := s.tight != nil - s.tight = newTightStateWithLevels(s.clientJPEGQuality, s.clientZlibLevel) - if replacing { - s.tight.pendingZlibReset = true - } - } - sendExtClipCaps := s.clientSupportsExtClipboard && !s.extClipCapsSent - if sendExtClipCaps { - s.extClipCapsSent = true - } - sendExtMouseAck := s.clientSupportsExtMouseButtons && !s.extMouseAckSent - if sendExtMouseAck { - s.extMouseAckSent = true - } - s.encMu.Unlock() + encs, sendExtClipCaps, sendExtMouseAck := s.applyEncodings(buf, int(numEnc)) if len(encs) > 0 { s.log.Debugf("client supports encodings: %s", strings.Join(encs, ", ")) } @@ -392,6 +359,57 @@ func (s *session) handleSetEncodings() error { return nil } +// applyEncodings parses the SetEncodings body, updates capability flags, +// rebuilds the tight state if quality/level changed, and reports which +// one-shot acknowledgements still need to be sent. +func (s *session) applyEncodings(buf []byte, numEnc int) (names []string, sendExtClipCaps, sendExtMouseAck bool) { + s.encMu.Lock() + defer s.encMu.Unlock() + // Per RFC 6143 §7.5.3 each SetEncodings replaces the previous list, so + // reset all flags before re-applying. extClipCapsSent stays sticky so + // we don't re-emit Caps every refresh. + s.resetEncodingCaps() + for i := range numEnc { + enc := int32(binary.BigEndian.Uint32(buf[i*4 : i*4+4])) + if name := s.applyEncoding(enc); name != "" { + names = append(names, name) + } + } + s.refreshTightStateLocked() + sendExtClipCaps = s.clientSupportsExtClipboard && !s.extClipCapsSent + if sendExtClipCaps { + s.extClipCapsSent = true + } + sendExtMouseAck = s.clientSupportsExtMouseButtons && !s.extMouseAckSent + if sendExtMouseAck { + s.extMouseAckSent = true + } + return names, sendExtClipCaps, sendExtMouseAck +} + +// refreshTightStateLocked reallocates s.tight when the requested quality +// or compression level no longer matches the cached state. Caller holds +// s.encMu. +func (s *session) refreshTightStateLocked() { + if !s.useTight { + return + } + if s.tight != nil && + s.tight.qualityLevel == s.clientJPEGQuality && + s.tight.compressLevel == s.clientZlibLevel { + return + } + // When we replace an in-use tightState the client's stream-0 + // inflater carries dictionary state from the old deflater. Carry + // the pending-reset flag so the next Basic rect tells the client + // to reset its inflater before decoding. + replacing := s.tight != nil + s.tight = newTightStateWithLevels(s.clientJPEGQuality, s.clientZlibLevel) + if replacing { + s.tight.pendingZlibReset = true + } +} + // resetEncodingCaps zeroes the encoding capability flags so the next pass // through applyEncoding reflects exactly what the client just advertised. // Caller holds s.encMu. tight / copyRectDet allocations are kept; their