diff --git a/go/helpers/helpers.go b/go/helpers/helpers.go index af533111..e27aa759 100644 --- a/go/helpers/helpers.go +++ b/go/helpers/helpers.go @@ -25,6 +25,7 @@ import ( "path/filepath" "regexp" "strings" + "sync/atomic" "syscall" "time" @@ -253,6 +254,11 @@ func execute(path string, args []string, conf *ExecConfig) (ExecResult, error) { cmd.Stderr = &errbuf } + if conf.Timeout != nil { + // Give up on lingering pipe holders after the process exits. + cmd.WaitDelay = 10 * time.Second + } + start := time.Now() // Start command asynchronously deck.InfofA("Executing: %v \n", cmd.Args).With(deck.V(2)).Go() @@ -261,15 +267,19 @@ func execute(path string, args []string, conf *ExecConfig) (ExecResult, error) { } var timer *time.Timer + var done atomic.Bool // Create a timer that will kill the process if conf.Timeout != nil { timer = time.AfterFunc(*conf.Timeout, func() { - cmd.Process.Kill() + if !done.Load() { // taskkill /PID isn't safe after Wait (PID reuse) + killProcessTree(cmd.Process) + } }) } // Wait for execution result.ExitErr = cmd.Wait() + done.Store(true) // Populate the result object result.Stdout = outbuf.Bytes() diff --git a/go/helpers/helpers_stub.go b/go/helpers/helpers_stub.go index 0acc3209..6669c97a 100644 --- a/go/helpers/helpers_stub.go +++ b/go/helpers/helpers_stub.go @@ -17,3 +17,10 @@ package helpers +import "os" + +func killProcessTree(p *os.Process) { + if p != nil { + p.Kill() + } +} diff --git a/go/helpers/helpers_windows.go b/go/helpers/helpers_windows.go index ceef7e28..1ea9c5c8 100644 --- a/go/helpers/helpers_windows.go +++ b/go/helpers/helpers_windows.go @@ -19,7 +19,10 @@ package helpers import ( "fmt" + "os" + "os/exec" "regexp" + "strconv" "syscall" "time" "unsafe" @@ -86,6 +89,15 @@ const ( swpShowWindow = 0x0040 ) +func killProcessTree(p *os.Process) { + if p == nil { + return + } + // taskkill /T walks the PPID chain, so run it before killing the parent. + exec.Command(os.ExpandEnv(`${SystemRoot}\System32\taskkill.exe`), "/T", "/F", "/PID", strconv.Itoa(p.Pid)).Run() + p.Kill() +} + // GetServiceState interrogates local system services and returns their status and configuration. func GetServiceState(name string) (svc.Status, mgr.Config, error) { m, err := mgr.Connect() diff --git a/go/helpers/helpers_windows_test.go b/go/helpers/helpers_windows_test.go index ee91951f..dca43918 100644 --- a/go/helpers/helpers_windows_test.go +++ b/go/helpers/helpers_windows_test.go @@ -18,8 +18,10 @@ package helpers import ( + "bytes" "errors" "fmt" + "os" "regexp" "testing" "time" @@ -104,3 +106,25 @@ func TestHasAdmin(t *testing.T) { t.Skip("Test running with elevated privileges, skipping check for non-elevated context.") } } + +func TestExecuteTimeoutKillsChildProcessTree(t *testing.T) { + timeout := 200 * time.Millisecond + var stdout, stderr bytes.Buffer + start := time.Now() + _, err := execute( + os.ExpandEnv("${SystemRoot}\\System32\\cmd.exe"), + []string{"/c", PsPath, "-NoProfile", "-NonInteractive", "-NoLogo", "-Command", "Start-Sleep -Seconds 30"}, + &ExecConfig{ + Timeout: &timeout, + WriteStdOut: &stdout, + WriteStdErr: &stderr, + }, + ) + elapsed := time.Since(start) + if !errors.Is(err, ErrTimeout) { + t.Errorf("execute() error = %v, want %v", err, ErrTimeout) + } + if elapsed >= 8*time.Second { + t.Errorf("execute() elapsed = %v, want < 8s (child process tree was not killed promptly on timeout)", elapsed) + } +}