When runc run or runc exec (without -d) is run as a background job of
a shell with job control, reading from (or writing to) the terminal
results in the kernel sending SIGTTIN (or SIGTTOU) to runc. The default
action for these signals is to stop the process, which is the expected
job control behavior.
Yet, runc catches all signals in order to forward them to the container.
If runc catches SIGTTIN, it forwards it to the container process, and
then the interrupted read is retried, resulting in another SIGTTIN, and
so on, so runc ends up in a busy loop.
Since commit d92dd226 the signal handler is set up in a separate
goroutine, so this only happens occasionally (when the signal handler
is set up before runc starts reading stdin); before that, it happened
every time.
SIGTTIN and SIGTTOU are job control signals of the host terminal, and
there is no point in forwarding those to the container. Exclude them
from the list of signals to catch, so the default action is used.
Note that signal.Reset can not be used to restore the default action,
since the Go runtime keeps its own handler installed for these signals
(which ignores them, resulting in the same busy loop).
Add an integration test.
Fixes#1430.
Assisted-by: Claude Opus 5.5
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
SIGURG is used by the Go runtime for asynchronous preemption, so runc
receives it from time to time. Since commit 0b74e49d, it is ignored in
the signal forwarding loop.
Instead of catching all signals and ignoring SIGURG later, use an
explicit list of signals to catch, which excludes SIGURG. The Go runtime
still handles it for preemption, but it no longer ends up in our signal
channel.
While at it, also exclude SIGCHLD from the list, since it is already
set up earlier.
Assisted-by: Claude Opus 5.5
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The previous wording could be misinterpreted as if
at least one branch would still support cgroup v1
even after May 2029.
See containerd/containerd PR 14505
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
SCMP_ACT_KILL kills a single thread, so "runc init" is left as a zombie
thread group leader surrounded by live threads that keep its descriptors
open. Check that runc reports that instead of hanging.
The container gets its own stdio rather than the one bats captures: were
this to regress, the leftover threads would keep bats' end open, and the
suite would hang instead of reporting a failure.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Assisted-by: Claude Opus 5
Seccomp's SCMP_ACT_KILL is SECCOMP_RET_KILL_THREAD: it kills the thread
that performed the syscall, not the whole thread group. As "runc init"
locks its main goroutine to the thread group leader, killing that thread
leaves a zombie leader whose remaining Go runtime threads are alive,
keeping the shared descriptor table -- and thus the child ends of the
sync socket and of the exec fifo -- open. The parent therefore never sees
an EOF, and since the thread group is not dead, neither wait(2) nor a
pidfd report anything either, so the parent waits forever.
Add waitForInitWrite, which polls the descriptor with a timeout and, when
nothing arrives, consults /proc/[pid]/stat -- the only thing that reports
a zombie thread group leader. Use it both for reading the sync socket and
for waiting on the exec fifo; the latter replaces the pidfd fast path,
which can not detect this case at all.
Kill and reap the leftover threads once a dead init is detected while
waiting for the exec fifo. They keep the container's stdio descriptors
open, so whoever inherited those would wait for an EOF just as long, and
they keep its cgroup populated, so destroying the container would fail.
Make nonChildProcess.terminate send SIGKILL rather than fail, so that
killDeadInit can use terminate for any init, be it our child ("runc
run") or not ("runc start"). In the latter case, init is only killed,
as reaping it is up to its parent.
Finally, do not wait(2) for the child in the setnsProcess.start error
path: a half-dead child is never reaped, and the deferred error handler
terminates and reaps it anyway.
Fixes#5087
Assisted-by: Claude Opus 5
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The runtime spec says about both defaultErrnoRet and errnoRet:
> When the action doesn't support an errno, the runtime MUST print and
> error and fail.
Yet runc silently ignores errnoRet set for any action other than
SCMP_ACT_ERRNO and SCMP_ACT_TRACE.
Add a check to the config validator, so the error is reported early
(before the container init is started) and clearly (including the
syscall name in case of errnoRet).
Since the validator checks all syscall rules, this also covers ones
which are later skipped as redundant (i.e. having the same action as
the default).
Add unit and integration tests, and a CHANGELOG entry.
Reported in issue 5550.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a new --cgroup-manager global option, accepting either "cgroupfs"
(the default) or "systemd". It is compatible with the option of the
same name crun has (except crun also has "disabled").
This helps tools which run either runc or crun, for example, crun's test
suite, which can now be run against runc.
Deprecate --systemd-cgroup in favor of --cgroup-manager=systemd. It is
still supported (and there are no plans to remove it), but conflicts with
--cgroup-manager set to anything other than systemd.
Switch the integration tests to use the new option, and add a few tests
for the new and the deprecated option.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
setReadonly remounts / with MS_BIND|MS_REMOUNT|MS_RDONLY. A bind
remount sets the per-mount flags to exactly the ones given, so when
not in a user namespace it silently clears nosuid, nodev, noexec and
nosymfollow which may be set on the rootfs mount.
This does not happen in a user namespace, where these flags are locked,
so the remount fails, and the fallback keeps them.
The fix is to always use the existing flags, not only on fallback.
In other words, the fallback is no longer needed.
While at it
- use statfsToMountFlags to convert, since ST_* and MS_*
values may differ (e.g. ST_NOSYMFOLLOW != MS_NOSYMFOLLOW);
- do not take other flags (e.g *ATIME*) as the kernel preserves
those on remount.
Add an integration test.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fix new errcheck warnings about unchecked errors received from channels:
libcontainer/configs/config.go:632:3: Error return value is not checked (errcheck)
libcontainer/logs/logs_linux_test.go:62:7: Error return value is not checked (errcheck)
notify_socket_test.go:73:7: Error return value is not checked (errcheck)
In the hook timeout case, the error from Wait is expected after Kill,
so add a nolint annotation (using "_ =" upsets staticcheck's S1005).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When the container does not have its own mount namespace, prepareRoot
is run in the host mount namespace. Since commit 91ca3314 ("chroot when
no mount namespaces is provided"), this results in host's / being
recursively made a slave (which, in the host mount namespace, means
private), and the parent mount of the container rootfs being made
private (or slave).
In the host mount namespace, do not change the propagation of the
existing mounts. Instead, make the new rootfs mount a slave, so that
container mounts do not propagate to the host and other mount
namespaces (otherwise, if the rootfs parent mount is shared, every
container mount appears twice in the host mount namespace, and is
propagated to all its slaves).
Add an integration test.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
ExecutableMemfd has a single caller (exeseal.Memfd), which always passes
MFD_ALLOW_SEALING|MFD_CLOEXEC, and it has been this way ever since the
function was added. Move it into exeseal.Memfd, hardcoding the flags.
This also makes sure the descriptor is always close-on-exec, which is in
line with the Go convention, and an executable memfd is never something
we want to leak across an execve.
Suggested-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
CreateInRoot has a single caller (createMountpoint), which always passes
the same O_CREAT|O_EXCL|O_NOFOLLOW flags. Hardcode those and drop the
argument, adding O_CLOEXEC while at it.
Previously, O_CLOEXEC was not set, which goes against the Go convention
(os package functions always set it, and descriptors that need to be
passed to a child are explicitly duped). The resulting handle has no
business surviving an execve.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
File descriptors created via the os package are close-on-exec by
default, but the raw syscall wrappers do exactly what they are told,
and a few call sites forgot to ask for it:
* registerMemoryEventV2 creates an inotify descriptor which is kept
open for the whole lifetime of the container by the OOM notifier
goroutine, so it would leak into every "runc exec" process.
* pivotRoot opens the old root, which is only needed until the
pivot_root(2) dance is over.
* readArgString in the seccomp agent opens /proc/PID/mem.
None of these descriptors are meant to be inherited, so set O_CLOEXEC
(IN_CLOEXEC for inotify).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These unit tests are built on two mocks, mockCgroupManager and
mockProcess, and the mocks cost more than the tests are worth.
mockCgroupManager implements the entire cgroups.Manager interface, so
oc/cgroups can not add a method to it without breaking the runc build
(see opencontainers/cgroups#61). mockProcess is in the same position with
respect to our own parentProcess interface: every method added there has
to be added to the mock, which never does anything meaningful with it.
The tests themselves do not pay for that:
- TestGetContainerPids merely asserted that Container.Processes returns
what the mock cgroup manager was configured to return.
- TestGetContainerState reimplemented the namespace type to /proc file
name mapping that it was meant to verify, so all it could detect was
drift between the two copies.
- TestGetContainerStateAfterUpdate round-tripped the container state
through a real state directory, which is genuinely useful, but with
the cgroup manager mocked out it could not tell whether Set did
anything beyond rewriting state.json.
Package coverage goes from 7.4% to 5.8%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For legacy reasons (see commit 087caf69), runc accepts a mount
destination prefixed with the container rootfs path, and strips the
prefix. This is not described by the runtime spec, which says the
destination is a path inside the container, and is not supported by
other runtimes (e.g. crun). It is also ambiguous, as such a path may
legitimately exist inside the container.
Remove the prefix stripping, so such a destination is used as is,
i.e. as a path inside the container.
Most of the stripping was done by hallucinateUnsafePath, used by
pathrs.MkdirAllInRoot and friends, so device node paths are no
longer stripped either.
Add an integration test.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Make the review-policy workflow reusable (via workflow_call), so other
opencontainers repositories with the same merge rules (such as
opencontainers/cgroups, see opencontainers/cgroups#91) can call it
instead of carrying a copy.
The calling repository gets the same logic, which is fine, since:
- CHANGELOG.md and vendor/ exceptions are no-ops where there's no
such file/directory;
- the merge_group trigger is the caller's, and it's harmless when unused.
The only difference was CODEOWNERS, which cgroups treats as a governance
file. Add it here, too, as it does define who reviews what.
Document how to use it from another repository.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The libseccomp and libpathrs libraries built by Dockerfile are installed
into $DYLIB_DIR/lib, which is only made known to the dynamic linker via
LD_LIBRARY_PATH. Yet runc init is executed with a clean environment, so
it can not find libpathrs.so.0 and dies, resulting in
unable to start container process: can't copy bootstrap data
to pipe: write init-p: broken pipe
As a result, "make integration" and some of "make unittest" fail.
Use ldconfig instead of LD_LIBRARY_PATH.
This went unnoticed because CI only builds the image, and never runs
tests in it. Add a quick smoke test (run.bats) to the release job, which
is the only one that builds the image.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Go's encoding/json (v1) matches object keys case-insensitively, and
silently ignores unknown keys. As a result, runc happily accepts a
config.json with e.g. "CPU" instead of "cpu", or "pagesize" instead of
"pageSize", while other runtimes (which run runc integration tests,
e.g. crun) may not. In addition, runc will likely switch to
encoding/json/v2 at some point, which matches keys case-sensitively,
so such keys will be silently ignored by runc, too.
Add a simple check-config helper, which checks that all keys in
config.json are known to runtime-spec (and spelled exactly as in the
spec), and call it from update_config, so tests using wrong keys fail
right away.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commits 1f1e91b1 and 2192670a (PR 2917) added a check that a mount
destination is absolute (as the runtime spec required back then) to
both libcontainer/specconv and libcontainer/configs/validate.
This broke Moby, which used relative destinations for swarm configs
(issue 2928), so commit b31a9340 (PR 3004) turned both errors into
warnings, with TODOs to make them errors again in a future release.
As a result, runc prints two warnings for the same relative mount
destination, for example:
level=warning msg="mount destination tmp/bind is not absolute. Support for non-absolute mount destinations will be removed in a future release."
level=warning msg=configuration error="mount &{... Destination:tmp/bind ...}: relative destination path is **deprecated**, using it as relative to /"
Later, the runtime spec was amended to allow relative mount
destinations on Linux, treating them as relative to "/" (see
opencontainers/runtime-spec PR 1225), and commit b17c6f23 (PR 4013)
updated the validator accordingly, removing the TODO and changing the
warning text. The specconv part was left as is, still having the stale
TODO and the "will be removed" text.
Remove the warning from specconv altogether, as the one from the
validator is sufficient (and is also seen by libcontainer users who
do not use specconv).
Fixes: b31a9340f9 ("libcontainer: relax validation for absolute paths")
Fixes: b17c6f237d ("validator: Relax warning for not abs mount dst path")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since bats does not set pipefail, the exit code of jq in update_config
is ignored, and a jq error results in an empty config.json (as awk
happily writes an empty input), so the test fails later, for an
unrelated reason (or, worse, passes, as it happened with the
"timens offsets with no timens" test).
Save jq output to a variable, and only write it to config.json if jq
succeeded.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin