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>
(cherry picked from commit be00ae07c3)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Perform some basic checks for CHANGELOG.md.
In particular, check for
- missing periods;
- extra spaces at EOL;
- non-ASCII characters.
Fix the issues found.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This job is failing with "No space left on device" lately, and this
helps to fix it.
Besides, it seems that caching does not help to shorten execution times
(validate/release job succeeds in under 8 minutes now; ymmv).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
golangci-lint-action v3 no longer installs golang itself, and the
version that comes with Ubuntu is not new/good enough.
Install go 1.17.x explicitly.
Introduce GO_VERSION environment variable to avoid duplication,
and use it instead of 1.x in other places, so that implicit go update
won't bring some unexpected failures.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit f7637defb8)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Also, remove "must be specified without patch version" as this is no
longer true.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
(cherry picked from commit f7d4613492)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This adds a new GHA CI job which runs a few extra linters. This is only
done for pull requests, and should only warn about new code.
The justification is simple: we want more linters, but since this is not
a new project, adding a new linter meaning we have to fix all the
existing warnings. In some cases having all the warnings fixed is
difficult and takes time, plus it is usually a low priority task.
Therefore, we are stuck with inability to add new linters because we
can't fix all their warnings. Meanwhile, new pull requests add more
code which is not linted.
This is an attempt to break this vicious cycle. Let's enable godot
and revive for now and see how it is going.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Jobs verify/compile-buildtags and verify/deps relied on whatever Go
version is available from the Ubuntu-20.04 image, which seems to be
1.15.x).
Job test/cross-i386 was installing whatever Go version is considered to
be the default one by actions/setup-go@v2, which seems to be go 1.15.15
at the moment.
Fix all three jobs to install Go 1.x (which should translate to latest
stable Go version, i.e. 1.17.2 as of now).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This implements cross-build for "make release", moving the build into a
container. This way we can support arm, arm64, ppc, and whatnot.
* script/seccomp.sh: separate out of script/release.sh, amend to support
cross-compile and save needed environment variables to a file.
* Dockerfile: add installing libseccomp from source, as this is needed
for release builds.
* script/release.sh: amend to support more architectures in addition to
the native build. Additional arches can be added by specifying
"-a <arch>" argument (can be specified multiple times), or
"make RELEASE_ARGS="-a arm64" release" if called via make.
All supported architectures can be enabled via "make releaseall".
* Makefile: move "release" target to "localrelease", add "release" and
"releaseall" targets to build via the Dockerfile. This is done because
most distros (including Fedora and openSUSE) lack cross-glibc, which is
needed to cross-compile libseccomp.
* Makefile: remove 'cross' and 'localcross' targets, as this is now done
by the release script.
* .github/workflows/validate.yum: amend the release CI job to cross-build
for supported architectures, remove cross job.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Treat warning as errors only in the CI. We can enforce it in the source
code (like setting CFLAGS in libcontainer/nsenter/nsenter.go), but that
can force other downstream to patch the code if thei C compiler produces
warnings. For that reason, we do it only on the CI.
Todays CGO warnings are quite hidden in the CI (only shown for the
compilation step, that is collapsed) and CI is green anyways. With this
patch, CI fails if a warning is introduced.
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
The two exceptions I had to add to codespellrc are:
- CLOS (used by intelrtd);
- creat (syscall name used in tests/integration/testdata/seccomp_*.json).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Today we support the seccomp build tag only that is used by default.
However, we are not testing that compiling without any build tag works.
I found the CI didn't catch this when working on #2682, that the CI was
green but compilation without build tags was broken.
We test compilation without build tags only, compilation with the only
build tag supported is done extensively in other actions.
Signed-off-by: Rodrigo Campos <rodrigo@kinvolk.io>
It seems that golangci-lint didn't warn us about new lint errors that
were added after we enabled it, so just run the full thing and give us
all the errors on every PR run -- as long as we keep master lint-clean
it doesn't matter whether we set this or not.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Most of these were false positives or cases where we want to ignore the
lint, but the change to the BPF generation is actually useful.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
libseccomp is LGPL, meaning if we statically link it, we have to include
the source code of the library.
Amend "make release" to download and build libseccomp, build runc
against it, and include its sources into the release directory.
The only caveat is I found no way to stop go build from using the
stock (distro-provided) libseccomp.a, so the script checks that
the stock libseccomp.a is not available, and aborts otherwise.
While at it:
- enable shellcheck for script/release.sh
- remove libseccomp installation from the gha job
- add dependecies needed for libseccomp build to the gha job
[v2: also include libseccomp .asc file]
[v3: rebase]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This uploads the results of make release step (static binary and
a source tarball) so that they are available. I am not very sure if
it's of any use, but at least one can download and play with a static
binary from any PR.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, scripts/validate-c is not working in CI (or maybe
maintainers ignored the failures from it) -- current C code
gets some changes if we run indent on it.
This commit fixes this, simplifying things along the way.
In particular:
1. Remove "validate" make target, add "cfmt" target that just runs
indent on all *.c files in the repository (NOTE that *.h files
are not included, as before).
This may help a contributor to fix their code -- they just need
to run "make cfmt" now instead of running "make validate" and
copy-pasting the indent command and options from the hint.
2. Split GHA validate/misc into validate/release and validate/cfmt.
The latter checks that the sources are not changed after "make cfmt".
3. Adds a few more options to indent. This was mostly motivated by
trying to save the existing formatting, minimizing the amount of
changes indent produces.
The new options are:
* -il0: sets the offset for goto labels to 0 (currently all labels
but one are not indented -- let's keep it that way);
* -ppi2: sets the indentation for nested preprocessor directives
to 2 spaces (same as it is done in "SYS_memfd_create" defines);
* -cp1: sets the indentation between #else / #endif and the
following comment to 1 space.
4. Reformat the code using the new indent options.
5. Remove the now-unused script/{.validate,validate-c}.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes github gives up 5xx errors, for example:
> panic: getImages error exit status 1 (output: curl: (22) The requested URL returned error: 502
> Failed to get https://github.com/docker-library/busybox/raw/dist-i386/stable/glibc/busybox.tar.xz
This is unfortunate but temporary, and adding --retry should handle
at least some cases, improving CI stability.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Having -EPERM is the default was a fairly significant mistake from a
future-proofing standpoint in that it makes any new syscall return a
non-ignorable error (from glibc's point of view). We need to correct
this now because faccessat2(2) is something glibc critically needs to
have support for, but they're blocked on container runtimes because we
return -EPERM unconditionally (leading to confusion in glibc). This is
also a problem we're probably going to keep running into in the future.
Unfortunately there are several issues which stop us from having a clean
solution to this problem:
1. libseccomp has several limitations which require us to emulate
behaviour we want:
a. We cannot do logic based on syscall number, meaning we cannot
specify a "largest known syscall number";
b. libseccomp doesn't know in which kernel version a syscall was
added, and has no API for "minimum kernel version" so we cannot
simply ask libseccomp to generate sane -ENOSYS rules for us.
c. Additional seccomp rules for the same syscall are not treated as
distinct rules -- if rules overlap, seccomp will merge them. This
means we cannot add per-syscall -EPERM fallbacks;
d. There is no inverse operation for SCMP_CMP_MASKED_EQ;
e. libseccomp does not allow you to specify multiple rules for a
single argument, making it impossible to invert OR rules for
arguments.
2. The runtime-spec does not have any way of specifying:
a. The errno for the default action;
b. The minimum kernel version or "newest syscall at time of profile
creation"; nor
c. Which syscalls were intentionally excluded from the allow list
(weird syscalls that are no longer used were excluded entirely,
but Docker et al expect those syscalls to get EPERM not ENOSYS).
3. Certain syscalls should not return -ENOSYS (especially only for
certain argument combinations) because this could also trigger glibc
confusion. This means we have to return -EPERM for certain syscalls
but not as a global default.
4. There is not an obvious (and reasonable) upper limit to syscall
numbers, so we cannot create a set of rules for each syscall above
the largest syscall number in libseccomp. This means we must handle
inverse rules as described below.
5. Any syscall can be specified multiple times, which can make
generation of hotfix rules much harder.
As a result, we have to work around all of these things by coming up
with a heuristic to stop the bleeding. In the future we could hopefully
improve the situation in the runtime-spec and libseccomp.
The solution applied here is to prepend a "stub" filter which returns
-ENOSYS if the requested syscall has a larger syscall number than any
syscall mentioned in the filter. The reason for this specific rule is
that syscall numbers are (roughly) allocated sequentially and thus newer
syscalls will (usually) have a larger syscall number -- thus causing our
filters to produce -ENOSYS if the filter was written before the syscall
existed.
Sadly this is not a perfect solution because syscalls can be added
out-of-order and the syscall table can contain holes for several
releases. Unfortuntely we do not have a nicer solution at the moment
because there is no library which provides information about which Linux
version a syscall was introduced in. Until that exists, this workaround
will have to be good enough.
The above behaviour only happens if the default action is a blocking
action (in other words it is not SCMP_ACT_LOG or SCMP_ACT_ALLOW). If the
default action is permissive then we don't do any patching.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This check (for commit subject being short) only make sense for pull
requests, and it fails on pushes to master (i.e. on merges).
Make sure it's only run for pull requests.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It currently takes close to 2 minutes to build docker image
(which is currently only used for cross compilation).
Let's see if caching docker layers will help.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Note that `make ci` in Travis included `make test`, but we're already
doing that elsewhere (see .github/workflows/test.yml).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In here we have to use Docker, as Ubuntu does not support
all the architectures we're compile-testing here.
Since this is the only step that is using Docker,
there is no sense to separate `make runcimage` from
the rest of it. In case we'll have to use Docker image
more, it will make sense to do so.
While at it, ditch script/tmpmount (added by commit 1735ad788f),
because
- it required root (because mount);
- it is probably no longer needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Note that
1. No golang caching is required, since we do not build any go code,
and it does not look like it make sense to cache a single binary
we donwload from github.
2. Use a problem matcher, so the PRs will be annotated with
shellcheck warnings.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin