Scope crash recovery to Linux, drain connection handlers before teardown

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)

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)

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)

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/<pid>/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)
 }

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{}
+}

client/vnc/server/shutdown_state_test.go

@@ -1,4 +1,4 @@
-//go:build unix
+//go:build linux && !android
 
 package server
 

client/vnc/server/shutdown_state_windows.go

@@ -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
-}

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 {