Currently, TestInit sets up logrus, and init uses it to log an error
from StartInitialization(). This is solely used by TestExecInError
to check that error returned from StartInitialization is the one it
expects.
Note that the very same error is communicated to the runc init parent
and is ultimately returned by container.Run(), so checking what
StartInitialization returned is redundant.
Remove logrus setup and use from TestMain/init.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a cosmetic change to improve code readability, making it easier
to distinguish between a local error and the error being returned.
While at it, rename e to err (it was originally called e to not clash
with returned error named err) and ee to err2.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of having newContainerInit return an interface, and let its
caller call Init(), it is easier to call Init directly.
Do that, and rename newContainerInit to containerInit.
I think it makes the code more readable and straightforward.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit e3cf217cf1 actions/setup-go@v4 uses caching
implicitly, and olangci/golangci-lint-action also uses caching.
These two caches clash, resulting in multiple warnings in CI logs.
The official golangci-lint-action solution is to disable caching
for setup-go job (see [1]). Do the same.
[1] https://github.com/golangci/golangci-lint-action/pull/704
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As of today, installing fails with
> libc6:i386 : Depends: libgcc-s1:i386 but it is not going to be installed
Add the package explicitly to work around that.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The first kludge is not needed since the switch to Ubuntu 22.04 in
commit 953e1cc48.
The second one is not needed since Go 1.20.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Adding fd field to mountConfig was not a good thing since mountConfig
contains data that is not specific to a particular mount, while fd is
a mount entry attribute.
Introduce mountEntry structure, which embeds configs.Mount and adds
srcFd to replace the removed mountConfig.fd.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Simplify mount call by removing the procfd argument, and use the new
mount() where procfd is not used. Now, the mount() arguments are the
same as for unix.Mount.
2. Introduce a new mountViaFDs function, which is similar to the old
mount(), except it can take procfd for both source and target.
The new arguments are called srcFD and dstFD.
3. Modify the mount error to show both srcFD and dstFD so it's clear
which one is used for which purpose. This fixes the issue of having
a somewhat cryptic errors like this:
> mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted
(in which fd 11 is actually the source, and fd 12 is the target).
After this change, it looks like
> mount src=/proc/self/fd/11, dst=/sys/fs/cgroup/systemd, dstFD=/proc/self/fd/12, flags=0x20502f: operation not permitted
so it's clear that 12 is a destination fd.
4. Fix the mountViaFDs callers to use dstFD (rather than procfd) for the
variable name.
5. Use srcFD where mountFd is set.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When given an environment variable that is invalid, it's not a good idea
to output the contents in case they are supposed to be private (though
such a container wouldn't start anyway so it seems unlikely there's a
real way to use this to exfiltrate environment variables you didn't
already know).
Reported-by: Carl Henrik Lunde <chlunde@ifi.uio.no>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
1. Use strings.TrimPrefix instead of fmt.Sscanf and simplify the code.
2. Add a test case and a benchmark.
The benchmark shows some improvement, compared to the old
implementation:
name old time/op new time/op delta
FindDeviceGroup-4 39.7µs ± 2% 26.8µs ± 2% -32.63% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
FindDeviceGroup-4 6.08kB ± 0% 4.23kB ± 0% -30.39% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
FindDeviceGroup-4 117 ± 0% 6 ± 0% -94.87% (p=0.008 n=5+5)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A version of vagrant available from the stock repos (2.2.19) is too old
and contains a bug that prevents downloading Fedora 38 image (see [1]).
Use packages from hashicorp repo, which currently has vagrant 2.3.4.
This resolves the problem of downloading the latest Fedora image.
Also, vagrant-libvirt plugin from Ubuntu repos is not working with
vagrant from hashicorp, so switch to using "vagrant plugin install".
The downside it, this takes extra 4 minutes or so in our CI, and I
am not sure how to cache it or speed it up.
[1] https://github.com/opencontainers/runc/pull/3835#issuecomment-1519321619
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 343951a22b added a call to os.Stat for the device path
when generating systemd device properties, to avoid systemd warning for
non-existing devices. The idea was, since systemd uses stat(2) to look
up device properties for a given path, it will fail anyway. In addition,
this allowed to suppress a warning like this from systemd:
> Couldn't stat device /dev/char/10:200
NOTE that this was done because:
- systemd could not add the rule anyway;
- runs puts its own set of rules on top of what systemd does.
Apparently, the above change broke some setups, resulting in inability
to use e.g. /dev/null inside a container. My guess is this is because
in cgroup v2 we add a second eBPF program, which is not used if the
first one (added by systemd) returns "access denied".
Next, commit 3b9582895b fixed that by adding a call to os.Stat for
"/sys/"+path (meaning, if "/dev/char/10:200" does not exist, we retry
with "/sys/dev/char/10:200", and if it exists, proceed with adding a
device rule with the original (non-"/sys") path).
How that second fix ever worked was a mystery, because the path we gave
to systemd still doesn't exist.
Well, I think now I know.
Since systemd v240 (commit 74c48bf5a8005f20) device access rules
specified as /dev/{block|char}/MM:mm are no longer looked up on the
filesystem, instead, if possible, those are parsed from the string.
So, we need to do different things, depending on systemd version:
- for systemd >= v240, use the /dev/{char,block}/MM:mm as is, without
doing stat() -- since systemd doesn't do stat() either;
- for older version, check if the path exists, and skip passing it on
to systemd otherwise.
- the check for /sys/dev/{block,char}/MM:mm is not needed in either
case.
Pass the systemd version to the function that generates the rules, and
fix it accordingly.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For a previous attempt to fix that (and added test cases), see commit
9087f2e827.
Alas, it's not always working because of cgroup directory TOCTOU.
To solve this and avoid the race, add an error _after_ the operation.
Implement it as a method that ignores the error that should be ignored.
Instead of currentStatus(), use faster runType(), since we are not
interested in Paused status here.
For Processes(), remove the pre-op check, and only use it after getting
an error, making the non-error path more straightforward.
For Signal(), add a second check after getting an error. The first check
is left as is because signalAllProcesses might print a warning if the
cgroup does not exist, and we'd like to avoid that.
This should fix an occasional failure like this one:
not ok 84 kill detached busybox
# (in test file tests/integration/kill.bats, line 27)
# `[ "$status" -eq 0 ]' failed
....
# runc kill test_busybox KILL (status=0):
# runc kill -a test_busybox 0 (status=1):
# time="2023-04-04T18:24:27Z" level=error msg="lstat /sys/fs/cgroup/devices/system.slice/runc-test_busybox.scope: no such file or directory"
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These checks ensure that all of the keys in the runc.keyring list are
actually the keys of the specified user and that the users themselves
are actually maintainers.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
We need to make sure the release is being signed by a key that is
actually listed as a trusted signing key, and we also need to ask the
person cutting the release whether the list of trusted keys is
acceptable.
Also add some verification checks after a release is signed to make sure
everything was signed with the correct keys.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Mrunal Patel