Conversation
kolyshkin
force-pushed
the
asan-check-job
branch
3 times, most recently
from
September 3, 2026 07:27
badf6fc to
19e5674
Compare
kolyshkin
changed the title
kolyshkin
force-pushed
the
asan-check-job
branch
2 times, most recently
from
September 3, 2026 18:27
b503de0 to
89735f3
Compare
kolyshkin
marked this pull request as ready for review
September 3, 2026 18:47
The capabilities structure passed to the exec code was allocated with xmalloc and every field was filled in afterwards -- every field except the _residual json_object pointer, which the generated free function passes to json_object_put. It was thus called on whatever the allocator happened to leave there. In practice glibc hands out zeroed memory here, so the pointer was NULL and json_object_put did nothing, but an allocator that fills fresh allocations with a pattern, such as the address sanitizer, makes "crun exec --cap" crash on exit: #0 json_object_put () from /lib64/libjson-c.so.5 #1 free_runtime_spec_schema_config_schema_process_capabilities () #2 free_runtime_spec_schema_config_schema_process () containers#3 cleanup_process_schemap () at src/exec.c:236 Use xmalloc0, as is already done a few lines above for the user structure. Signed-off-by: Kir Kolyshkin <[email protected]>
period_len is only read when period_str is not NULL, and the two are set together, but gcc does not make that connection once the sanitizers change what it inlines, and --enable-werror then fails the build: src/libcrun/cgroup-resources.c:1106:21: error: 'period_len' may be used uninitialized [-Werror=maybe-uninitialized] Initialize it. Signed-off-by: Kir Kolyshkin <[email protected]>
Add a configure option that builds libcrun, the crun binary and the testing library with the address and undefined behaviour sanitizers, so that the test suite can be run under them. The flags are kept in their own substituted variables and appended to the per-target flags, following what --enable-coverage already does, rather than added to CFLAGS. Putting them in CFLAGS would reach tests/init as well, which is linked statically and cannot be built with the sanitizers; it would also make the configure check for a working -static flag fail, which in turn disables the whole test suite. The undefined behaviour sanitizer needs -fno-sanitize-recover: without it it only prints what it finds and lets the program carry on, so a test suite run would stay green no matter what it reported. Signed-off-by: Kir Kolyshkin <[email protected]>
Run the test suite against a build with the address and undefined behaviour sanitizers. Nothing did so far: the check jobs pass ASAN_OPTIONS to make check, but nothing was ever built with a sanitizer, so the variable had no effect. The only sanitizer build is the fuzzing one, which covers the code the fuzzing targets reach and nothing else. Both as root and rootless: the two reach different code -- cgroup setup, the mount handling and the user namespace paths above all -- so both are worth looking at under the sanitizers. Signed-off-by: Kir Kolyshkin <[email protected]>
The fuzzer harness is a subreaper, so every process a container leaves behind is reparented to it. They were collected from a SIGCHLD handler, which has two problems. The handler consumes the status of the processes libcrun_container_run() is itself waiting for, so the non-detached target never saw its container exit. And because the reaping was asynchronous, a container from one iteration could outlive it and die during the next one, which under honggfuzz -- it traces the whole process tree -- attributes the death to an input that had nothing to do with it. Drop the handler and drain the children at the end of every container run instead, with a bound, since a container process is under no obligation to ever exit. Signed-off-by: Kir Kolyshkin <[email protected]>
Modes 0 and 1 fork and run a real container, and honggfuzz's crash counter cannot be used to judge them. honggfuzz attaches with PTRACE_O_TRACEFORK, so every process the container spawns is traced as well, and any of them dying from a signal is recorded as a crash of the target -- including the ones that are meant to die: killed by the fuzzed seccomp profile, killed by the kernel because a fuzzed rlimit made execve() fail past the point of no return, or killed by the runtime itself while it cleans the container up. None of honggfuzz's own filters can be used to sort those out: the crash counter is incremented before the stackhash blocklist is consulted, and --verifier gives up on a crash it could not unwind, which is exactly what these are. So drop --exit_upon_crash for those two, let the run finish, and decide afterwards by replaying every artifact in a fresh process with the sanitizers told to report what they catch. A memory error in the runtime is reported by ASan or UBSan, which are still live in the process that hits it; a container process killed after execve() has left the sanitizer runtime behind with the rest of its address space and produces nothing. Only the former fails the job. The other modes do not fork and keep asserting crashes_count:0. Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
asan-check-job
branch
from
September 3, 2026 21:30
89735f3 to
5e9dc22
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Run the test suite under the address and undefined behaviour sanitizers.
Nothing did so far: the check jobs already pass
ASAN_OPTIONSto everymake check, but nothing was ever built with a sanitizer, so the variable --and the
libasan6the workflow installs -- did nothing. The only sanitizerbuild is the fuzzing one, which covers what the fuzzing targets reach and
nothing else.
Four commits, the first two being what turning it on immediately found:
exec:the capabilities structure was allocated withxmallocand everyfield filled in except the
_residualjson_object pointer, which thegenerated free function hands to
json_object_put. glibc happens to returnzeroed memory there; with the sanitizer's allocator
crun exec --cap CAP_KILLsegfaults on exit, after the exec succeeded.cgroup:-Werrorfails the sanitizer build on amaybe-uninitializedthat gcc only sees once the sanitizers change what it inlines. The warning
is wrong --
period_lenis only read whenperiod_stris set, and they areset together -- but the build has to pass.
build: add --enable-sanitizers. The flags go into their own substitutedvariables appended to the per-target flags, the way
--enable-coveragedoes, rather than into
CFLAGS. InCFLAGSthey would reachtests/init,which is linked statically, and they would make the configure check for a
working
-staticfail, which silently disables the entire test suite.Note
-fno-sanitize-recover: without it UBSan only prints and the suitestays green regardless.
ci: add a check-sanitizers job, root and rootless -- the two reachdifferent code, cgroup setup and the mount and user namespace paths above
all.
The new job passes. The
testing-farmfailures are unrelated; they fail onevery PR right now.