Foreground runc exec and runc run forwards all the signals (that it can)
to the process being run.
Since Go 1.14, go runtime uses SIGURG for async preemptive scheduling.
This means that runc regularly receives SIGURG and, in case of
foreground runc run/exec, it gets forwarded to the container process.
For example:
[kir@kir-rhat runc]$ sudo ./runc --debug exec xx67 sleep 1m
...
DEBU[0000] child process in init()
DEBU[0000] setns_init: about to exec
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
...
Or, with slightly better debug messages from commit 58c1ff39a5:
DEBU[0000]signals.go:102 main.(*signalHandler).forward() forwarding SIGURG to 819784
DEBU[0000]signals.go:102 main.(*signalHandler).forward() forwarding SIGURG to 819784
Obviously, this signal is an internal implementation detail of Go
runtime, and should not be forwarded to the container process.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Remove intelrtd.Manager interface, since we only have a single
implementation, and do not expect another one.
Rename intelRdtManager to Manager, and modify its users accordingly.
Remove NewIntelRdtManager from factory.
Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the
feature is not available.
Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew.
Add internal function newManager to be used for tests (to make sure
some testing is done even when the feature is not available in
kernel/hardware).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
TestGetContainerStats test a function that is smaller than the test
itself, and only calls a couple of other functions (which are
represented by mocks). It does not make sense to have it.
mockIntelRdtManager is only needed for TestGetContainerStats
and TestGetContainerState, which basically tests that Path
is called. Also, it does not make much sense to have it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since we are looking up the path to newuidmap/newgidmap in one context,
and executing those in another (libct/nsenter), it might make sense to
use a stricter rules for looking up path to those binaries.
Practically it means that if someone wants to use custom newuidmap and
newgidmap binaries from $PATH, it would be impossible to use these from
the current directory by means of PATH=.:$PATH; instead one would have
to do something like PATH=$(pwd):$PATH.
See https://go.dev/blog/path-security for background.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These were introduced in commit d8b669400 back in 2017, with a TODO
of "make binary names configurable". Apparently, everyone is happy with
the hardcoded names. In fact, they *are* configurable (by prepending the
PATH with a directory containing own version of newuidmap/newgidmap).
Now, these binaries are only needed in a few specific cases (when
rootless is set etc.), so let's look them up only when needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We only have one implementation of config validator, which is always
used. It makes no sense to have Validator interface.
Having validate.Validator field in Factory does not make sense for all
the same reasons.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those are *always* /proc/self/exe init, and it does not make sense
to ever change these. More to say, if InitArgs option func (removed
by this commit) is used to change these parameters, it will break
things, since "init" is hardcoded elsewhere.
Remove this.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Here's how it looks now:
$ runc --debug exec ctid sleep 1h
...
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
DEBU[0022]signals.go:102 main.(*signalHandler).forward() sending signal to process terminated
DEBU[0022]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
This is obviously not very readable.
Use unix.SignalName, plus a numeric representation of the signal, since
SignalName does not know all signals.
Add PID while we're at it.
With this commit:
DEBU[0000]signals.go:103 main.(*signalHandler).forward() forwarding signal 23 (SIGURG) to 891345
DEBU[0020]signals.go:103 main.(*signalHandler).forward() forwarding signal 45 () to 891345
DEBU[0020]signals.go:103 main.(*signalHandler).forward() forwarding signal 23 (SIGURG) to 891345
DEBU[0020]signals.go:103 main.(*signalHandler).forward() forwarding signal 23 (SIGURG) to 891345
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was introduced in an initial commit, back in the day when criu was
a highly experimental thing. Today it's not; most users who need it have
it packaged by their distro vendor.
The usual way to run a binary is to look it up in directories listed in
$PATH. This is flexible enough and allows for multiple scenarios (custom
binaries, extra binaries, etc.). This is the way criu should be run.
Make --criu a hidden option (thus removing it from help). Remove the
option from man pages, integration tests, etc. Remove all traces of
CriuPath from data structures.
Add a warning that --criu is ignored and will be removed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is needed since the future commits will touch this code, and then
the lint-extra CI job complains.
> libcontainer/factory.go#L245
> var-naming: var fdsJson should be fdsJSON (revive)
and
> libcontainer/init_linux.go#L181
> error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
and
> notify_socket.go#L94
> receiver-naming: receiver name n should be consistent with previous receiver name s for notifySocket (revive)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since the next commit is going to touch this structure, our CI
(lint-extra) is about to complain about improperly named field:
> Warning: var-naming: struct field ContainerId should be ContainerID (revive)
Make it happy.
Brought to use by gopls rename.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This method was removed earlier by commit 097c6d7425,
but the documentation was not updated. Fix it.
Fixes: 097c6d7425
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those were added by commit 59c5c3ac0 back in Apr 2015, but AFAICS were
never used and are obsoleted by more generic container hooks (initially
added by commit 05567f2c94 in Sep 2015).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 551629417 we can (and should) use Info() to get access to
file stat. Do this.
While going over directory entries, a parallel runc delete can remove
an entry, and with the current code it results in a fatal error (which
was not observed in practice, but looks quite possible). To fix,
add a special case to continue on ErrNotExist.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case of a read-only /dev, it's better to move on and let whatever is
run in a container to handle any possible errors.
This solves runc exec for a user with read-only /dev.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since we already called fstat, we know the current file uid. In case it
is the same as the one we want it to be, there's no point in trying
chown.
Remove the specific /dev/null check, as the above also covers it
(comparing /dev/null uid with itself is true).
This also fixes runc exec with read-only /dev for root user.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Use os/file Chown method instead of bare unix.Fchown as it already have
access to underlying fd, and produces nice-looking errors. This allows
us to remove our error wrapping and some linter annotations.
We still use unix.Fstat since os.Stat access to os-specific fields
like uid/gid is not very straightforward. The only change here is to use
file name (rather than fd) in the error text.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For the Nth time I wanted to replace parsing mountinfo with
statfs and the check for superblock magic, but it is not possible
since some code relies of mount options check which can only
be obtained via mountinfo.
Add a note about it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
dependabot[bot]