1. In case --root option is not provided, do nothing.
2. Instead of checking if root value is empty string, check it after
filepath.Abs, and reject "/". Improve docstring while at it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Variable xdgRuntimeDir is only checked to be non-empty. Change it to
a boolean.
2. Refactor so that os.Getenv is not called twice.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is a mix of styles when handling CLI commands. In most cases we
return an error, which is handled from app.Run in main.go (it calls
fatal if there is an error).
In a few cases, though, we call fatal(err) from random places.
Let's be consistent and always return an error. The only exception is
runc exec, which needs to exit with a particular exit code.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The value of root is already an absolute path since commit
ede8a86ec1, so it does not make sense to call filepath.Abs()
again.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. This valid warning is reported by shellcheck v0.8.0:
In tests/integration/helpers.bash line 38:
KERNEL_MINOR="${KERNEL_VERSION#$KERNEL_MAJOR.}"
^-----------^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.
Did you mean:
KERNEL_MINOR="${KERNEL_VERSION#"$KERNEL_MAJOR".}"
Fix this.
2. These (invalid) warnings are also reported by the new version:
In tests/integration/events.bats line 13:
@test "events --stats" {
^-- SC2030 (info): Modification of status is local (to subshell caused by @bats test).
In tests/integration/events.bats line 41:
[ "$status" -eq 0 ]
^-----^ SC2031 (info): status was modified in a subshell. That change might be lost.
Basically, this is happening because shellcheck do not really track
the call tree and/or local variables. This is a known (and reported)
deficiency, and the alternative to disabling these warnings is moving
the code around, which is worse due to more changes in git history.
So we have to silence/disable these.
3. Update shellcheck to 0.8.0.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
Commit 029b73c1b replaced a regular expression with code checking the
characters. Despite what the comment said about ASCII, the check was
performed rune by rune, not byte by byte.
Note the check was still correct, basically comparing int32's, but the
byte by byte way is a tad faster and more straightforward. The change
also fixes the issue of a misleading comment.
Benchmark before/after shows a modest improvement:
name old time/op new time/op delta
CheckPropertyName-4 164ns ± 2% 123ns ± 2% -24.73% (p=0.029 n=4+4)
name old alloc/op new alloc/op delta
CheckPropertyName-4 96.0B ± 0% 64.0B ± 0% -33.33% (p=0.029 n=4+4)
name old allocs/op new allocs/op delta
CheckPropertyName-4 6.00 ± 0% 4.00 ± 0% -33.33% (p=0.029 n=4+4)
Fixes: 029b73c1b
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 643f8a2b40 renamed isValidName to checkPropertyName, but fell
short of renaming its test and benchmark. Fix that.
Fixes: 643f8a2b40
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>
After adding some debug info to cloned_binary.c I found out that
is_self_cloned() is not working right when runc binary is on tmpfs,
resulting in one extra re-exec of runc.
With some added debug:
$ mkdir bin
$ sudo mount -t tmpfs tmp bin
$ sudo cp runc bin
$ sudo ./bin/runc --debug exec xxx true
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 1 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[763590]: try_bindfd: 5
DEBU[0000] nsexec[763590]: re-exec itself...
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 1 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[763590]: try_bindfd: -1
DEBU[0000] nsexec[763590]: fallback to make_execfd: 5
DEBU[0000] nsexec[763590]: re-exec itself...
DEBU[0000] nsexec[763590]: => is_self_cloned
DEBU[0000] nsexec[763590]: got seals 15 (want 15)
DEBU[0000] nsexec[763590]: <= is_self_cloned, is_cloned = 1
From the above, it is seen that
- `is_self_cloned` returns 0,
- `try_bindfd` is called and succeeds,
- runc re-execs itself,
- the second call to `is_self_cloned` returns 0 again (because GET_SEALS returns 1),
- runc falls back to `make_execfd`, and re-execs again,
- finally, the third `is_self_cloned` returns 1.
I guess that the code relied on the following (quoting fcntl(2)):
> Currently, file seals can be applied only to a file descriptor
> returned by memfd_create(2) (if the MFD_ALLOW_SEALING was employed).
> On other filesystems, all fcntl() operations that operate on seals
> will return EINVAL.
It looks like in case of a file on tmpfs it returns 1 (F_SEAL_SEAL).
With the fix:
DEBU[0000] nsexec[768367]: => is_self_cloned
DEBU[0000] nsexec[768367]: got seals 1 (want 15)
DEBU[0000] nsexec[768367]: no CLONED_BINARY_ENV
DEBU[0000] nsexec[768367]: <= is_self_cloned, is_cloned = 0
DEBU[0000] nsexec[768367]: try_bindfd: 5
DEBU[0000] nsexec[768367]: re-exec itself...
DEBU[0000] nsexec[768367]: => is_self_cloned
DEBU[0000] nsexec[768367]: got seals 1 (want 15)
DEBU[0000] nsexec[768367]: fstatfs says ro = 1
DEBU[0000] nsexec[768367]: fstat says nlink = 1
DEBU[0000] nsexec[768367]: <= is_self_cloned, is_cloned = 1
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>
The utils.Annotations was used here before only because it made it
possible to distinguish between "key not found" and "empty value" cases.
With the previous commit, utils.SearchLabels can do that, and so it
makes sense to use it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using strings.Split generates temporary strings for GC to collect.
Rewrite the function to not do that.
Also, add a second return value, so that the caller can distinguish
between an empty value found and no key found cases.
Fix the test accordingly.
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>
Kir Kolyshkin