From 1940971b1dd105a2785eca122613fb9d70736a65 Mon Sep 17 00:00:00 2001 From: sreya Date: Tue, 21 Jun 2022 20:26:35 +0000 Subject: [PATCH 1/3] fix: cleanup reaper implementation - Clean up the agent/reaper API to be a more isolated and reusable package. --- agent/reaper/reaper.go | 28 ++++++++++++++++++++++++++++ agent/reaper/reaper_stub.go | 7 +------ agent/reaper/reaper_test.go | 11 +++++------ agent/reaper/reaper_unix.go | 34 +++++++++------------------------- cli/agent.go | 9 +++++++-- 5 files changed, 50 insertions(+), 39 deletions(-) create mode 100644 agent/reaper/reaper.go diff --git a/agent/reaper/reaper.go b/agent/reaper/reaper.go new file mode 100644 index 0000000000000..f2b87c4fed1ae --- /dev/null +++ b/agent/reaper/reaper.go @@ -0,0 +1,28 @@ +package reaper + +import "github.com/hashicorp/go-reap" + +type Option func(o *options) + +// WithExecArgs specifies the exec arguments for the fork exec call. +// By default the same arguments as the parent are used as dictates by +// os.Args. Since ForkReap calls a fork-exec it is the responsibility of +// the caller to avoid fork-bombing oneself. +func WithExecArgs(args ...string) Option { + return func(o *options) { + o.ExecArgs = args + } +} + +// WithPIDCallback sets the channel that reaped child process PIDs are pushed +// onto. +func WithPIDCallback(ch reap.PidCh) Option { + return func(o *options) { + o.PIDs = ch + } +} + +type options struct { + ExecArgs []string + PIDs reap.PidCh +} diff --git a/agent/reaper/reaper_stub.go b/agent/reaper/reaper_stub.go index b7516b76f59b2..7dda87a1b10df 100644 --- a/agent/reaper/reaper_stub.go +++ b/agent/reaper/reaper_stub.go @@ -4,16 +4,11 @@ package reaper import "github.com/hashicorp/go-reap" -// IsChild returns true if we're the forked process. -func IsChild() bool { - return false -} - // IsInitProcess returns true if the current process's PID is 1. func IsInitProcess() bool { return false } -func ForkReap(_ reap.PidCh) error { +func ForkReap(opt ...Option) error { return nil } diff --git a/agent/reaper/reaper_test.go b/agent/reaper/reaper_test.go index 8e9ca9143d3e3..8e23bec198c5c 100644 --- a/agent/reaper/reaper_test.go +++ b/agent/reaper/reaper_test.go @@ -24,17 +24,16 @@ func TestReap(t *testing.T) { t.Skip("Detected CI, skipping reaper tests") } - // Because we're forkexecing these tests will try to run twice... - if reaper.IsChild() { - t.Skip("I'm a child!") - } - // OK checks that's the reaper is successfully reaping // exited processes and passing the PIDs through the shared // channel. t.Run("OK", func(t *testing.T) { pids := make(reap.PidCh, 1) - err := reaper.ForkReap(pids) + err := reaper.ForkReap( + reaper.WithPIDCallback(pids), + // Provide some argument that immediately exits. + reaper.WithExecArgs("/bin/sh", "-c", "exit 0"), + ) require.NoError(t, err) cmd := exec.Command("tail", "-f", "/dev/null") diff --git a/agent/reaper/reaper_unix.go b/agent/reaper/reaper_unix.go index 8a2a8656857a4..4fa82ac83ba4d 100644 --- a/agent/reaper/reaper_unix.go +++ b/agent/reaper/reaper_unix.go @@ -3,7 +3,6 @@ package reaper import ( - "fmt" "os" "syscall" @@ -11,17 +10,6 @@ import ( "golang.org/x/xerrors" ) -// agentEnvMark is a simple environment variable that we use as a marker -// to indicated that the process is a child as opposed to the reaper. -// Since we are forkexec'ing we need to be able to differentiate between -// the two to avoid fork bombing ourselves. -const agentEnvMark = "CODER_DO_NOT_REAP" - -// IsChild returns true if we're the forked process. -func IsChild() bool { - return os.Getenv(agentEnvMark) != "" -} - // IsInitProcess returns true if the current process's PID is 1. func IsInitProcess() bool { return os.Getpid() == 1 @@ -33,19 +21,16 @@ func IsInitProcess() bool { // the reaper and an exec.Command waiting for its process to complete. // The provided 'pids' channel may be nil if the caller does not care about the // reaped children PIDs. -func ForkReap(pids reap.PidCh) error { - // Check if the process is the parent or the child. - // If it's the child we want to skip attempting to reap. - if IsChild() { - return nil +func ForkReap(opt ...Option) error { + opts := &options{ + ExecArgs: os.Args, } - go reap.ReapChildren(pids, nil, nil, nil) + for _, o := range opt { + o(opts) + } - args := os.Args - // This is simply done to help identify the real agent process - // when viewing in something like 'ps'. - args = append(args, "#Agent") + go reap.ReapChildren(opts.PIDs, nil, nil, nil) pwd, err := os.Getwd() if err != nil { @@ -54,8 +39,7 @@ func ForkReap(pids reap.PidCh) error { pattrs := &syscall.ProcAttr{ Dir: pwd, - // Add our marker for identifying the child process. - Env: append(os.Environ(), fmt.Sprintf("%s=true", agentEnvMark)), + Env: os.Environ(), Sys: &syscall.SysProcAttr{ Setsid: true, }, @@ -67,7 +51,7 @@ func ForkReap(pids reap.PidCh) error { } //#nosec G204 - pid, _ := syscall.ForkExec(args[0], args, pattrs) + pid, _ := syscall.ForkExec(opts.ExecArgs[0], opts.ExecArgs, pattrs) var wstatus syscall.WaitStatus _, err = syscall.Wait4(pid, &wstatus, 0, nil) diff --git a/cli/agent.go b/cli/agent.go index cfebbd1148ced..12adafec6652a 100644 --- a/cli/agent.go +++ b/cli/agent.go @@ -32,6 +32,7 @@ func workspaceAgent() *cobra.Command { auth string pprofEnabled bool pprofAddress string + noReap bool ) cmd := &cobra.Command{ Use: "agent", @@ -58,9 +59,12 @@ func workspaceAgent() *cobra.Command { // Spawn a reaper so that we don't accumulate a ton // of zombie processes. - if reaper.IsInitProcess() && !reaper.IsChild() && isLinux { + if reaper.IsInitProcess() && !noReap && isLinux { logger.Info(cmd.Context(), "spawning reaper process") - err := reaper.ForkReap(nil) + // Do not start a reaper on the child process. It's important + // to do this else we fork bomb ourselves. + args := append(os.Args, "--no-reap") + err := reaper.ForkReap(reaper.WithExecArgs(args...)) if err != nil { logger.Error(cmd.Context(), "failed to reap", slog.Error(err)) return xerrors.Errorf("fork reap: %w", err) @@ -182,6 +186,7 @@ func workspaceAgent() *cobra.Command { cliflag.StringVarP(cmd.Flags(), &auth, "auth", "", "CODER_AGENT_AUTH", "token", "Specify the authentication type to use for the agent") cliflag.BoolVarP(cmd.Flags(), &pprofEnabled, "pprof-enable", "", "CODER_AGENT_PPROF_ENABLE", false, "Enable serving pprof metrics on the address defined by --pprof-address.") + cliflag.BoolVarP(cmd.Flags(), &noReap, "no-reap", "", "", false, "Do not start a process reaper.") cliflag.StringVarP(cmd.Flags(), &pprofAddress, "pprof-address", "", "CODER_AGENT_PPROF_ADDRESS", "127.0.0.1:6060", "The address to serve pprof.") return cmd } From 09e61d5c04196876ed18fb641842c4a398f6dea1 Mon Sep 17 00:00:00 2001 From: sreya Date: Tue, 21 Jun 2022 20:36:34 +0000 Subject: [PATCH 2/3] stub woes --- agent/reaper/reaper_stub.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/agent/reaper/reaper_stub.go b/agent/reaper/reaper_stub.go index 7dda87a1b10df..538a7db71887a 100644 --- a/agent/reaper/reaper_stub.go +++ b/agent/reaper/reaper_stub.go @@ -2,8 +2,6 @@ package reaper -import "github.com/hashicorp/go-reap" - // IsInitProcess returns true if the current process's PID is 1. func IsInitProcess() bool { return false From fa115a36c4534bee55d90b690aa46fdf507904e5 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Tue, 21 Jun 2022 17:35:35 -0500 Subject: [PATCH 3/3] Update agent/reaper/reaper.go Co-authored-by: Dean Sheather --- agent/reaper/reaper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/reaper/reaper.go b/agent/reaper/reaper.go index f2b87c4fed1ae..d6cf5a5e78a0e 100644 --- a/agent/reaper/reaper.go +++ b/agent/reaper/reaper.go @@ -5,7 +5,7 @@ import "github.com/hashicorp/go-reap" type Option func(o *options) // WithExecArgs specifies the exec arguments for the fork exec call. -// By default the same arguments as the parent are used as dictates by +// By default the same arguments as the parent are used as dictated by // os.Args. Since ForkReap calls a fork-exec it is the responsibility of // the caller to avoid fork-bombing oneself. func WithExecArgs(args ...string) Option {