Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

never send procError after the socket closed #4173

Merged
merged 2 commits into from
Jan 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion libcontainer/init_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 9 additions & 1 deletion libcontainer/sync_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"io"
"os"
"sync/atomic"

"golang.org/x/sys/unix"
)
Expand All @@ -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 {
Expand All @@ -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)
}
Expand Down
18 changes: 18 additions & 0 deletions tests/integration/exec.bats
Original file line number Diff line number Diff line change
Expand Up @@ -322,3 +322,21 @@ function check_exec_debug() {
runc exec --cgroup second test_busybox grep -w second /proc/self/cgroup
[ "$status" -eq 0 ]
}

lifubang marked this conversation as resolved.
Show resolved Hide resolved
@test "RUNC_DMZ=legacy runc exec [execve error]" {
cat <<EOF >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"* ]]
}
16 changes: 16 additions & 0 deletions tests/integration/run.bats
Original file line number Diff line number Diff line change
Expand Up @@ -247,3 +247,19 @@ EOF
[ "$status" -ne 0 ]
[[ "$output" = *"exec /run.sh: "* ]]
}

lifubang marked this conversation as resolved.
Show resolved Hide resolved
@test "RUNC_DMZ=legacy runc run [execve error]" {
cat <<EOF >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" ]]
}
Loading