Repository navigation
Workarounds for different libseccomp version during runtime (vs compile time) - #129
Conversation
474fbe0 to
b73c1eb
Compare
|
@drakenclimber @pcmoore can you please take a look? This fixes a real issue. |
|
I also ran the patchset through Codex. It came up with another error, but I'm not sure how I feel about it:
v2.4.0 of libseccomp came out in March 2019. I admit that I would be tempted to only support v2.4.0 and newer, and avoid the problem of builds that don't have Thoughts? |
With that said, one way to solve this (and still support v2.3.1) would be to use weak references: |
Every other use of the variable is quoted; this one was not. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This separates code sensitive to libseccomp C headers version. While at it, fix the comment telling about the minimally required libseccomp version. No change in functionality. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For some reason, since commit 4b17538 the filter attributes missed from includes were defined to _SCMP_FLTATR_MIN (which is an invalid value) and so, when used, resulted in EINVAL (see [1], [2]). From the first sight it looks like those are to be never used in practice as we have the API level check. Yet, when the libseccomp version used during runtime is newer than the one used during build time, it is quite possible (since GetAPI returned the runtime version). Fix to use the correct numbers. [1]: kubernetes/kubernetes#140039 [2]: opencontainers/runc#5347 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Most functions that require recent libseccomp version check if that version is available, and return an appropriate error (VersionError). The problem is, checkVersion only looks at the run-time libseccomp version. When compiling against an older libseccomp and running with a newer one, the weakly referenced newer functions do resolve at run time, so the check passes -- even though the package was compiled without the corresponding structs and constants from the newer headers. Same for all other functionality that uses checkAPI or checkVersion. This also affects our own tests. To fix, let's use the minimum of runtime and compile time version. Version comparison is done via a single versionGE helper, shared by checkVersion and getMinVersion, rather than an ad-hoc bit-packing scheme that could silently corrupt comparisons if a minor or micro version ever exceeded 63. While at it: - fix the GetLibraryVersion doc comment: it claimed to return the version the bindings are built against, but it always returned the runtime version (and now returns the lower of the compile-time and run-time versions); - clarify, in both the VersionError error message and its doc comment, that the version being reported is the effective one, i.e. the lower of the compile-time and the run-time versions. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Version guards in Go are not sufficient to run a binary compiled against a newer libseccomp with an older run-time library, because the reference to a symbol that the older library does not have is not resolvable: > ./libseccomp-golang.test: symbol lookup error: ./libseccomp-golang.test: undefined symbol: seccomp_api_get Lazy binding merely delays this error until the first call (and is not even guaranteed -- distro toolchains commonly default to -z now), so it can not be relied upon. For seccomp_api_get in particular the call is made from init, meaning the program never starts at all. Declare all functions added after v2.3.1 (the minimum version supported by this package) as weak references instead. A weak reference to a symbol that is missing at run time resolves to NULL rather than being an error, so the library loads, and the existing version and API level guards can do their job and report a proper VersionError. Since a NULL pointer must not be called, all such functions are now called via compat_* wrappers which check for availability first. In practice the Go side guards already prevent these calls, so the wrappers are only a safety net. API level operations are a special case: since the API level guards are based upon them, they can not themselves use checkAPI, and a plain availability check is not enough either -- the symbol being weak, a package compiled against libseccomp < v2.4.0 would start reporting an API level merely because the run-time library happens to be newer. Guard getAPI and setAPI with checkVersion, which, as of the previous commit, takes the compile-time version into account, thus retaining the existing behavior of GetAPI and SetAPI. As a side effect, the weak declarations also replace the dummy implementations that were needed to compile against older headers, as they provide the missing prototypes. Finally, add a check-symbols make target, guarding against the two ways this can regress unnoticed, neither of which is visible on the build host: a strong reference to a post-v2.3.1 symbol, making the binary fail to load with an older library, and a direct call from Go to a weakly referenced one, which would call a NULL pointer. The latter can not be seen in the symbol table (a weak reference stays weak no matter who calls it), so it is checked for in the sources instead. As the check needs the test binary, split the test target into test-build and test-run. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Similar to the previous fix for checkVersion, getAPI() (used by checkAPI) needs to use the minimum of the compile-time and run-time API levels. Otherwise, if the bindings are compiled against an older libseccomp (whose seccomp_compat.h shims stub out functionality like SCMP_FLTATR_CTL_SSB or the notify API) but a newer libseccomp is loaded at run time, seccomp_api_get() reports a higher level than the compiled bindings can actually use, and checkAPI incorrectly allows callers through. Add SCMP_COMPAT_MAX_API_LEVEL to seccomp_compat.h, derived from the version-to-API-level mapping documented in seccomp_api_get(3) and consistent with the existing version gates in that file, and use it to cap the value returned by getAPI(). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since we're not checking API level here, it is slightly more preferable to use checkVersion. Fixes: 80346ad Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Otherwise, if the binary was compiled with libseccomp >= 2.6.0 and then used with libseccomp < 2.6.0, it fails like this: > ./libseccomp-golang.test: symbol lookup error: ./libseccomp-golang.test: undefined symbol: seccomp_transaction_start Apparently, Go does lazy resolving for C symbols, which, on one hand, allows to use the binary with older libseccomp, but on the other hand, once you call the unsupported functionality, it fails. So, let's add runtime version guards, similar to ones guarding seccomp_notify_* calls. Fix the tests accordingly, adding a checkVersionedErr helper for the common "want an error unless this version is available" assertion. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This should help reveal code and test bugs when the package is used with a different version of libseccomp from the one it was built against. This (currently) tests both scenarios (when compile time libseccomp version is less than that of runtime, and vice versa). Rather than skipping TestExpectedSeccompVersion, tell it to expect the lower of the compile-time and the run-time versions, which is what the package reports. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The test job built libseccomp from source in every matrix cell, which, with 3 Go versions x 5 libseccomp versions x 4 runners, meant building the very same library 60 times per workflow run. Move the build into a separate job, which builds each version once per architecture and uploads the result as an artifact, and have the test job download and unpack it instead. The build recipe now exists in a single place, ready to be reused by the job added next. The library is installed to a fixed absolute path, since libseccomp.pc records the prefix it was configured with, so the artifact has to be unpacked to the very same place it was built for. It is packed into a tarball because GitHub artifacts preserve neither symlinks nor file modes. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The test job builds against a single libseccomp version and runs against either the same one, or the distro-provided one, so a binary built against a newer libseccomp and used with an older one -- the case that needs weak references to work at all -- was never actually tested. Add a job which builds the test binary against one version, and then runs that very same binary against every version, in both directions, expecting to see the lower of the two reported. The libseccomp builds come from the artifacts of the job added in the previous commit, so no libseccomp is built here. A few notes on how this is done: - LD_LIBRARY_PATH is used rather than LD_PRELOAD, so that the version being tested is the only libseccomp that can be loaded at all. The job makes sure the library is resolved as intended, rather than assuming it. - LD_BIND_NOW is used to have the dynamic linker resolve all the symbols upfront. Otherwise, a missing symbol is only discovered once the affected function is called, meaning most of the breakage this is supposed to catch would go unnoticed here, only to be found by users whose libseccomp is older than that of whoever built the binary. - The Go build cache is disabled, as it takes neither PKG_CONFIG_LIBDIR nor the flags returned by pkg-config into account. Without this, a cache entry populated by another matrix cell is happily reused for a different libseccomp version, and the job silently tests nothing. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The matrix was a full product of 3 Go versions, 5 libseccomp versions and 4 runners, i.e. 60 jobs. The Go version, though, does not interact with the libseccomp version in any way: the C shims, the weak symbol declarations and the version guards are all the same no matter which Go compiles them. Sweeping every Go version across every libseccomp version and every runner therefore tests the same thing over and over. Keep the full libseccomp x runner coverage with the latest Go, and give the other supported Go versions a cell each, bringing this down to 22 jobs. Every libseccomp version is still built and tested on both architectures and on both distro versions. While at it, do not build libseccomp v2.3.1 for arm64, as the only user of it is the amd64-only cross-version job. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
@drakenclimber thanks for suggestion (I tried weak symbols before but for some reasons they didn't work for me last time). I've implemented weak symbols for newer functionality, and tests of various libseccomp build/runtime version combinations. The patchset looks a bit excessive (and somewhat limiting -- theoretically, we can use newer libseccomp functionality, but I am not aiming for this). Yet I believe the fixes cover all the corner cases now. If you have time, please take another look. |
|
@pcmoore @drakenclimber please take a look |
|
Thanks for adding the weak symbol logic, @kolyshkin. I realize it was a lot of work, but I think it is a good change. The changes look good to me.
|
|
0.12.0 with all this is out the door now |
This started a week ago when I looked into kubernetes/kubernetes#140039 and opencontainers/runc#5347, and unfolded to be a much bigger problem.
When we use different libseccomp versions during compile and run time, all hell break loose due to some of the code assumptions being wrong. Example:
libseccomp-golang/seccomp.go
Lines 25 to 29 in 3f07703
In here, we use seccomp_precompute wrapper which returns an error for compile-time libseccomp < 2.6.0. Next,
checkAPIandcheckVersionchecks are done against the runtime libseccomp, and thus if runtime libseccomp >= 2.6.0 we actually call this wrapper and get an error.For this PR, I chose a conservative way to fix this: limit the version number (and the API level) to whatever was available during compile time. This way, the new functionality of newer libseccomp at runtime is not available (until you recompile against its headers), but there are no bad surprises either.
(A more radical approach would be to try to get most of the runtime libseccomp version, even if it's newer, but that needs
-ldl, use ofdlsymetc).Initial CI improvements made while working on this are now moved to #130.