Skip to content

Conversation

tianon
Copy link
Member

@tianon tianon commented Sep 8, 2025

Also, update to the latest version/builds.

See also:

cc @kolyshkin @cyphar @ricardobranco777

The comment from @tshah14 in #4836 (comment) about static vs dynamic made me realize we have a uclibc variant in DOI that is statically compiled and might get us a "free pass" in runc (so chasing the bugfix in BusyBox upstream can be fully independent), but unfortunately the architecture support on uclibc is a significantly smaller set (we lose ppc64le, for example, which is the one we're trying to fix 😂), so I figured maybe it's worth trying the musl variant to see if perhaps it has enough of a simpler libc implementation as to avoid the segfault?

Notably this loses mips64le and armv5, but I don't think either of those are actively tested in runc anyhow so maybe that's an acceptable compromise, assuming this works? 🙇

Also, update to the latest version/builds.

Signed-off-by: Tianon Gravi <[email protected]>
@tianon
Copy link
Member Author

tianon commented Sep 8, 2025

That's a lot more red CI than I expected 😬

FWIW, this does work for the unshare/vfork segfault problem, at least on the ppc64le machine I've got:

$ docker run -it --rm --pull=always --cap-add SYS_ADMIN --security-opt apparmor=unconfined busybox:glibc sh -c 'unshare -mrpf sh -c "ls && true" && true'
glibc: Pulling from library/busybox
Digest: sha256:a2c55ed708c564a69a695e0a3bb16a4c47d2bb268d2ebd06f0d77336801b80de
Status: Image is up to date for busybox:glibc
Segmentation fault (core dumped)

$ docker run -it --rm --pull=always --cap-add SYS_ADMIN --security-opt apparmor=unconfined busybox:musl sh -c 'unshare -mrpf sh -c "ls && true" && true'
musl: Pulling from library/busybox
f3f5d636887b: Pull complete 
Digest: sha256:254e6134b1bf813b34e920bc4235864a54079057d51ae6db9a4f2328f261c2ad
Status: Downloaded newer image for busybox:musl
bin   dev   etc   home  proc  root  sys   tmp   usr   var

(lots of extra bits in that command to make sure the shell doesn't short-circuit and we get nicer segfault output that otherwise gets eaten somewhere in the stack)

@kolyshkin
Copy link
Contributor

TestSeccompPermitWriteMultipleConditions fails because ls binary in container now uses writev(2) instead of write(2), so basically an issue with the test.

Can be fixed with something like this:

diff --git a/libcontainer/integration/seccomp_test.go b/libcontainer/integration/seccomp_test.go
index 4e8e0912..a32dd44a 100644
--- a/libcontainer/integration/seccomp_test.go
+++ b/libcontainer/integration/seccomp_test.go
@@ -335,10 +335,10 @@ func TestSeccompMultipleConditionSameArgDeniesStdout(t *testing.T) {
                },
        }
 
-       buffers := runContainerOk(t, config, "ls", "/")
-       // Verify that nothing was printed
-       if len(buffers.Stdout.String()) != 0 {
-               t.Fatalf("Something was written to stdout, write call succeeded!\n")
+       buffers, _, _ := runContainer(t, config, "echo", "hey")
+       // Verify that nothing was printed.
+       if out := buffers.Stdout.String(); out != "" {
+               t.Fatalf("want empty stdout, got: %q", out)
        }
 }
 

Apparently, ls was still exiting with exit code of 0 when failing to write to stdout, and echo now exits with 1. The new code is not ideal either as it ignores the exit code and so if a container failed to run the test will pass.

@rata
Copy link
Member

rata commented Sep 9, 2025

I'm unsure if I'd like to drop glibc from all busybox images. It's definitely very common to use.

Can we report the glibc issue upstream and either skip those tests or just use musl on that arch? What do you think?

@tianon
Copy link
Member Author

tianon commented Sep 11, 2025

The new code is not ideal either as it ignores the exit code and so if a container failed to run the test will pass.

We could fix that by checking both the exit code and the error, right?

func runContainer(t testing.TB, config *configs.Config, args ...string) (buffers *stdBuffers, exitCode int, err error) {

I'm not sold on switching to musl entirely either, but if we want to use it at all we need to fix this test to work on both, which is probably worthwhile either way IMO. 😅

Adjusting to being able to handle different libc implementations in the tests would allow for slightly better architecture coverage too.

@tianon tianon marked this pull request as draft September 11, 2025 20:23
@tianon
Copy link
Member Author

tianon commented Sep 11, 2025

exitCode -1, error exit status 1

seems like it's maybe unexpected that we're getting exitCode of -1 from runContainer when the err is literally that it exited with 1 but perhaps I don't understand runContainer 😅

Co-authored-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Tianon Gravi <[email protected]>
@tianon
Copy link
Member Author

tianon commented Sep 11, 2025

Oof, am I reading correctly that this is just copying in a likely non-static binary into the container and has been relying on glibc inside the container to work?

HELPER="key_label"
cp "${TESTBINDIR}/${HELPER}" rootfs/bin/
LABEL="system_u:system_r:container_t:s0:c4,c5"
update_config ' .process.selinuxLabel |= "'"$LABEL"'"
| .process.args = ["/bin/'"$HELPER"'"]'
runc run tst

@tianon
Copy link
Member Author

tianon commented Sep 11, 2025

Oh it's a Go binary, so maybe it could be compiled with CGO_ENABLED=0 or something to get a proper static binary?

@kolyshkin
Copy link
Contributor

@tianon it is a static binary, at least on my machine:

[kir@kir-tp1 runc]$ make key_label
mkdir tests/cmd/_bin
go build -trimpath "-buildmode=pie"  -tags "seccomp urfave_cli_no_docs" -ldflags "-X main.gitCommit=v1.3.0-rc.1-285-g01c93bc8  " -o tests/cmd/_bin ./tests/cmd/key_label
[kir@kir-tp1 runc]$ ldd tests/cmd/_bin/key_label 
	statically linked

The comment says it's written in Go for it to be static.

@tianon
Copy link
Member Author

tianon commented Sep 11, 2025

From the CI, which I don't understand otherwise: 😅

# (from function `run_check_label' in file tests/integration/selinux.bats, line 44,
#  in test file tests/integration/selinux.bats, line 91)
#   `run_check_label' failed
# runc spec (status=0)
#
# runc run tst (status=255)
# exec /bin/key_label: no such file or directory
# --- teardown ---
# <no matches>
not ok 253 runc exec (session keyring security label)
# (from function `exec_check_label' in file tests/integration/selinux.bats, line 63,
#  in test file tests/integration/selinux.bats, line 95)
#   `exec_check_label' failed
# runc spec (status=0)
#
# runc run -d --console-socket /tmp/bats-run-CHZTmV/runc.xGXVSO/tty/sock tst (status=0)
#
# runc exec tst /bin/key_label (status=255)
# exec /bin/key_label: no such file or directory
# --- teardown ---
# ----
# type=AVC msg=audit(09/11/2025 21:13:36.375:27368) : avc:  denied  { read write } for  pid=56875 comm=sh name=tty dev="tmpfs" ino=8 scontext=system_u:system_r:container_t:s0:c4,c5 tcontext=unconfined_u:object_r:container_runtime_tmpfs_t:s0 tclass=chr_file permissive=0
not ok 254 runc run (session keyring security label + userns)
# (from function `run_check_label' in file tests/integration/selinux.bats, line 44,
#  in test file tests/integration/selinux.bats, line 100)
#   `run_check_label' failed
# runc spec (status=0)
#
# skipping file /home: cannot remap user 65534:65534 -> -1:-1
# runc run tst (status=255)
# exec /bin/key_label: no such file or directory
# --- teardown ---
# ----
# type=AVC msg=audit(09/11/2025 21:13:36.375:27368) : avc:  denied  { read write } for  pid=56875 comm=sh name=tty dev="tmpfs" ino=8 scontext=system_u:system_r:container_t:s0:c4,c5 tcontext=unconfined_u:object_r:container_runtime_tmpfs_t:s0 tclass=chr_file permissive=0
not ok 255 runc exec (session keyring security label + userns)
# (from function `exec_check_label' in file tests/integration/selinux.bats, line 63,
#  in test file tests/integration/selinux.bats, line 105)
#   `exec_check_label' failed
# runc spec (status=0)
#
# skipping file /home: cannot remap user 65534:65534 -> -1:-1
# runc run -d --console-socket /tmp/bats-run-CHZTmV/runc.PxVABX/tty/sock tst (status=0)
#
# runc exec tst /bin/key_label (status=255)
# exec /bin/key_label: no such file or directory
# --- teardown ---
# <no matches>

@kolyshkin
Copy link
Contributor

exitCode -1, error exit status 1

seems like it's maybe unexpected that we're getting exitCode of -1 from runContainer when the err is literally that it exited with 1 but perhaps I don't understand runContainer 😅

From the runContainer code, exitCode == -1 means the process was either stopped, or received signal 1. Although I don't understand how that can happen together with the "exit status 1" error. 😩

@tianon
Copy link
Member Author

tianon commented Sep 12, 2025

My guess is a race on

return buffers, -1, err
(or the Wait one just below it) 👀

@tianon
Copy link
Member Author

tianon commented Sep 12, 2025

I see you've already got your own branch off this, but do feel free to push directly here if that's helpful to you - this is honestly more for your sake than mine 😅💖

@kolyshkin
Copy link
Contributor

I guess this can be closed as we figured out the bug is in busybox itself (it uses vfork in an unsupported way, see #4836)

@kolyshkin kolyshkin closed this Sep 18, 2025
@tianon tianon deleted the busybox-musl branch September 18, 2025 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants