From 988d521f25e85ebef56e3d5f0efb5e4407302099 Mon Sep 17 00:00:00 2001 From: criyle Date: Thu, 15 Aug 2019 23:41:32 -0700 Subject: [PATCH] change to cancelable function style & release resource in run_program --- cmd/run_program/main.go | 133 +++++++++++++++++++----------------- deamon/container_init.go | 20 ++++-- deamon/master_cmd_execve.go | 39 +++++------ forkexec/fork.go | 1 + go.mod | 2 +- go.sum | 4 +- 6 files changed, 107 insertions(+), 92 deletions(-) diff --git a/cmd/run_program/main.go b/cmd/run_program/main.go index d553403..53bd76b 100644 --- a/cmd/run_program/main.go +++ b/cmd/run_program/main.go @@ -18,7 +18,18 @@ import ( ) const ( - pathEnv = "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" + pathEnv = "PATH=/usr/local/bin:/usr/bin:/bin" +) + +var ( + addReadable, addWritable, addRawReadable, addRawWritable arrayFlags + allowProc, unsafe, showDetails, namespace, useCGroup, memfile bool + timeLimit, realTimeLimit, memoryLimit, outputLimit, stackLimit uint64 + inputFileName, outputFileName, errorFileName, workPath string + + useDeamon bool + pType, result string + args []string ) // Runner can be ptraced runner or namespaced runner @@ -33,21 +44,6 @@ func printUsage() { } func main() { - var ( - addReadable, addWritable, addRawReadable, addRawWritable arrayFlags - allowProc, unsafe, showDetails, namespace, useCGroup, memfile bool - useDeamon bool - pType, result string - timeLimit, realTimeLimit, memoryLimit, outputLimit, stackLimit uint64 - inputFileName, outputFileName, errorFileName, workPath string - - runner Runner - cg *cgroup.CGroup - err error - execFile uintptr - rt specs.TraceResult - ) - deamon.ContainerInit() flag.Usage = printUsage @@ -75,17 +71,11 @@ func main() { flag.BoolVar(&useDeamon, "deamon", false, "Use deamon container to execute file") flag.Parse() - args := flag.Args() + args = flag.Args() if len(args) == 0 { printUsage() } - println := func(v ...interface{}) { - if showDetails { - fmt.Fprintln(os.Stderr, v...) - } - } - if realTimeLimit < timeLimit { realTimeLimit = timeLimit + 2 } @@ -95,6 +85,33 @@ func main() { if workPath == "" { workPath, _ = os.Getwd() } + rt, f, err := run() + + if err != nil { + debug(err) + c, ok := err.(specs.TraceCode) + if !ok { + c = specs.TraceCodeFatal + } + // Handle fatal error from trace + fmt.Fprintf(f, "%d %d %d %d\n", int(c), rt.UserTime, rt.UserMem, rt.ExitCode) + if c == specs.TraceCodeFatal { + os.Exit(1) + } + } else { + fmt.Fprintf(f, "%d %d %d %d\n", 0, rt.UserTime, rt.UserMem, rt.ExitCode) + } +} + +func run() (*specs.TraceResult, *os.File, error) { + var ( + runner Runner + cg *cgroup.CGroup + err error + execFile uintptr + rt specs.TraceResult + f *os.File + ) addRead := runconfig.GetExtraSet(addReadable, addRawReadable) addWrite := runconfig.GetExtraSet(addWritable, addRawWritable) @@ -103,11 +120,11 @@ func main() { if useCGroup { cg, err = cgroup.NewCGroup("run_program") if err != nil { - panic(err) + return nil, nil, err } defer cg.Destroy() if err = cg.SetMemoryLimitInBytes(memoryLimit << 20); err != nil { - panic(err) + return nil, nil, err } } @@ -123,23 +140,22 @@ func main() { if memfile { fin, err := os.Open(args[0]) if err != nil { - panic(fmt.Sprintln("filed to open args[0]", err)) + return nil, nil, fmt.Errorf("filed to open args[0]: %v", err) } execf, err := memfd.DupToMemfd("run_program", fin) if err != nil { - panic(fmt.Sprintln("dup to memfd failed", err)) + return nil, nil, fmt.Errorf("dup to memfd failed: %v", err) } fin.Close() defer execf.Close() execFile = execf.Fd() - println("memfd: ", execFile) + debug("memfd: ", execFile) } // open input / output / err files files, err := prepareFiles(inputFileName, outputFileName, errorFileName) if err != nil { - println(err) - os.Exit(1) + return nil, nil, fmt.Errorf("failed to prepare files: %v", err) } defer closeFiles(files) @@ -164,18 +180,22 @@ func main() { sTime := time.Now() root, err := ioutil.TempDir("", "dm") if err != nil { - panic("cannot make temp root for deamon namespace") + return nil, nil, fmt.Errorf("cannot make temp root for deamon namespace: %v", err) } + defer os.RemoveAll(root) + m, err := deamon.New(root) if err != nil { - panic(fmt.Sprintln("failed to new master", err)) + return nil, nil, fmt.Errorf("failed to new master: %v", err) } err = m.Ping() if err != nil { - panic(fmt.Sprintln("failed to ping deamon", err)) + return nil, nil, fmt.Errorf("failed to ping deamon: %v", err) } + rTime := time.Now() - s, err := m.Execve(&deamon.ExecveParam{ + done := make(chan struct{}) + s, err := m.Execve(done, &deamon.ExecveParam{ Args: args, Envv: []string{pathEnv}, Fds: fds, @@ -184,18 +204,16 @@ func main() { SyncFunc: syncFunc, }) if err != nil { - panic(fmt.Sprintln("failed to execve", err)) + return nil, nil, fmt.Errorf("failed to execve: %v", err) } tC := time.After(time.Duration(int64(realTimeLimit) * int64(time.Second))) select { case <-tC: - s.Kill <- 1 - rt = <-s.Wait + close(done) + rt = <-s - case rt = <-s.Wait: - s.Kill <- 1 + case rt = <-s: } - println(rt) eTime := time.Now() rt.SetUpTime = int64(rTime.Sub(sTime)) rt.RunningTime = int64(eTime.Sub(rTime)) @@ -204,8 +222,10 @@ func main() { h.SyscallAllow = append(h.SyscallAllow, h.SyscallTrace...) root, err := ioutil.TempDir("", "ns") if err != nil { - panic("cannot make temp root for new namespace") + return nil, nil, fmt.Errorf("cannot make temp root for new namespace") } + defer os.RemoveAll(root) + runner = &rununshared.RunUnshared{ Args: h.Args, Env: []string{pathEnv}, @@ -253,16 +273,14 @@ func main() { } } - var f *os.File if result == "stdout" { f = os.Stdout } else if result == "stderr" { f = os.Stderr } else { - f, err := os.OpenFile(result, os.O_WRONLY|os.O_CREATE, 0755) + f, err := os.Create(result) if err != nil { - println("Failed to open result file: ", err) - os.Exit(1) + return nil, nil, fmt.Errorf("Failed to open result file: %v", err) } defer f.Close() } @@ -271,33 +289,26 @@ func main() { if runner != nil { rt, err = runner.Start() } - println("results:", rt, err) + debug("results:", rt, err) if useCGroup { cpu, err := cg.CpuacctUsage() if err != nil { - panic(err) + return nil, nil, fmt.Errorf("cgroup cpu: %v", err) } memory, err := cg.MemoryMaxUsageInBytes() if err != nil { - panic(err) + return nil, nil, fmt.Errorf("cgroup memory: %v", err) } - println("cgroup: cpu: ", cpu, " memory: ", memory) + debug("cgroup: cpu: ", cpu, " memory: ", memory) rt.UserTime = cpu / uint64(time.Millisecond) rt.UserMem = memory >> 10 } + return &rt, f, nil +} - if err != nil { - c, ok := err.(specs.TraceCode) - if !ok { - c = specs.TraceCodeFatal - } - // Handle fatal error from trace - fmt.Fprintf(f, "%d %d %d %d\n", int(c), rt.UserTime, rt.UserMem, rt.ExitCode) - if c == specs.TraceCodeFatal { - os.Exit(1) - } - } else { - fmt.Fprintf(f, "%d %d %d %d\n", 0, rt.UserTime, rt.UserMem, rt.ExitCode) +func debug(v ...interface{}) { + if showDetails { + fmt.Fprintln(os.Stderr, v...) } } diff --git a/deamon/container_init.go b/deamon/container_init.go index 6b3e988..e5db279 100644 --- a/deamon/container_init.go +++ b/deamon/container_init.go @@ -186,22 +186,28 @@ func handleExecve(s *unixsocket.Socket, cmd *Cmd, msg *unixsocket.Msg) error { return sendErrorReply(s, "execve: %v", err) } - done := make(chan int) + // done is to signal kill goroutine exits + done := make(chan struct{}) + // waitDone is to signal kill goroutine to collect zombies + waitDone := make(chan struct{}) + // recv kill go func() { + // signal done + defer close(done) // msg must be kill recvCmd(s) // kill all syscall.Kill(-1, syscall.SIGKILL) + // make sure collect zombie does not consume the exit status + <-waitDone // collect zombies for { - _, err = syscall.Wait4(-1, nil, 0, nil) + _, err = syscall.Wait4(-1, nil, syscall.WNOHANG, nil) if err != nil { break } } - // signal done - close(done) }() // wait pid @@ -209,6 +215,8 @@ func handleExecve(s *unixsocket.Socket, cmd *Cmd, msg *unixsocket.Msg) error { loop: for { _, err = syscall.Wait4(pid, &wstatus, 0, nil) + // sync with kill goroutine + close(waitDone) if err != nil { sendErrorReply(s, "execve: wait4 %v", err) break loop @@ -223,9 +231,7 @@ loop: var status specs.TraceCode switch wstatus.Signal() { // kill signal treats as TLE - case syscall.SIGKILL: - status = specs.TraceCodeTLE - case syscall.SIGXCPU: + case syscall.SIGXCPU, syscall.SIGKILL: status = specs.TraceCodeTLE case syscall.SIGXFSZ: status = specs.TraceCodeOLE diff --git a/deamon/master_cmd_execve.go b/deamon/master_cmd_execve.go index 57449ed..9f3ada8 100644 --- a/deamon/master_cmd_execve.go +++ b/deamon/master_cmd_execve.go @@ -21,16 +21,9 @@ type ExecveParam struct { SyncFunc func(pid int) error } -// ExecveStatus is the return value for execve -// Wait channel will produce the waitpid exit status -// Kill channel will kill the process if value received -type ExecveStatus struct { - Wait <-chan specs.TraceResult - Kill chan<- int -} - // Execve runs process inside container -func (m *Master) Execve(param *ExecveParam) (*ExecveStatus, error) { +// accepts done for cancelation +func (m *Master) Execve(done <-chan struct{}, param *ExecveParam) (<-chan specs.TraceResult, error) { var files []int if param.ExecFile > 0 { files = append(files, int(param.ExecFile)) @@ -57,33 +50,37 @@ func (m *Master) Execve(param *ExecveParam) (*ExecveStatus, error) { m.sendCmd(&Cmd{Cmd: cmdKill}, nil) return nil, fmt.Errorf("execve: no pid recved or error(%v)", reply.Error) } - if err := param.SyncFunc(int(msg.Cred.Pid)); err != nil { - m.sendCmd(&Cmd{Cmd: cmdKill}, nil) - return nil, fmt.Errorf("execve: syncfunc failed(%v)", err) + if param.SyncFunc != nil { + if err := param.SyncFunc(int(msg.Cred.Pid)); err != nil { + m.sendCmd(&Cmd{Cmd: cmdKill}, nil) + return nil, fmt.Errorf("execve: syncfunc failed(%v)", err) + } } if err := m.sendCmd(&Cmd{Cmd: cmdOk}, nil); err != nil { return nil, fmt.Errorf("execve: ok failed(%v)", err) } - wait := make(chan specs.TraceResult) - kill := make(chan int) + // make sure goroutine not leaked (blocked) even if result is not consumed + wait := make(chan specs.TraceResult, 1) + waitDone := make(chan struct{}) // Wait go func() { + defer close(wait) reply2, _, _ := m.recvReply() + close(waitDone) wait <- specs.TraceResult{ ExitCode: reply2.ExitStatus, TraceStatus: reply2.TraceStatus, } // done signal (should recv after kill) m.recvReply() - close(wait) }() - // Kill + // Kill (if wait is done, a kill message need to be send to collect zombies) go func() { - <-kill + select { + case <-done: + case <-waitDone: + } m.sendCmd(&Cmd{Cmd: cmdKill}, nil) }() - return &ExecveStatus{ - Wait: wait, - Kill: kill, - }, nil + return wait, nil } diff --git a/forkexec/fork.go b/forkexec/fork.go index 8d63d7e..0363fca 100644 --- a/forkexec/fork.go +++ b/forkexec/fork.go @@ -137,6 +137,7 @@ func (r *Runner) Start() (int, error) { // if stopped before execve, then do not wait until execve if r.Ptrace && r.Seccomp != nil || r.StopBeforeSeccomp { + unix.Close(p[0]) return int(pid), nil } diff --git a/go.mod b/go.mod index 70189ab..073217c 100644 --- a/go.mod +++ b/go.mod @@ -4,5 +4,5 @@ go 1.12 require ( github.com/seccomp/libseccomp-golang v0.9.1 - golang.org/x/sys v0.0.0-20190804053845-51ab0e2deafa + golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a ) diff --git a/go.sum b/go.sum index 2702148..0c3188b 100644 --- a/go.sum +++ b/go.sum @@ -1,4 +1,4 @@ github.com/seccomp/libseccomp-golang v0.9.1 h1:NJjM5DNFOs0s3kYE1WUOr6G8V97sdt46rlXTMfXGWBo= github.com/seccomp/libseccomp-golang v0.9.1/go.mod h1:GbW5+tmTXfcxTToHLXlScSlAvWlF4P2Ca7zGrPiEpWo= -golang.org/x/sys v0.0.0-20190804053845-51ab0e2deafa h1:KIDDMLT1O0Nr7TSxp8xM5tJcdn8tgyAONntO829og1M= -golang.org/x/sys v0.0.0-20190804053845-51ab0e2deafa/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a h1:aYOabOQFp6Vj6W1F80affTUvO9UxmJRx8K0gsfABByQ= +golang.org/x/sys v0.0.0-20190813064441-fde4db37ae7a/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=