From 7fd908650b9bae041770034437f6577390e49f48 Mon Sep 17 00:00:00 2001 From: Ace-Tang Date: Thu, 23 Aug 2018 18:49:39 +0800 Subject: [PATCH] bugfix: avoid data race in set exec exit status there are two goroutines response for changed exec config information, if exec process got error, data race appears, exec exit hook runs in a goroutine, it modify exec exit status, StartExec also modify exec exit status in another goroutine since error happend. Fix it by only one place can modify exec exit status. Signed-off-by: Ace-Tang --- ctrd/container.go | 15 +++++++-------- daemon/mgr/container_exec.go | 1 - test/cli_exec_test.go | 13 ++++++++++++- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/ctrd/container.go b/ctrd/container.go index 87865befd..d79ea9dac 100644 --- a/ctrd/container.go +++ b/ctrd/container.go @@ -122,14 +122,13 @@ func (c *Client) execContainer(ctx context.Context, process *Process) error { exitTime: status.ExitTime(), } - if err := <-fail; err != nil { - msg.err = err - } - - for _, hook := range c.hooks { - if err := hook(process.ExecID, msg); err != nil { - logrus.Errorf("failed to execute the exec exit hooks: %v", err) - break + // run hook if not got fail here + if err := <-fail; err == nil { + for _, hook := range c.hooks { + if err := hook(process.ExecID, msg); err != nil { + logrus.Errorf("failed to execute the exec exit hooks: %v", err) + break + } } } diff --git a/daemon/mgr/container_exec.go b/daemon/mgr/container_exec.go index d4d2febc3..af2be0615 100644 --- a/daemon/mgr/container_exec.go +++ b/daemon/mgr/container_exec.go @@ -77,7 +77,6 @@ func (mgr *ContainerManager) StartExec(ctx context.Context, execid string, attac exitCode := 126 execConfig.ExitCode = int64(exitCode) } - mgr.ExecProcesses.Put(execid, execConfig) }() if attach != nil { diff --git a/test/cli_exec_test.go b/test/cli_exec_test.go index 019801cfe..afdd59338 100644 --- a/test/cli_exec_test.go +++ b/test/cli_exec_test.go @@ -144,12 +144,23 @@ func (suite *PouchExecSuite) TestExecExitCode(c *check.C) { command.PouchRun("exec", name, "sh", "-c", "exit 0").Assert(c, icmd.Success) } -// TestExecFail test exec fail should not hang. +// TestExecFail test exec fail should not hang, and test failed exec exit code should not be zero. func (suite *PouchExecSuite) TestExecFail(c *check.C) { name := "TestExecFail" res := command.PouchRun("run", "-d", "--name", name, "-u", name, busyboxImage, "top") defer DelContainerForceMultyTime(c, name) c.Assert(res.Stderr(), check.NotNil) + + // test a 'executable file not found' fail should get exit code 126. + name = "TestExecFailCode" + command.PouchRun("run", "-d", "--name", name, busyboxImage, "top").Assert(c, icmd.Success) + command.PouchRun("exec", name, "shouldnotexit").Assert(c, icmd.Expected{ExitCode: 126}) + + // test a 'ls /nosuchfile' fail should get exit code not equal to 0. + res = command.PouchRun("exec", name, "ls", "/nosuchfile") + if res.ExitCode == 0 { + c.Fatalf("failed exec process exit code should not be 0") + } } // TestExecUser test exec with user.