From 44918fce0dd31af115acb40220e872866cf9123a Mon Sep 17 00:00:00 2001 From: Alessandro Ros Date: Thu, 29 Feb 2024 22:42:11 +0100 Subject: [PATCH] when stopping hooks, stop all their subprocesses too (#3004) (#3087) --- go.mod | 2 +- internal/externalcmd/cmd.go | 13 ++++--- internal/externalcmd/cmd_unix.go | 14 +++---- internal/externalcmd/cmd_win.go | 67 +++++++++++++++++++++++++++----- 4 files changed, 73 insertions(+), 23 deletions(-) diff --git a/go.mod b/go.mod index b1212478..757e927c 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ require ( github.com/pion/webrtc/v3 v3.2.22 github.com/stretchr/testify v1.8.4 golang.org/x/crypto v0.20.0 + golang.org/x/sys v0.17.0 golang.org/x/term v0.17.0 gopkg.in/yaml.v2 v2.4.0 ) @@ -65,7 +66,6 @@ require ( github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778 // indirect golang.org/x/arch v0.3.0 // indirect golang.org/x/net v0.21.0 // indirect - golang.org/x/sys v0.17.0 // indirect golang.org/x/text v0.14.0 // indirect google.golang.org/protobuf v1.30.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/internal/externalcmd/cmd.go b/internal/externalcmd/cmd.go index 8f8ba0c8..45aad769 100644 --- a/internal/externalcmd/cmd.go +++ b/internal/externalcmd/cmd.go @@ -42,14 +42,12 @@ func NewCmd( ) *Cmd { // replace variables in both Linux and Windows, in order to allow using the // same commands on both of them. - expandEnv := func(variable string) string { + cmdstr = os.Expand(cmdstr, func(variable string) string { if value, ok := env[variable]; ok { return value } return os.Getenv(variable) - } - - cmdstr = os.Expand(cmdstr, expandEnv) + }) if onExit == nil { onExit = func(_ error) {} @@ -79,8 +77,13 @@ func (e *Cmd) Close() { func (e *Cmd) run() { defer e.pool.wg.Done() + env := append([]string(nil), os.Environ()...) + for key, val := range e.env { + env = append(env, key+"="+val) + } + for { - err := e.runOSSpecific() + err := e.runOSSpecific(env) if errors.Is(err, errTerminated) { return } diff --git a/internal/externalcmd/cmd_unix.go b/internal/externalcmd/cmd_unix.go index fa9b5831..ce4db958 100644 --- a/internal/externalcmd/cmd_unix.go +++ b/internal/externalcmd/cmd_unix.go @@ -13,7 +13,7 @@ import ( "github.com/kballard/go-shellquote" ) -func (e *Cmd) runOSSpecific() error { +func (e *Cmd) runOSSpecific(env []string) error { cmdParts, err := shellquote.Split(e.cmdstr) if err != nil { return err @@ -21,14 +21,13 @@ func (e *Cmd) runOSSpecific() error { cmd := exec.Command(cmdParts[0], cmdParts[1:]...) - cmd.Env = append([]string(nil), os.Environ()...) - for key, val := range e.env { - cmd.Env = append(cmd.Env, key+"="+val) - } - + cmd.Env = env cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + // set process group in order to allow killing subprocesses + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + err = cmd.Start() if err != nil { return err @@ -51,7 +50,8 @@ func (e *Cmd) runOSSpecific() error { select { case <-e.terminate: - syscall.Kill(cmd.Process.Pid, syscall.SIGINT) //nolint:errcheck + // the minus is needed to kill all subprocesses + syscall.Kill(-cmd.Process.Pid, syscall.SIGINT) //nolint:errcheck <-cmdDone return errTerminated diff --git a/internal/externalcmd/cmd_win.go b/internal/externalcmd/cmd_win.go index 36a76508..518d9e66 100644 --- a/internal/externalcmd/cmd_win.go +++ b/internal/externalcmd/cmd_win.go @@ -9,11 +9,52 @@ import ( "os/exec" "strings" "syscall" + "unsafe" "github.com/kballard/go-shellquote" + "golang.org/x/sys/windows" ) -func (e *Cmd) runOSSpecific() error { +// taken from +// https://gist.github.com/hallazzang/76f3970bfc949831808bbebc8ca15209 +func createProcessGroup() (windows.Handle, error) { + h, err := windows.CreateJobObject(nil, nil) + if err != nil { + return 0, err + } + + info := windows.JOBOBJECT_EXTENDED_LIMIT_INFORMATION{ + BasicLimitInformation: windows.JOBOBJECT_BASIC_LIMIT_INFORMATION{ + LimitFlags: windows.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, + }, + } + _, err = windows.SetInformationJobObject( + h, + windows.JobObjectExtendedLimitInformation, + uintptr(unsafe.Pointer(&info)), + uint32(unsafe.Sizeof(info))) + if err != nil { + return 0, err + } + + return h, nil +} + +func closeProcessGroup(h windows.Handle) error { + return windows.CloseHandle(h) +} + +func addProcessToGroup(h windows.Handle, p *os.Process) error { + type process struct { + Pid int + Handle uintptr + } + + return windows.AssignProcessToJobObject(h, + windows.Handle((*process)(unsafe.Pointer(p)).Handle)) +} + +func (e *Cmd) runOSSpecific(env []string) error { var cmd *exec.Cmd // from Golang documentation: @@ -39,15 +80,22 @@ func (e *Cmd) runOSSpecific() error { cmd = exec.Command(cmdParts[0], cmdParts[1:]...) } - cmd.Env = append([]string(nil), os.Environ()...) - for key, val := range e.env { - cmd.Env = append(cmd.Env, key+"="+val) - } - + cmd.Env = env cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - err := cmd.Start() + // create a process group to kill all subprocesses + g, err := createProcessGroup() + if err != nil { + return err + } + + err = cmd.Start() + if err != nil { + return err + } + + err = addProcessToGroup(g, cmd.Process) if err != nil { return err } @@ -69,13 +117,12 @@ func (e *Cmd) runOSSpecific() error { select { case <-e.terminate: - // on Windows, it's not possible to send os.Interrupt to a process. - // Kill() is the only supported way. - cmd.Process.Kill() + closeProcessGroup(g) <-cmdDone return errTerminated case c := <-cmdDone: + closeProcessGroup(g) if c != 0 { return fmt.Errorf("command exited with code %d", c) }