From 3df6a02f5dd0ac67e828f1bef511a822d4195fc9 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Jul 2021 16:10:28 -0700 Subject: [PATCH 1/5] libct/nsenter: test: improve newPipe 1. Make sure we close all file descriptors at the end of the test. 2. Make sure we close child fds after the start. 3. Use newPipe for logs as well, for simplicity and uniformity. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsenter_test.go | 52 +++++++++++++--------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/libcontainer/nsenter/nsenter_test.go b/libcontainer/nsenter/nsenter_test.go index 9201f02641d..d0badcf977d 100644 --- a/libcontainer/nsenter/nsenter_test.go +++ b/libcontainer/nsenter/nsenter_test.go @@ -28,10 +28,7 @@ type logentry struct { func TestNsenterValidPaths(t *testing.T) { args := []string{"nsenter-exec"} - parent, child, err := newPipe() - if err != nil { - t.Fatalf("failed to create pipe %v", err) - } + parent, child := newPipe(t) namespaces := []string{ // join pid ns of the current process @@ -49,6 +46,7 @@ func TestNsenterValidPaths(t *testing.T) { if err := cmd.Start(); err != nil { t.Fatalf("nsenter failed to start %v", err) } + child.Close() // write cloneFlags r := nl.NewNetlinkRequest(int(libcontainer.InitMsg), 0) @@ -89,10 +87,7 @@ func TestNsenterValidPaths(t *testing.T) { func TestNsenterInvalidPaths(t *testing.T) { args := []string{"nsenter-exec"} - parent, child, err := newPipe() - if err != nil { - t.Fatalf("failed to create pipe %v", err) - } + parent, child := newPipe(t) namespaces := []string{ // join pid ns of the current process @@ -108,6 +103,8 @@ func TestNsenterInvalidPaths(t *testing.T) { if err := cmd.Start(); err != nil { t.Fatal(err) } + child.Close() + // write cloneFlags r := nl.NewNetlinkRequest(int(libcontainer.InitMsg), 0) r.AddData(&libcontainer.Int32msg{ @@ -130,10 +127,7 @@ func TestNsenterInvalidPaths(t *testing.T) { func TestNsenterIncorrectPathType(t *testing.T) { args := []string{"nsenter-exec"} - parent, child, err := newPipe() - if err != nil { - t.Fatalf("failed to create pipe %v", err) - } + parent, child := newPipe(t) namespaces := []string{ // join pid ns of the current process @@ -149,6 +143,8 @@ func TestNsenterIncorrectPathType(t *testing.T) { if err := cmd.Start(); err != nil { t.Fatal(err) } + child.Close() + // write cloneFlags r := nl.NewNetlinkRequest(int(libcontainer.InitMsg), 0) r.AddData(&libcontainer.Int32msg{ @@ -171,18 +167,8 @@ func TestNsenterIncorrectPathType(t *testing.T) { func TestNsenterChildLogging(t *testing.T) { args := []string{"nsenter-exec"} - parent, child, err := newPipe() - if err != nil { - t.Fatalf("failed to create exec pipe %v", err) - } - logread, logwrite, err := os.Pipe() - if err != nil { - t.Fatalf("failed to create log pipe %v", err) - } - defer func() { - _ = logwrite.Close() - _ = logread.Close() - }() + parent, child := newPipe(t) + logread, logwrite := newPipe(t) namespaces := []string{ // join pid ns of the current process @@ -200,6 +186,9 @@ func TestNsenterChildLogging(t *testing.T) { if err := cmd.Start(); err != nil { t.Fatalf("nsenter failed to start %v", err) } + child.Close() + logwrite.Close() + // write cloneFlags r := nl.NewNetlinkRequest(int(libcontainer.InitMsg), 0) r.AddData(&libcontainer.Int32msg{ @@ -219,7 +208,7 @@ func TestNsenterChildLogging(t *testing.T) { logsDecoder := json.NewDecoder(logread) var logentry *logentry - err = logsDecoder.Decode(&logentry) + err := logsDecoder.Decode(&logentry) if err != nil { t.Fatalf("child log: %v", err) } @@ -238,12 +227,19 @@ func init() { } } -func newPipe() (parent *os.File, child *os.File, err error) { +func newPipe(t *testing.T) (parent *os.File, child *os.File) { + t.Helper() fds, err := unix.Socketpair(unix.AF_LOCAL, unix.SOCK_STREAM|unix.SOCK_CLOEXEC, 0) if err != nil { - return nil, nil, err + t.Fatal("socketpair failed:", err) } - return os.NewFile(uintptr(fds[1]), "parent"), os.NewFile(uintptr(fds[0]), "child"), nil + parent = os.NewFile(uintptr(fds[1]), "parent") + child = os.NewFile(uintptr(fds[0]), "child") + t.Cleanup(func() { + parent.Close() + child.Close() + }) + return } // initWaiter reads back the initial \0 from runc init From feb1fe11a188a9d7474cacf10997b27ef472a0e0 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Jul 2021 16:33:29 -0700 Subject: [PATCH 2/5] libct/nsenter: test: fix TestNsenterValidPaths The test was not working since at least commit 64bb59f5920b1 renamed pid to stage2_pid (or maybe even earlier), so the pid was never received (i.e. pid.Pid was 0). The problem was not caught because os.FindProcess never return an error on Unix. Factor out and fix pid decode function: - use DisallowUnknownInput to get error if JSON will be changed; - check pids to make sure they are valid - and use unix.Wait4 to reap zombies. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsenter_test.go | 43 +++++++++++++++------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/libcontainer/nsenter/nsenter_test.go b/libcontainer/nsenter/nsenter_test.go index d0badcf977d..2e05657fcd6 100644 --- a/libcontainer/nsenter/nsenter_test.go +++ b/libcontainer/nsenter/nsenter_test.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "io" - "io/ioutil" "os" "os/exec" "strings" @@ -17,10 +16,6 @@ import ( "golang.org/x/sys/unix" ) -type pid struct { - Pid int `json:"Pid"` -} - type logentry struct { Msg string `json:"msg"` Level string `json:"level"` @@ -64,25 +59,11 @@ func TestNsenterValidPaths(t *testing.T) { initWaiter(t, parent) - decoder := json.NewDecoder(parent) - var pid *pid - if err := cmd.Wait(); err != nil { t.Fatalf("nsenter exits with a non-zero exit status") } - if err := decoder.Decode(&pid); err != nil { - dir, _ := ioutil.ReadDir(fmt.Sprintf("/proc/%d/ns", os.Getpid())) - for _, d := range dir { - t.Log(d.Name()) - } - t.Fatalf("%v", err) - } - p, err := os.FindProcess(pid.Pid) - if err != nil { - t.Fatalf("%v", err) - } - _, _ = p.Wait() + reapChildren(t, parent) } func TestNsenterInvalidPaths(t *testing.T) { @@ -257,3 +238,25 @@ func initWaiter(t *testing.T, r io.Reader) { } t.Fatalf("waiting for init preliminary setup: %v", err) } + +func reapChildren(t *testing.T, parent *os.File) { + t.Helper() + decoder := json.NewDecoder(parent) + decoder.DisallowUnknownFields() + var pid struct { + Pid2 int `json:"stage2_pid"` + Pid1 int `json:"stage1_pid"` + } + if err := decoder.Decode(&pid); err != nil { + t.Fatal(err) + } + + // Reap children. + _, _ = unix.Wait4(pid.Pid1, nil, 0, nil) + _, _ = unix.Wait4(pid.Pid2, nil, 0, nil) + + // Sanity check. + if pid.Pid1 == 0 || pid.Pid2 == 0 { + t.Fatal("got pids:", pid) + } +} From 2c46455c3f85936c9fdaa1576df10e191f4b8203 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Jul 2021 16:41:41 -0700 Subject: [PATCH 3/5] libct/nsenter: test: improve TestNsenterChildLogging Instead of reading a single message, do read all the logs from the init, and use DisallowUnknownFields for stricter checking. While at it, use reapChildren to reap zombies (and add an extra check). Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsenter_test.go | 41 +++++++++++++++++----------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/libcontainer/nsenter/nsenter_test.go b/libcontainer/nsenter/nsenter_test.go index 2e05657fcd6..409c8ccf25b 100644 --- a/libcontainer/nsenter/nsenter_test.go +++ b/libcontainer/nsenter/nsenter_test.go @@ -16,11 +16,6 @@ import ( "golang.org/x/sys/unix" ) -type logentry struct { - Msg string `json:"msg"` - Level string `json:"level"` -} - func TestNsenterValidPaths(t *testing.T) { args := []string{"nsenter-exec"} parent, child := newPipe(t) @@ -186,20 +181,12 @@ func TestNsenterChildLogging(t *testing.T) { initWaiter(t, parent) - logsDecoder := json.NewDecoder(logread) - var logentry *logentry - - err := logsDecoder.Decode(&logentry) - if err != nil { - t.Fatalf("child log: %v", err) - } - if logentry.Level == "" || logentry.Msg == "" { - t.Fatalf("child log: empty log fields: level=\"%s\" msg=\"%s\"", logentry.Level, logentry.Msg) - } - + getLogs(t, logread) if err := cmd.Wait(); err != nil { t.Fatalf("nsenter exits with a non-zero exit status") } + + reapChildren(t, parent) } func init() { @@ -260,3 +247,25 @@ func reapChildren(t *testing.T, parent *os.File) { t.Fatal("got pids:", pid) } } + +func getLogs(t *testing.T, logread *os.File) { + logsDecoder := json.NewDecoder(logread) + logsDecoder.DisallowUnknownFields() + var logentry struct { + Level string `json:"level"` + Msg string `json:"msg"` + } + + for { + if err := logsDecoder.Decode(&logentry); err != nil { + if errors.Is(err, io.EOF) { + return + } + t.Fatal("init log decoding error:", err) + } + t.Logf("logentry: %+v", logentry) + if logentry.Level == "" || logentry.Msg == "" { + t.Fatalf("init log: empty log entry: %+v", logentry) + } + } +} From 78b271555e9805ceff19717ad1d53a2832002770 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 27 Jul 2021 17:46:35 -0700 Subject: [PATCH 4/5] libct/nsenter: test: rm misleading comments Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsenter_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/libcontainer/nsenter/nsenter_test.go b/libcontainer/nsenter/nsenter_test.go index 409c8ccf25b..a7c346e5899 100644 --- a/libcontainer/nsenter/nsenter_test.go +++ b/libcontainer/nsenter/nsenter_test.go @@ -66,7 +66,6 @@ func TestNsenterInvalidPaths(t *testing.T) { parent, child := newPipe(t) namespaces := []string{ - // join pid ns of the current process fmt.Sprintf("pid:/proc/%d/ns/pid", -1), } cmd := &exec.Cmd{ @@ -106,7 +105,6 @@ func TestNsenterIncorrectPathType(t *testing.T) { parent, child := newPipe(t) namespaces := []string{ - // join pid ns of the current process fmt.Sprintf("net:/proc/%d/ns/pid", os.Getpid()), } cmd := &exec.Cmd{ From 33dcb994f471db95588e3fee5d14bd43d2131a4b Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 2 Sep 2021 10:42:32 -0700 Subject: [PATCH 5/5] libct/nsenter/nsenter_test.go: logging nits - add missing colons before error message; - unify error messages after cmd.Start and cmd.Wait, so that they show context and the error itself. Signed-off-by: Kir Kolyshkin --- libcontainer/nsenter/nsenter_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/libcontainer/nsenter/nsenter_test.go b/libcontainer/nsenter/nsenter_test.go index a7c346e5899..0cbf0aae61b 100644 --- a/libcontainer/nsenter/nsenter_test.go +++ b/libcontainer/nsenter/nsenter_test.go @@ -34,7 +34,7 @@ func TestNsenterValidPaths(t *testing.T) { } if err := cmd.Start(); err != nil { - t.Fatalf("nsenter failed to start %v", err) + t.Fatalf("nsenter failed to start: %v", err) } child.Close() @@ -55,7 +55,7 @@ func TestNsenterValidPaths(t *testing.T) { initWaiter(t, parent) if err := cmd.Wait(); err != nil { - t.Fatalf("nsenter exits with a non-zero exit status") + t.Fatalf("nsenter error: %v", err) } reapChildren(t, parent) @@ -76,7 +76,7 @@ func TestNsenterInvalidPaths(t *testing.T) { } if err := cmd.Start(); err != nil { - t.Fatal(err) + t.Fatalf("nsenter failed to start: %v", err) } child.Close() @@ -115,7 +115,7 @@ func TestNsenterIncorrectPathType(t *testing.T) { } if err := cmd.Start(); err != nil { - t.Fatal(err) + t.Fatalf("nsenter failed to start: %v", err) } child.Close() @@ -135,7 +135,7 @@ func TestNsenterIncorrectPathType(t *testing.T) { initWaiter(t, parent) if err := cmd.Wait(); err == nil { - t.Fatalf("nsenter exits with a zero exit status") + t.Fatalf("nsenter error: %v", err) } } @@ -158,7 +158,7 @@ func TestNsenterChildLogging(t *testing.T) { } if err := cmd.Start(); err != nil { - t.Fatalf("nsenter failed to start %v", err) + t.Fatalf("nsenter failed to start: %v", err) } child.Close() logwrite.Close() @@ -181,7 +181,7 @@ func TestNsenterChildLogging(t *testing.T) { getLogs(t, logread) if err := cmd.Wait(); err != nil { - t.Fatalf("nsenter exits with a non-zero exit status") + t.Fatalf("nsenter error: %v", err) } reapChildren(t, parent)