From 6bb653a6e881594eebd9532f6ed95339d3856ac6 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Thu, 25 Feb 2016 14:30:49 -0800 Subject: [PATCH] Return proper exit code for exec errors Exec erros from the exec() syscall in the container's init should be treated as if the container ran but couldn't execute the process for the user instead of returning a libcontainer error as if it was an issue in the library. Before specifying different commands like `/etc`, `asldfkjasdlfj`, or `/alsdjfkasdlfj` would always return 1 on the command line with a libcontainer specific error message. Now they return the correct message and exit status defined for unix processes. Example: ```bash root@deathstar:/containers/redis# runc start test exec: "/asdlfkjasldkfj": file does not exist root@deathstar:/containers/redis# echo $? 127 root@deathstar:/containers/redis# runc start test exec: "asdlfkjasldkfj": executable file not found in $PATH root@deathstar:/containers/redis# echo $? 127 root@deathstar:/containers/redis# runc start test exec: "/etc": permission denied root@deathstar:/containers/redis# echo $? 126 ``` Signed-off-by: Michael Crosby --- libcontainer/factory_linux.go | 45 ++++++++++++++++--------- libcontainer/integration/execin_test.go | 38 +++++++++++---------- libcontainer/integration/init_test.go | 20 +++++++++++ start.go | 2 +- utils.go | 18 ++++++++++ 5 files changed, 89 insertions(+), 34 deletions(-) diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index 14e4f33a..eb327be7 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -227,32 +227,40 @@ func (l *LinuxFactory) StartInitialization() (err error) { pipe = os.NewFile(uintptr(pipefd), "pipe") it = initType(os.Getenv("_LIBCONTAINER_INITTYPE")) ) + defer pipe.Close() // clear the current process's environment to clean any libcontainer // specific env vars. os.Clearenv() - var i initer - defer func() { - // We have an error during the initialization of the container's init, - // send it back to the parent process in the form of an initError. - // If container's init successed, syscall.Exec will not return, hence - // this defer function will never be called. + i, err := newContainerInit(it, pipe) + if err != nil { + l.sendError(nil, pipe, err) + return err + } + if err := i.Init(); err != nil { + if !isExecError(err) { + l.sendError(i, pipe, err) + } + return err + } + return nil +} + +func (l *LinuxFactory) sendError(i initer, pipe *os.File, err error) { + // We have an error during the initialization of the container's init, + // send it back to the parent process in the form of an initError. + // If container's init successed, syscall.Exec will not return, hence + // this defer function will never be called. + if i != nil { if _, ok := i.(*linuxStandardInit); ok { // Synchronisation only necessary for standard init. if err := utils.WriteJSON(pipe, syncT{procError}); err != nil { panic(err) } } - if err := utils.WriteJSON(pipe, newSystemError(err)); err != nil { - panic(err) - } - // ensure that this pipe is always closed - pipe.Close() - }() - i, err = newContainerInit(it, pipe) - if err != nil { - return err } - return i.Init() + if err := utils.WriteJSON(pipe, newSystemError(err)); err != nil { + panic(err) + } } func (l *LinuxFactory) loadState(root string) (*State, error) { @@ -280,3 +288,8 @@ func (l *LinuxFactory) validateID(id string) error { } return nil } + +func isExecError(err error) bool { + _, ok := err.(*exec.Error) + return ok +} diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index a80c9581..8327362f 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -4,8 +4,10 @@ import ( "bytes" "io" "os" + "os/exec" "strconv" "strings" + "syscall" "testing" "time" @@ -138,25 +140,27 @@ func TestExecInError(t *testing.T) { }() ok(t, err) - for i := 0; i < 42; i++ { - var out bytes.Buffer - unexistent := &libcontainer.Process{ - Cwd: "/", - Args: []string{"unexistent"}, - Env: standardEnvironment, - Stdout: &out, - } - err = container.Start(unexistent) - if err == nil { - t.Fatal("Should be an error") - } - if !strings.Contains(err.Error(), "executable file not found") { - t.Fatalf("Should be error about not found executable, got %s", err) - } - if !bytes.Contains(out.Bytes(), []byte("executable file not found")) { - t.Fatalf("executable file not found error not delivered to stdio:\n%s", out.String()) + var out bytes.Buffer + unexistent := &libcontainer.Process{ + Cwd: "/", + Args: []string{"unexistent"}, + Env: standardEnvironment, + Stdout: &out, + Stderr: &out, + } + err = container.Start(unexistent) + if err != nil { + t.Fatal(err) + } + ws, err := unexistent.Wait() + if err != nil { + if _, ok := err.(*exec.ExitError); !ok { + t.Fatal(err) } } + if s := ws.Sys().(syscall.WaitStatus).ExitStatus(); s != 127 { + t.Fatalf("expected wait status of 127 but received %d", s) + } } func TestExecInTTY(t *testing.T) { diff --git a/libcontainer/integration/init_test.go b/libcontainer/integration/init_test.go index eaa6caf6..7da6096f 100644 --- a/libcontainer/integration/init_test.go +++ b/libcontainer/integration/init_test.go @@ -1,8 +1,11 @@ package integration import ( + "fmt" "os" + "os/exec" "runtime" + "strconv" "testing" "github.com/Sirupsen/logrus" @@ -24,8 +27,25 @@ func init() { logrus.Fatalf("unable to initialize for container: %s", err) } if err := factory.StartInitialization(); err != nil { + // return proper unix error codes + if exerr, ok := err.(*exec.Error); ok { + switch exerr.Err { + case os.ErrPermission: + fmt.Fprintln(os.Stderr, err) + os.Exit(126) + case exec.ErrNotFound: + fmt.Fprintln(os.Stderr, err) + os.Exit(127) + default: + if os.IsNotExist(exerr.Err) { + fmt.Fprintf(os.Stderr, "exec: %s: %v\n", strconv.Quote(exerr.Name), os.ErrNotExist) + os.Exit(127) + } + } + } logrus.Fatal(err) } + panic("init: init failed to start contianer") } var ( diff --git a/start.go b/start.go index 8b7b5446..4df3e44f 100644 --- a/start.go +++ b/start.go @@ -84,7 +84,7 @@ func init() { if err := factory.StartInitialization(); err != nil { fatal(err) } - panic("--this line should have never been executed, congratulations--") + panic("libcontainer: container init failed to exec") } } diff --git a/utils.go b/utils.go index 96212e36..2d098c76 100644 --- a/utils.go +++ b/utils.go @@ -6,7 +6,9 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" + "strconv" "syscall" "github.com/Sirupsen/logrus" @@ -161,6 +163,22 @@ func getContainer(context *cli.Context) (libcontainer.Container, error) { // fatal prints the error's details if it is a libcontainer specific error type // then exits the program with an exit status of 1. func fatal(err error) { + // return proper unix error codes + if exerr, ok := err.(*exec.Error); ok { + switch exerr.Err { + case os.ErrPermission: + fmt.Fprintln(os.Stderr, err) + os.Exit(126) + case exec.ErrNotFound: + fmt.Fprintln(os.Stderr, err) + os.Exit(127) + default: + if os.IsNotExist(exerr.Err) { + fmt.Fprintf(os.Stderr, "exec: %s: %v\n", strconv.Quote(exerr.Name), os.ErrNotExist) + os.Exit(127) + } + } + } if lerr, ok := err.(libcontainer.Error); ok { lerr.Detail(os.Stderr) os.Exit(1)