From 35aa63ea874f249d9c1b84250aa3f5aef960b3e7 Mon Sep 17 00:00:00 2001 From: lifubang Date: Sun, 14 Jan 2024 12:54:32 +0800 Subject: [PATCH 1/2] never send procError after the socket closed Signed-off-by: lifubang --- libcontainer/init_linux.go | 6 +++++- libcontainer/sync_unix.go | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 0117ace5990..85be7cc4122 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -107,7 +107,11 @@ func startInitialization() (retErr error) { defer func() { // If this defer is ever called, this means initialization has failed. - // Send the error back to the parent process in the form of an initError. + // Send the error back to the parent process in the form of an initError + // if the sync socket has not been closed. + if syncPipe.isClosed() { + return + } ierr := initError{Message: retErr.Error()} if err := writeSyncArg(syncPipe, procError, ierr); err != nil { fmt.Fprintln(os.Stderr, err) diff --git a/libcontainer/sync_unix.go b/libcontainer/sync_unix.go index f94486cb300..c5d8f55ec95 100644 --- a/libcontainer/sync_unix.go +++ b/libcontainer/sync_unix.go @@ -4,6 +4,7 @@ import ( "fmt" "io" "os" + "sync/atomic" "golang.org/x/sys/unix" ) @@ -14,7 +15,8 @@ import ( // which ends up making things like json.Decoder hang forever if the packet is // bigger than the internal read buffer. type syncSocket struct { - f *os.File + f *os.File + closed atomic.Bool } func newSyncSocket(f *os.File) *syncSocket { @@ -26,9 +28,15 @@ func (s *syncSocket) File() *os.File { } func (s *syncSocket) Close() error { + // Even with errors from Close(), we have to assume the pipe was closed. + s.closed.Store(true) return s.f.Close() } +func (s *syncSocket) isClosed() bool { + return s.closed.Load() +} + func (s *syncSocket) WritePacket(b []byte) (int, error) { return s.f.Write(b) } From 0bc4732c07bae471cb5e7bf17b2ef9cb2426e13e Mon Sep 17 00:00:00 2001 From: lfbzhm Date: Thu, 18 Jan 2024 15:31:57 +0000 Subject: [PATCH 2/2] test for execve error without runc-dmz Signed-off-by: lfbzhm --- tests/integration/exec.bats | 18 ++++++++++++++++++ tests/integration/run.bats | 16 ++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/tests/integration/exec.bats b/tests/integration/exec.bats index 069214cc956..78f922d04b4 100644 --- a/tests/integration/exec.bats +++ b/tests/integration/exec.bats @@ -322,3 +322,21 @@ function check_exec_debug() { runc exec --cgroup second test_busybox grep -w second /proc/self/cgroup [ "$status" -eq 0 ] } + +@test "RUNC_DMZ=legacy runc exec [execve error]" { + cat <rootfs/run.sh +#!/mmnnttbb foo bar +sh +EOF + chmod +x rootfs/run.sh + RUNC_DMZ=legacy runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox + RUNC_DMZ=legacy runc exec -t test_busybox /run.sh + [ "$status" -ne 0 ] + + # After the sync socket closed, we should not send error to parent + # process, or else we will get a unnecessary error log(#4171). + # Although we never close the sync socket when doing exec, + # but we need to keep this test to ensure this behavior is always right. + [ ${#lines[@]} -eq 1 ] + [[ ${lines[0]} = *"exec failed: unable to start container process: exec /run.sh: no such file or directory"* ]] +} diff --git a/tests/integration/run.bats b/tests/integration/run.bats index bf23d22a103..82c58027780 100644 --- a/tests/integration/run.bats +++ b/tests/integration/run.bats @@ -247,3 +247,19 @@ EOF [ "$status" -ne 0 ] [[ "$output" = *"exec /run.sh: "* ]] } + +@test "RUNC_DMZ=legacy runc run [execve error]" { + cat <rootfs/run.sh +#!/mmnnttbb foo bar +sh +EOF + chmod +x rootfs/run.sh + update_config '.process.args = [ "/run.sh" ]' + RUNC_DMZ=legacy runc run test_hello + [ "$status" -ne 0 ] + + # After the sync socket closed, we should not send error to parent + # process, or else we will get a unnecessary error log(#4171). + [ ${#lines[@]} -eq 1 ] + [[ ${lines[0]} = "exec /run.sh: no such file or directory" ]] +}