diff --git a/client/internal/engine.go b/client/internal/engine.go index f7c7e1862..de151592d 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -82,6 +82,12 @@ const ( PeerConnectionTimeoutMax = 45000 // ms PeerConnectionTimeoutMin = 30000 // ms disableAutoUpdate = "disabled" + + // systemInfoTimeout bounds how long the sync loop waits for system info / posture + // check gathering. The gathering runs uncancellable system calls (process scan, + // exec, os.Stat); without this bound a single stuck call freezes handleSync, and + // thus syncMsgMux, for as long as the call hangs (observed multi-minute freezes). + systemInfoTimeout = 15 * time.Second ) var ErrResetConnection = fmt.Errorf("reset connection") @@ -1084,11 +1090,22 @@ func (e *Engine) updateChecksIfNew(checks []*mgmProto.Checks) error { } e.checks = checks - info, err := system.GetInfoWithChecks(e.ctx, checks, e.overlayAddresses()...) - if err != nil { - log.Warnf("failed to get system info with checks: %v", err) - info = system.GetInfo(e.ctx) + info, ok := system.GetInfoWithChecksTimeout(e.ctx, systemInfoTimeout, checks, e.overlayAddresses()...) + if !ok { + // Gathering timed out; skip the meta sync this cycle rather than blocking the + // sync loop (and syncMsgMux) on a stuck system call. A later sync will retry. + return nil } + e.applyInfoFlags(info) + + if err := e.mgmClient.SyncMeta(info); err != nil { + return fmt.Errorf("could not sync meta: error %s", err) + } + return nil +} + +// applyInfoFlags sets the engine's config-derived feature flags on the gathered system info. +func (e *Engine) applyInfoFlags(info *system.Info) { info.SetFlags( e.config.RosenpassEnabled, e.config.RosenpassPermissive, @@ -1107,12 +1124,6 @@ func (e *Engine) updateChecksIfNew(checks []*mgmProto.Checks) error { e.config.EnableSSHRemotePortForwarding, e.config.DisableSSHAuth, ) - - if err := e.mgmClient.SyncMeta(info); err != nil { - log.Errorf("could not sync meta: error %s", err) - return err - } - return nil } // overlayAddresses returns our own WireGuard overlay address (v4 and v6) so it @@ -1272,31 +1283,15 @@ func (e *Engine) receiveManagementEvents() { e.shutdownWg.Add(1) go func() { defer e.shutdownWg.Done() - info, err := system.GetInfoWithChecks(e.ctx, e.checks, e.overlayAddresses()...) - if err != nil { - log.Warnf("failed to get system info with checks: %v", err) + info, ok := system.GetInfoWithChecksTimeout(e.ctx, systemInfoTimeout, e.checks, e.overlayAddresses()...) + if !ok { + // Gathering timed out; connect the stream with base info so management + // connectivity still comes up rather than blocking here. info = system.GetInfo(e.ctx) } - info.SetFlags( - e.config.RosenpassEnabled, - e.config.RosenpassPermissive, - &e.config.ServerSSHAllowed, - e.config.DisableClientRoutes, - e.config.DisableServerRoutes, - e.config.DisableDNS, - e.config.DisableFirewall, - e.config.BlockLANAccess, - e.config.BlockInbound, - e.config.DisableIPv6, - e.config.LazyConnectionEnabled, - e.config.EnableSSHRoot, - e.config.EnableSSHSFTP, - e.config.EnableSSHLocalPortForwarding, - e.config.EnableSSHRemotePortForwarding, - e.config.DisableSSHAuth, - ) + e.applyInfoFlags(info) - err = e.mgmClient.Sync(e.ctx, info, e.handleSync) + err := e.mgmClient.Sync(e.ctx, info, e.handleSync) if err != nil { // happens if management is unavailable for a long time. // We want to cancel the operation of the whole client diff --git a/client/system/info.go b/client/system/info.go index 27588859e..496b478a3 100644 --- a/client/system/info.go +++ b/client/system/info.go @@ -2,9 +2,11 @@ package system import ( "context" + "errors" "net/netip" "slices" "strings" + "time" log "github.com/sirupsen/logrus" "google.golang.org/grpc/metadata" @@ -174,7 +176,7 @@ func GetInfoWithChecks(ctx context.Context, checks []*proto.Checks, excludeIPs . processCheckPaths = append(processCheckPaths, check.GetFiles()...) } - files, err := checkFileAndProcess(processCheckPaths) + files, err := checkFileAndProcess(ctx, processCheckPaths) if err != nil { return nil, err } @@ -187,3 +189,43 @@ func GetInfoWithChecks(ctx context.Context, checks []*proto.Checks, excludeIPs . log.Debugf("all system information gathered successfully") return info, nil } + +// GetInfoWithChecksTimeout is GetInfoWithChecks bounded by timeout. Posture-check gathering +// runs uncancellable system calls (process enumeration, os.Stat), so calling it inline can +// block the caller for as long as such a call hangs. It runs in a goroutine instead: if it +// does not return within timeout the caller gets (nil, false) and should proceed with +// degraded behavior rather than block. On a gathering error it falls back to base GetInfo. +// +// The buffered channel lets the abandoned goroutine finish and exit once its blocking call +// returns, so it does not leak beyond the duration of that call. +func GetInfoWithChecksTimeout(ctx context.Context, timeout time.Duration, checks []*proto.Checks, excludeIPs ...netip.Addr) (*Info, bool) { + ctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + infoCh := make(chan *Info, 1) + go func() { + info, err := GetInfoWithChecks(ctx, checks, excludeIPs...) + if err != nil { + if ctx.Err() != nil { + return + } + log.Warnf("failed to get system info with checks: %v", err) + info = GetInfo(ctx) + info.removeAddresses(excludeIPs...) + } + infoCh <- info + }() + + select { + case info := <-infoCh: + return info, true + case <-ctx.Done(): + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + log.Warnf("gathering system info with checks timed out after %s", timeout) + } else { + // Parent context canceled (e.g. shutdown), not a timeout. + log.Warnf("gathering system info with checks canceled: %v", ctx.Err()) + } + return nil, false + } +} diff --git a/client/system/info_android.go b/client/system/info_android.go index 794ff15ed..3c71573bb 100644 --- a/client/system/info_android.go +++ b/client/system/info_android.go @@ -50,7 +50,7 @@ func GetInfo(ctx context.Context) *Info { } // checkFileAndProcess checks if the file path exists and if a process is running at that path. -func checkFileAndProcess(paths []string) ([]File, error) { +func checkFileAndProcess(_ context.Context, _ []string) ([]File, error) { return []File{}, nil } diff --git a/client/system/info_darwin.go b/client/system/info_darwin.go index 4a31920ec..e7bf367f6 100644 --- a/client/system/info_darwin.go +++ b/client/system/info_darwin.go @@ -32,7 +32,7 @@ func GetInfo(ctx context.Context) *Info { sysName := string(bytes.Split(utsname.Sysname[:], []byte{0})[0]) machine := string(bytes.Split(utsname.Machine[:], []byte{0})[0]) release := string(bytes.Split(utsname.Release[:], []byte{0})[0]) - swVersion, err := exec.Command("sw_vers", "-productVersion").Output() + swVersion, err := exec.CommandContext(ctx, "sw_vers", "-productVersion").Output() if err != nil { log.Warnf("got an error while retrieving macOS version with sw_vers, error: %s. Using darwin version instead.\n", err) swVersion = []byte(release) diff --git a/client/system/info_ios.go b/client/system/info_ios.go index ad42b1edf..1b0c084b3 100644 --- a/client/system/info_ios.go +++ b/client/system/info_ios.go @@ -105,7 +105,7 @@ func isDuplicated(addresses []NetworkAddress, addr NetworkAddress) bool { } // checkFileAndProcess checks if the file path exists and if a process is running at that path. -func checkFileAndProcess(paths []string) ([]File, error) { +func checkFileAndProcess(_ context.Context, _ []string) ([]File, error) { return []File{}, nil } diff --git a/client/system/info_js.go b/client/system/info_js.go index 994d439a7..f32532881 100644 --- a/client/system/info_js.go +++ b/client/system/info_js.go @@ -103,7 +103,7 @@ func collectLocationInfo(info *Info) { } } -func checkFileAndProcess(_ []string) ([]File, error) { +func checkFileAndProcess(_ context.Context, _ []string) ([]File, error) { return []File{}, nil } diff --git a/client/system/info_test.go b/client/system/info_test.go index dcda18e61..a7fa02197 100644 --- a/client/system/info_test.go +++ b/client/system/info_test.go @@ -4,6 +4,7 @@ import ( "context" "net/netip" "testing" + "time" "github.com/stretchr/testify/assert" "google.golang.org/grpc/metadata" @@ -35,6 +36,20 @@ func Test_CustomHostname(t *testing.T) { assert.Equal(t, want, got.Hostname) } +func TestGetInfoWithChecksTimeout_Success(t *testing.T) { + info, ok := GetInfoWithChecksTimeout(context.Background(), 30*time.Second, nil) + assert.True(t, ok, "expected gathering to complete within the timeout") + assert.NotNil(t, info) +} + +func TestGetInfoWithChecksTimeout_Timeout(t *testing.T) { + // A 1ns budget expires before the (real) system-info gathering can finish, so the + // caller must get (nil, false) instead of blocking on the in-flight goroutine. + info, ok := GetInfoWithChecksTimeout(context.Background(), time.Nanosecond, nil) + assert.False(t, ok, "expected timeout to be reported") + assert.Nil(t, info) +} + func Test_NetAddresses(t *testing.T) { addr, err := networkAddresses() if err != nil { diff --git a/client/system/process.go b/client/system/process.go index 87e21eb9d..07f69a212 100644 --- a/client/system/process.go +++ b/client/system/process.go @@ -3,24 +3,30 @@ package system import ( + "context" "os" "slices" "github.com/shirou/gopsutil/v3/process" ) -// getRunningProcesses returns a list of running process paths. -func getRunningProcesses() ([]string, error) { - processIDs, err := process.Pids() +// getRunningProcesses returns a list of running process paths. The context bounds the work: +// the per-PID loop bails as soon as ctx is done, and the gopsutil calls honor it where they +// can, so a stuck enumeration cannot run unbounded. +func getRunningProcesses(ctx context.Context) ([]string, error) { + processIDs, err := process.PidsWithContext(ctx) if err != nil { return nil, err } processMap := make(map[string]bool) for _, pID := range processIDs { + if err := ctx.Err(); err != nil { + return nil, err + } p := &process.Process{Pid: pID} - path, _ := p.Exe() + path, _ := p.ExeWithContext(ctx) if path != "" { processMap[path] = false } @@ -35,18 +41,21 @@ func getRunningProcesses() ([]string, error) { } // checkFileAndProcess checks if the file path exists and if a process is running at that path. -func checkFileAndProcess(paths []string) ([]File, error) { +func checkFileAndProcess(ctx context.Context, paths []string) ([]File, error) { files := make([]File, len(paths)) if len(paths) == 0 { return files, nil } - runningProcesses, err := getRunningProcesses() + runningProcesses, err := getRunningProcesses(ctx) if err != nil { return nil, err } for i, path := range paths { + if err := ctx.Err(); err != nil { + return nil, err + } file := File{Path: path} _, err := os.Stat(path) diff --git a/client/system/process_test.go b/client/system/process_test.go index 505808a9e..44a1c8ba0 100644 --- a/client/system/process_test.go +++ b/client/system/process_test.go @@ -1,6 +1,7 @@ package system import ( + "context" "testing" "github.com/shirou/gopsutil/v3/process" @@ -9,7 +10,7 @@ import ( func Benchmark_getRunningProcesses(b *testing.B) { b.Run("getRunningProcesses new", func(b *testing.B) { for i := 0; i < b.N; i++ { - ps, err := getRunningProcesses() + ps, err := getRunningProcesses(context.Background()) if err != nil { b.Fatalf("unexpected error: %v", err) } @@ -29,12 +30,38 @@ func Benchmark_getRunningProcesses(b *testing.B) { } } }) - s, _ := getRunningProcesses() + s, _ := getRunningProcesses(context.Background()) b.Logf("getRunningProcesses returned %d processes", len(s)) s, _ = getRunningProcessesOld() b.Logf("getRunningProcessesOld returned %d processes", len(s)) } +func TestCheckFileAndProcess_ContextCanceled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // With a canceled context and non-empty paths the gathering must bail with an error + // instead of running the (potentially blocking) process scan / stat loop. + if _, err := checkFileAndProcess(ctx, []string{"/does/not/exist"}); err == nil { + t.Fatal("expected error on canceled context, got nil") + } +} + +func TestCheckFileAndProcess_EmptyPaths(t *testing.T) { + // No check paths means no work to do: it must return immediately with no error, + // even on a canceled context (nothing to scan or stat). + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + files, err := checkFileAndProcess(ctx, nil) + if err != nil { + t.Fatalf("unexpected error for empty paths: %v", err) + } + if len(files) != 0 { + t.Fatalf("expected no files, got %d", len(files)) + } +} + func getRunningProcessesOld() ([]string, error) { processes, err := process.Processes() if err != nil {