From 9672e04ce657e02bedb6136527119c45ee6a5b69 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Sat, 29 Aug 2026 13:43:12 +0200 Subject: [PATCH] Scope crash recovery to Linux, drain connection handlers before teardown --- client/vnc/server/server.go | 32 ++++++++++++ client/vnc/server/server_darwin.go | 2 + client/vnc/server/server_windows.go | 2 + client/vnc/server/shutdown_state.go | 18 +++---- client/vnc/server/shutdown_state_other.go | 49 +++++++++++++++++ client/vnc/server/shutdown_state_test.go | 2 +- client/vnc/server/shutdown_state_windows.go | 19 ------- .../networkmap/networkmapcompute.go | 52 +++++++++++-------- 8 files changed, 126 insertions(+), 50 deletions(-) create mode 100644 client/vnc/server/shutdown_state_other.go delete mode 100644 client/vnc/server/shutdown_state_windows.go diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index 8aef5e9b3..95b9d6164 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -183,6 +183,9 @@ type Server struct { ctx context.Context cancel context.CancelFunc vmgr virtualSessionManager + // handlers counts the in-flight connection handlers so Stop can wait for + // them before tearing down the capturer and injector they use. + handlers sync.WaitGroup // onVirtualProcesses forwards live virtual-session process records to the // daemon for crash recovery; nil when nothing is listening. onVirtualProcesses func(*ShutdownState) @@ -461,6 +464,26 @@ func (s *Server) closeActiveSessions() { } } +// handlerDrainTimeout bounds how long Stop waits for connection handlers. They +// are already unblocked by the socket closes above, so this only covers one +// wedged in a syscall; shutdown must not hang on it. +const handlerDrainTimeout = 5 * time.Second + +// awaitHandlers waits for the in-flight connection handlers, giving up after +// handlerDrainTimeout so a stuck one cannot hold shutdown open. +func (s *Server) awaitHandlers() { + done := make(chan struct{}) + go func() { + s.handlers.Wait() + close(done) + }() + select { + case <-done: + case <-time.After(handlerDrainTimeout): + s.log.Warnf("timed out after %s waiting for VNC connection handlers to finish", handlerDrainTimeout) + } +} + // trackConn registers a freshly accepted connection so Stop() can close // it even before the session is registered in sessionConns. func (s *Server) trackConn(c net.Conn) { @@ -758,6 +781,13 @@ func (s *Server) Stop() error { s.platformShutdown() } + // Let the handlers finish before the capturer and injector go away. Their + // sockets are closed above, so each is on its way out, and the last thing a + // session does is release the modifiers and buttons the client left held: + // closing the injector first would drop those and leave the host with a + // stuck Shift or mouse button. + s.awaitHandlers() + if c, ok := s.capturer.(interface{ Close() }); ok { c.Close() } @@ -805,7 +835,9 @@ func (s *Server) acceptLoop(ln net.Listener) { continue } enableTCPKeepAlive(conn, s.log) + s.handlers.Add(1) go func(c net.Conn) { + defer s.handlers.Done() defer s.releaseConnSlot() defer s.untrackConn(c) s.handleConnection(c) diff --git a/client/vnc/server/server_darwin.go b/client/vnc/server/server_darwin.go index 906c77854..c9c3b9d8e 100644 --- a/client/vnc/server/server_darwin.go +++ b/client/vnc/server/server_darwin.go @@ -62,7 +62,9 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) { enableTCPKeepAlive(conn, s.log) metered := newMetricsConn(conn, s.sessionRecorder) s.retrackConn(conn, metered) + s.handlers.Add(1) go func(c net.Conn) { + defer s.handlers.Done() defer s.releaseConnSlot() defer s.untrackConn(c) s.handleServiceConnection(c, mgr) diff --git a/client/vnc/server/server_windows.go b/client/vnc/server/server_windows.go index 38fcb7563..b9730f675 100644 --- a/client/vnc/server/server_windows.go +++ b/client/vnc/server/server_windows.go @@ -349,7 +349,9 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) { enableTCPKeepAlive(conn, s.log) metered := newMetricsConn(conn, s.sessionRecorder) s.retrackConn(conn, metered) + s.handlers.Add(1) go func(c net.Conn) { + defer s.handlers.Done() defer s.releaseConnSlot() defer s.untrackConn(c) s.handleServiceConnection(c, sm) diff --git a/client/vnc/server/shutdown_state.go b/client/vnc/server/shutdown_state.go index 6ceba076e..6169d618f 100644 --- a/client/vnc/server/shutdown_state.go +++ b/client/vnc/server/shutdown_state.go @@ -1,4 +1,4 @@ -//go:build unix +//go:build linux && !android package server @@ -138,23 +138,23 @@ func isOurProcess(proc sessionProcess, desc string) bool { } // processStartTime reads field 22 of /proc//stat, the process start time in -// clock ticks since boot. Parsed from the last ')' so a comm containing spaces -// or parentheses cannot shift the field offsets. +// clock ticks since boot. Parsed from the last ')' so an executable name +// containing spaces or parentheses cannot shift the field offsets. func processStartTime(pid int) (uint64, error) { raw, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) if err != nil { return 0, err } - commEnd := bytes.LastIndexByte(raw, ')') - if commEnd < 0 { + closeParen := bytes.LastIndexByte(raw, ')') + if closeParen < 0 { return 0, fmt.Errorf("malformed /proc/%d/stat", pid) } - // Fields after comm: state is field 3, so start time (field 22) is the - // 20th entry of the remainder. - fields := strings.Fields(string(raw[commEnd+1:])) + // Fields after the executable name: state is field 3, so start time + // (field 22) is the 20th entry of the remainder. + fields := strings.Fields(string(raw[closeParen+1:])) const startTimeOffset = 19 if len(fields) <= startTimeOffset { - return 0, fmt.Errorf("/proc/%d/stat has %d fields after comm", pid, len(fields)) + return 0, fmt.Errorf("/proc/%d/stat has only %d fields after the executable name", pid, len(fields)) } return strconv.ParseUint(fields[startTimeOffset], 10, 64) } diff --git a/client/vnc/server/shutdown_state_other.go b/client/vnc/server/shutdown_state_other.go new file mode 100644 index 000000000..dbeb2fca5 --- /dev/null +++ b/client/vnc/server/shutdown_state_other.go @@ -0,0 +1,49 @@ +//go:build !linux || android + +package server + +// Crash recovery for virtual-session processes is implemented on Linux only, +// where /proc gives a process identity stable enough to signal safely: a PID +// alone can have been reused by the time the daemon restarts, and Cleanup +// signals a whole process group. +// +// The type exists everywhere so the shared server Config can name it, and so +// the state manager can be handed one uniformly. +// +// What that leaves uncovered, per platform: +// +// - FreeBSD runs virtual sessions but mounts no procfs by default, so there +// is no start time to pin an identity to. Reaping on a PID alone could +// signal an unrelated process group, which is worse than leaving an X +// server behind, so nothing is reaped. +// - Windows has no virtual sessions. Its console agent is tied to the daemon +// by a Job Object with kill-on-close, which reaps it when the service dies. +// Assignment to that job can fail (the log says so at the time), and an +// agent that was never assigned does outlive a service crash; recovering +// from that would need a Windows process identity this does not implement. +// - macOS spawns a per-connection agent that exits with its connection. +type ShutdownState struct { + // Processes is never acted on here; the field exists so the virtual-session + // plumbing, which FreeBSD shares with Linux, compiles unchanged. + Processes map[string]sessionProcess `json:"processes,omitempty"` +} + +// Name returns the state name for the state manager. +func (s *ShutdownState) Name() string { + return "vnc_sessions_state" +} + +// Cleanup has nothing it can safely reap on these platforms. +func (s *ShutdownState) Cleanup() error { + return nil +} + +// sessionProcess is the placeholder identity these platforms record, which is +// to say none. +type sessionProcess struct{} + +// describeProcess records nothing: without a way to tell a reused PID from the +// original, a record would only invite an unsafe kill later. +func describeProcess(_ int) sessionProcess { + return sessionProcess{} +} diff --git a/client/vnc/server/shutdown_state_test.go b/client/vnc/server/shutdown_state_test.go index 040b41489..d12a48433 100644 --- a/client/vnc/server/shutdown_state_test.go +++ b/client/vnc/server/shutdown_state_test.go @@ -1,4 +1,4 @@ -//go:build unix +//go:build linux && !android package server diff --git a/client/vnc/server/shutdown_state_windows.go b/client/vnc/server/shutdown_state_windows.go deleted file mode 100644 index 0c03e14be..000000000 --- a/client/vnc/server/shutdown_state_windows.go +++ /dev/null @@ -1,19 +0,0 @@ -//go:build windows - -package server - -// ShutdownState exists on Windows only so the shared server Config can name it. -// Virtual sessions are an X11 feature: the Windows path proxies to an agent the -// service control manager owns, so there are no residual processes of ours to -// reap after a crash. -type ShutdownState struct{} - -// Name returns the state name for the state manager. -func (s *ShutdownState) Name() string { - return "vnc_sessions_state" -} - -// Cleanup has nothing to do on Windows. -func (s *ShutdownState) Cleanup() error { - return nil -} diff --git a/shared/management/networkmap/networkmapcompute.go b/shared/management/networkmap/networkmapcompute.go index 66aa71de2..11c6f3d70 100644 --- a/shared/management/networkmap/networkmapcompute.go +++ b/shared/management/networkmap/networkmapcompute.go @@ -16,6 +16,35 @@ type authRequirements struct { needAllowedUserIDs bool } +// collectFor records what rule needs to resolve its authorized users, for a +// peer the resolver will authorize under it. +// +// Both marker protocols resolve users the same way, so a VNC rule needs exactly +// what an SSH rule needs: the group-to-user mapping when the rule names groups, +// the account's allowed-user set when it names nobody, and nothing at all when +// it carries its own user. +func (a *authRequirements) collectFor(rule *nmdata.PolicyRule, peerSSHEnabled bool) { + isMarkerRule := rule.Protocol == string(types.PolicyRuleProtocolNetbirdSSH) || + rule.Protocol == string(types.PolicyRuleProtocolNetbirdVNC) + if !isMarkerRule { + if nmdata.PolicyRuleImpliesLegacySSH(rule) && peerSSHEnabled { + a.needAllowedUserIDs = true + } + return + } + + switch { + case len(rule.AuthorizedGroups) > 0: + for groupID := range rule.AuthorizedGroups { + a.neededGroupIDs[groupID] = struct{}{} + } + case rule.AuthorizedUser != "": + // Carries its own user; no lookup inputs needed. + default: + a.needAllowedUserIDs = true + } +} + // GetPeerNetworkMapComponents computes the peer's NetworkMapComponents from the // slim twin store. It mirrors the former Account.GetPeerNetworkMapComponents // exactly, operating on nmdata twins throughout — no Account reference and no @@ -369,27 +398,8 @@ func (nmd *NetworkMapData) getPeersGroupsPoliciesRoutes( // that appears only in Sources is authorized too. Gating this on // peerInDestinations alone leaves that peer's rule reaching the // resolver with none of the inputs it needs to name a user. - // - // Both marker protocols resolve users the same way, so VNC needs - // exactly what SSH needs. - receivingPeer := peerInDestinations || (rule.Bidirectional && peerInSources) - if !receivingPeer { - continue - } - if rule.Protocol == string(types.PolicyRuleProtocolNetbirdSSH) || - rule.Protocol == string(types.PolicyRuleProtocolNetbirdVNC) { - switch { - case len(rule.AuthorizedGroups) > 0: - for groupID := range rule.AuthorizedGroups { - authReqs.neededGroupIDs[groupID] = struct{}{} - } - case rule.AuthorizedUser != "": - // Carries its own user; no lookup inputs needed. - default: - authReqs.needAllowedUserIDs = true - } - } else if nmdata.PolicyRuleImpliesLegacySSH(rule) && peerSSHEnabled { - authReqs.needAllowedUserIDs = true + if peerInDestinations || (rule.Bidirectional && peerInSources) { + authReqs.collectFor(rule, peerSSHEnabled) } } if policyRelevant {