Tsan fixes - #1662
Conversation
tokio's I/O driver runs on its own worker thread(s). When another thread
registers an I/O source , tokio allocates and initializes a `ScheduledIo`,
then hands the fd to the kernel via `epoll_ctl`. The driver worker only ever
sees that `ScheduledIo` after `epoll_wait` returns its token (strictly
after the registration syscall). That happens-before edge is real but runs
through the kernel, which ThreadSanitizer cannot instrument, so it reports a
false positive between RegistrationSet::allocate (init) and
Driver::turn -> ScheduledIo::{set_readiness,wake} (use). Matching either
tokio frame silences that pattern without hiding races in our own fd handling.
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
This can give us better stack traces in sanitized builds. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
This is NOT for distribution to customers. Instead, this is intended for use in our labs to help hunt down locking issues. Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Adds an opt-in `vlab_tsan` job that points VLAB at the thread-sanitized
image the `build` matrix already produces. Gated on the `ci:+tsan` label
or a workflow_dispatch input, and deliberately left out of `summary`'s
`needs`: this is a lab instrument for hunting locking issues, so a race
report should start an investigation rather than block a PR.
The only seam into the reusable VLAB workflow is `prebuild`, which runs
as an ordinary step in the same job as `hhfab vlab up`. That is enough:
it can bump the dataplane to the sanitized tag and raise the gateway VM
RAM via `$GITHUB_ENV` (the default 6144 MB is unlikely to survive TSan's
shadow memory, and the container has no memory limit, so the VM is the
ceiling). `collect_show_tech` is on because a race that TSan reports
without killing anything is a *passing* run, and show-tech is what
captures the container's stderr.
Three things had to change to make that tag reachable:
Container tags now carry a sanitizer suffix. The profile alone does not
identify a build -- `fuzz` is shared by every sanitizer variant -- so
thread and address builds would otherwise collide. This mirrors the
`version_san` suffix the justfile already computes locally.
Only the plain release build writes the bare `${VERSION}` tag. All three
matrix cells were writing it, so its contents were decided by whichever
job finished last.
The validator is built unsanitized but still published under the variant
tag. rustc has no sanitizer support for `wasm32-wasip1`, and
`nix/platforms.nix` does not filter sanitizers per platform, so the flags
reached rustc and failed the build. Skipping the cell is not an option:
fabricator derives the validator ref from the single `DataplaneVersion`
field, so pointing that at a sanitized dataplane makes hhfab mirror a
matching `dataplane/validator` tag that has to exist.
Finally, `__tsan_default_options` sets `exitcode=0`. TSan's default of 66
turns "we found a race" into a DaemonSet restart loop, which destroys the
evidence and disguises the finding as a liveness failure. The hook lives
next to the existing suppressions hook because the DaemonSet's
environment is fixed by the gateway controller, leaving no seam for
`TSAN_OPTIONS`.
Signed-off-by: Daniel Noland <daniel@githedgehog.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves sanitizer (notably TSan) usability across the Nix build + container packaging flow by ensuring frame pointers are present, embedding TSan suppressions into the binary, and wiring up symbolization/debug-info so sanitizer reports are actionable. It also extends CI to exercise a thread-sanitized build variant.
Changes:
- Add frame-pointer flags for ASan/LSan/TSan builds (C/C++ and Rust) in Nix profiles.
- Embed a compiled-in TSan suppression list via a sanitizer hook, with a build script that exposes sanitizer selection as
cfgs. - Update Nix container/tar build logic and dev CI matrix to better support sanitizer debugging workflows (llvm-symbolizer/debug-info plumbing, additional build variant).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| nix/profiles.nix | Adds frame-pointer flags to sanitizer profiles for better stack traces. |
| default.nix | Introduces is-sanitized, includes .suppress files in cleaned sources, and adds/debug-wires symbolization + debug-info in images. |
| dataplane/src/sanitizer/tsan.suppress | Adds a TSan suppression list for known tokio I/O-driver false positives. |
| dataplane/src/sanitizer/mod.rs | Adds the __tsan_default_suppressions hook to bake suppressions into the binary. |
| dataplane/src/main.rs | Wires the new sanitizer module into the binary build (non-loom). |
| dataplane/build.rs | Detects active -Zsanitizer=<kind> from rustflags and emits corresponding cfgs. |
| Cargo.lock | Updates the lockfile to reflect dependency resolution changes. |
| .github/workflows/dev.yml | Adjusts build matrices and adds a thread-sanitizer build variant. |
Comments suppressed due to low confidence (1)
dataplane/build.rs:24
confidence: 8
tags: [logic]
This build script emits `cargo::...` directives. Other build scripts in this repo use the more widely-supported `cargo:...` form; switching to `cargo:` avoids depending on newer Cargo behavior and keeps the style consistent.
// Declare every cfg we might set so the crate's `unexpected_cfgs` lint stays
// quiet even in the builds where we don't set it.
for (_, cfg) in SANITIZERS {
println!("cargo::rustc-check-cfg=cfg({cfg})");
}
</details>
| /// rustflags here keeps the crate stable-buildable while still letting | ||
| /// `src/sanitizer.rs` compile in the matching runtime hooks (e.g. baked-in | ||
| /// `ThreadSanitizer` suppressions) for -- and only for -- the relevant sanitizer | ||
| /// build. |
| // Trailing `"\0"` makes this a valid C string: the sanitizer hook returns a raw | ||
| // `*const c_char` that the TSan runtime reads until a NUL, and `include_str!` | ||
| // alone yields no terminator. `suppress.txt` never contains an interior NUL, so | ||
| // the whole file survives to the terminator. |
| @@ -0,0 +1,11 @@ | |||
| # tokio's I/O driver runs on its own worker thread(s). When another thread | |||
| # registers an I/O source , tokio allocates and initializes a `ScheduledIo`, | |||
| paths = [ | ||
| pkgs.pkgsHostHost.dockerTools.fakeNss | ||
| pkgs.pkgsHostHost.busybox | ||
| pkgs.pkgsHostHost.dockerTools.fakeNss | ||
| pkgs.pkgsHostHost.dockerTools.usrBinEnv | ||
| pkgs.pkgsHostHost.libc.debug | ||
| pkgs.pkgsHostHost.llvmPackages'.llvm | ||
| pkgs.pkgsHostHost.llvmPackages'.llvm.lib |
| - name: "thread" | ||
| profile: "fuzz" | ||
| sanitize: "thread" | ||
| instrument: "none" |
5999675 to
54f1d76
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (5)
default.nix:817
confidence: 8
tags: [logic]
`dataplane.tar` currently always includes `perf` and the `dp-debug` environment in the tar inputs, which will significantly increase image size for non-sanitized builds and undermines the `is-sanitized` intent.
If these tools are only needed for sanitizer runs, gate them behind `is-sanitized` (similar to `llvm-symbolizer-pkg`).
${workspace.cli} \
${pkgs.pkgsHostHost.busybox} \
${pkgs.perf} \
${lib.optionalString is-sanitized "${llvm-symbolizer-pkg}"} \
${dp-debug}
**default.nix:844**
* ```yaml
confidence: 7
tags: [logic]
containers.dataplane now unconditionally includes libc.debug and the full LLVM toolchain in the runtime image. That’s a large size/attack-surface increase for non-sanitized builds and conflicts with the earlier is-sanitized comment about avoiding production bloat.
Consider keeping the base image minimal and only adding these dependencies when is-sanitized is true (or moving them to the dedicated dataplane-debugger image).
paths = [
pkgs.pkgsHostHost.busybox
pkgs.pkgsHostHost.dockerTools.fakeNss
pkgs.pkgsHostHost.dockerTools.usrBinEnv
pkgs.pkgsHostHost.libc.debug
pkgs.pkgsHostHost.llvmPackages'.llvm
pkgs.pkgsHostHost.llvmPackages'.llvm.lib
workspace.cli
# workspace.cli.debug
workspace.dataplane
# workspace.dataplane.debug
workspace.init
# workspace.init.debug
dataplane/src/sanitizer/mod.rs:36
confidence: 6
tags: [docs]
The comment refers to `suppress.txt`, but the included file is `tsan.suppress`. This makes the in-code documentation misleading when tracing how the C string is constructed.
// `*const c_char` that the TSan runtime reads until a NUL, and `include_str!`
// alone yields no terminator. `suppress.txt` never contains an interior NUL, so
// the whole file survives to the terminator.
concat!(include_str!("tsan.suppress"), "\0").as_ptr().cast()
**dataplane/build.rs:13**
* ```yaml
confidence: 6
tags: [docs]
The build-script docs mention src/sanitizer.rs, but the added sanitizer module lives at src/sanitizer/mod.rs. Updating the path avoids confusion when someone tries to follow the comment.
/// a nightly feature would break a plain `cargo build` on stable. Sniffing the
/// rustflags here keeps the crate stable-buildable while still letting
/// `src/sanitizer.rs` compile in the matching runtime hooks (e.g. baked-in
/// `ThreadSanitizer` suppressions) for -- and only for -- the relevant sanitizer
/// build.
dataplane/src/sanitizer/tsan.suppress:2
confidence: 7
tags: [docs]
There’s a small punctuation typo in this suppression-file comment (extra space before the comma), which makes the explanation harder to read.
registers an I/O source , tokio allocates and initializes a ScheduledIo,
</details>
| }; | ||
| config.Entrypoint = [ "/bin/dataplane" ]; | ||
| # config.Entrypoint = [ "/bin/perf" "-o" "/blah/dataplane-perf.data" "/bin/dataplane" ]; | ||
| config.Entrypoint = [ "/dataplane" ]; |
| ln -s "${workspace.cli}/bin/cli" "$tmp/dataplane-cli" | ||
| ln -s "${dp-debug}/lib" "$tmp/lib" | ||
| ${lib.optionalString is-sanitized '' |
Need to chat with Sergei before we do any of that Signed-off-by: Daniel Noland <daniel@githedgehog.com>
54f1d76 to
3d7b879
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
dataplane/src/sanitizer/mod.rs:45
confidence: 7
tags: [logic]
`__tsan_default_options` is not `pub`. Like `__tsan_default_suppressions`, this hook is discovered by the runtime via symbol lookup; keeping it exported helps ensure it is not treated as an internal symbol and that the runtime actually uses these defaults.
extern "C" fn __tsan_default_options() -> *const core::ffi::c_char {
**dataplane/src/sanitizer/mod.rs:34**
* ```yaml
confidence: 9
tags: [docs]
The comment mentions suppress.txt, but the embedded file is tsan.suppress. Updating the filename in the comment will reduce confusion when editing suppression contents.
// alone yields no terminator. `suppress.txt` never contains an interior NUL, so
dataplane/build.rs:11
confidence: 9
tags: [docs]
This comment references `src/sanitizer.rs`, but the sanitizer hooks live under `src/sanitizer/mod.rs` (module directory). Keeping the path accurate helps future readers find the hooks.
/// src/sanitizer.rs compile in the matching runtime hooks (e.g. baked-in
**dataplane/src/sanitizer/tsan.suppress:2**
* ```yaml
confidence: 10
tags: [style]
There’s an extra space before the comma in “source , tokio…”.
# registers an I/O source , tokio allocates and initializes a `ScheduledIo`,
default.nix:848
confidence: 9
tags: [logic]
`containers.dataplane` sets its entrypoint to `/dataplane`, but this image is built from a `buildEnv` that links `/bin` (so the binary is expected at `/bin/dataplane`). Unlike `dataplane.tar` or `containers.debug-tools`, this image doesn’t create a `/dataplane` symlink, so the entrypoint would fail if this output is used.
config.Entrypoint = [ "/dataplane" ];
**default.nix:38**
* ```yaml
confidence: 6
tags: [other]
is-sanitized is documented as a way to bundle sanitizer-only tooling “without bloating production images”, but this file now unconditionally adds large debug/tooling closures (e.g. dp-debug, pkgs.perf, llvmPackages'.llvm, libc.debug) to the dataplane.tar and containers.dataplane outputs. If the intent is truly “sanitizer-only”, these additions should be gated on is-sanitized; otherwise the comment should be updated to reflect that production images will also carry these tools.
# True for any `sanitize=` build (thread/address/leak/cfi/...). Used to bundle
# sanitizer-only tooling (llvm-symbolizer + its debug-info wiring) without
# bloating production images.
is-sanitized = sanitizers != [ ];
|
|
||
| #[cfg(sanitize_thread)] | ||
| #[unsafe(no_mangle)] | ||
| extern "C" fn __tsan_default_suppressions() -> *const core::ffi::c_char { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (6)
default.nix:848
confidence: 9
tags: [logic]
`containers.dataplane` sets `config.Entrypoint = [ "/dataplane" ]`, but the image contents are linked under `/bin` via `pathsToLink`, so `/dataplane` is unlikely to exist at runtime (while `/bin/dataplane` does). This would make the image fail immediately when started.
Change the entrypoint back to `/bin/dataplane`, or add an explicit `/dataplane` symlink to the image root.
config.Entrypoint = [ "/dataplane" ];
**default.nix:725**
* ```yaml
confidence: 8
tags: [logic]
$tmp/lib is currently symlinked to ${dp-debug}/lib unconditionally. That forces the large dp-debug closure into all images, even non-sanitized builds, which contradicts the intent of is-sanitized and increases image size/attack surface.
Consider keeping an ordinary /lib directory for non-sanitized builds, and only switching it to the debug-info tree for sanitized builds.
ln -s "${dp-debug}/lib" "$tmp/lib"
default.nix:838
confidence: 7
tags: [other]
`containers.dataplane` unconditionally adds `libc.debug` and `llvmPackages'.llvm*` to the image contents. If those are only needed for sanitized images, they should be gated on `is-sanitized` to avoid inflating non-sanitized images.
pkgs.pkgsHostHost.libc.debug
pkgs.pkgsHostHost.llvmPackages'.llvm
pkgs.pkgsHostHost.llvmPackages'.llvm.lib
**dataplane/src/sanitizer/mod.rs:35**
* ```yaml
confidence: 9
tags: [docs]
The comment refers to suppress.txt, but the embedded suppression file is tsan.suppress. This makes the explanation harder to follow when debugging sanitizer behavior.
// Trailing `"\0"` makes this a valid C string: the sanitizer hook returns a raw
// `*const c_char` that the TSan runtime reads until a NUL, and `include_str!`
// alone yields no terminator. `suppress.txt` never contains an interior NUL, so
// the whole file survives to the terminator.
dataplane/build.rs:12
confidence: 8
tags: [docs]
The build-script comment says `src/sanitizer.rs`, but the module added in this PR is `src/sanitizer/mod.rs`. Updating this avoids confusion for future edits.
/// rustflags here keeps the crate stable-buildable while still letting
/// src/sanitizer.rs compile in the matching runtime hooks (e.g. baked-in
/// ThreadSanitizer suppressions) for -- and only for -- the relevant sanitizer
**dataplane/src/sanitizer/tsan.suppress:2**
* ```yaml
confidence: 10
tags: [docs]
There’s an extra space before the comma in “I/O source ,” which reads like a typo.
# registers an I/O source , tokio allocates and initializes a `ScheduledIo`,
| ${pkgs.pkgsHostHost.busybox} \ | ||
| ${pkgs.perf} \ | ||
| ${lib.optionalString is-sanitized "${llvm-symbolizer-pkg}"} \ | ||
| ${dp-debug} |
| if: >- | ||
| ${{ | ||
| github.event_name == 'pull_request' | ||
| && contains(github.event.pull_request.labels.*.name, 'ci:+tsan') | ||
| || github.event_name == 'workflow_dispatch' && inputs.run_tsan_vlab | ||
| }} |
No description provided.