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>
The test uses ".linux.namespace" (rather than ".linux.namespaces") in
its jq expression, which results in a jq error. As update_config does
not check jq exit code, config.json ends up being empty, and so the
subsequent runc run fails because of that, not because of what the test
is supposed to check.
Fix the jq expression, so that runc now fails with:
time namespace offsets specified, but time namespace isn't enabled in the config
Fixes: aa5f4c11 ("tests: add several timens tests")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The "runc run (hugetlb limits)" test uses "pagesize" key in
linux.resources.hugepageLimits entries in config.json.
The OCI runtime spec uses "pageSize" key. The test only works because
Go's encoding/json matches keys case-insensitively, which is not the
case for other OCI runtimes, such as crun, when running runc's
integration tests against those.
Use the proper key.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The "update cpuset parameters via resources.CPU" test uses "CPU",
"Cpus", and "Mems" keys in linux.resources in config.json, and in the
input for runc update -r.
The OCI runtime spec uses "cpu", "cpus", and "mems" keys. The test only
works because Go's encoding/json matches keys case-insensitively, which
is not the case for other OCI runtimes, such as crun, when running
runc's integration tests against those.
Use the proper keys. Since the CPU resources are already set by setup(),
merge the new values into them (which is what effectively happened with
the old keys, too).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The log file is opened with O_SYNC since commit 8f206929 ("Ensure logs
are flushed", PR #637). The intent was to make sure log entries are not
lingering in a buffer, but *os.File is not buffered: logrus does a
single Write per entry, which is a single write(2), and the data is
visible to any reader (e.g. containerd reading the log after runc
exits) as soon as it returns.
What O_SYNC actually does is wait for the data to hit the disk on every
write, which only helps to preserve the log in case of a host crash or
power loss. This is of little value for runc logs, while the cost on a
slow disk (e.g. a network-attached cloud volume) is substantial,
especially with --debug.
Remove O_SYNC.
Fixes: #5540
Suggested-by: Benjamin Peterson <benjamin@engflow.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
With the merge queue enabled, all required checks must also pass for the
merge group commit. As review-policy only runs on pull_request_review, it
is never reported there, and the queue times out.
Add the merge_group trigger, and skip the actual check for it: a pull
request can only be added to the queue after review-policy has passed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using host's / as the container rootfs does not work: with a private
mount namespace, pivot_root (or MS_MOVE in case of no-pivot) fails,
and without it, container mounts (such as proc, dev etc.) are mounted
on top of the host ones, effectively breaking the host.
Disallow it.
Add an integration test.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When the container does not have its own mount namespace, using
--no-pivot results in msMoveRoot being run in the host mount namespace.
It unmounts host's procfs and sysfs mounts, and then moves the container
rootfs on top of the host's /, effectively breaking the host.
Similarly, rootfs propagation is applied to / and to the parent mount of
the container rootfs. Without a private mount namespace, this changes
the propagation of host mounts.
Disallow both configurations.
Add integration tests (the --no-pivot one is marked as unsafe, since
without the fix it breaks the host).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Split security into mountns and selinuxCheck, as these are unrelated.
In mountns, return early if there is a private mount namespace.
No functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The order of checks is not important (none of them modify the config),
so sort the list alphabetically, making it easier to add new entries.
The only visible effect is which error is reported first in case
the configuration has multiple issues.
Suggested-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The runtime spec says that if umask is unspecified, it should not be
changed from the calling process' umask. runc, though, sets the umask
of the container init process to 0022 in this case (as it did before
umask support was added, and as crun does, too). For runc exec, the
umask is not changed.
Document this deviation.
Fixes#3533.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The runtime spec says ociVersion is REQUIRED and MUST be in SemVer
format, and a runtime must generate an error when encountering an
invalid value. runc does not validate ociVersion at all, accepting
a missing, empty, or invalid value, as well as a version with a
different major.
Document this deviation.
Fixes#1846.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The test_mount_order test used host rootfs paths as mount destinations,
relying on runc stripping the rootfs prefix, which is a legacy runc
behavior not described by the runtime spec.
Use paths inside the container instead, as the spec says, so the test
can also be used with other runtimes (e.g. crun). Bind mount sources
inside the container rootfs are still used, as this is what the test
is about.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Akihiro Suda