Skip to content

Commit 759e734

Browse files
zackeesclaude
andcommitted
feat(dylint): land ban_raw_subprocess governance lint (#264)
Closes #264. Lands the `ban_raw_subprocess` custom dylint that was prototyped in PR #262 and deferred. The lint fires on `Command::{spawn, output, status}` (both `std::process::Command` and `tokio::process::Command`) outside an explicit allowlist, so every subprocess fbuild launches has to flow through one of the blessed wrappers in `fbuild-core::{subprocess, containment}` and the running-process-core / Job-Object invariants can't drift. This commit addresses all three CR blockers from PR #262: 1. **CI integration script.** Ports `ci/build_dylint_driver.py` from zccache (same script, same dylint git rev `4bd91ce…`). Published `dylint_driver 5.0.0` doesn't compile against nightly-2026-03-26 (rustc internals drift — `env_depinfo` / `file_depinfo` no longer exist), so cargo-dylint needs a driver built from the same git rev as our `dylint_linting` pin. The script clones the dylint repo, builds a matching driver, and exports DYLINT_DRIVER_PATH. Two Windows-specific tweaks vs the zccache version: - Writes `rust-toolchain.toml` (not extensionless `rust-toolchain`) so rustup unambiguously parses TOML. - Anchors RUSTC/CARGO env vars to the nightly binaries so a chocolatey-installed stable `rustc` earlier in PATH can't shadow them and trip the build-script's `#![feature(...)]` with E0554. 2. **Scope filter.** Previous prototype's `SOURCE_PREFIX = "crates/"` matched `crates/*/tests/`, `examples/`, and `benches/` — flagging test drivers that legitimately spawn binaries under test. Tightened to `crates/*/src/` via `in_production_scope` (requires BOTH `crates/` AND a subsequent `/src/` segment in the file path). Out of scope by design: integration tests, example code, bench harnesses, `ci/`, `dylints/`, build scripts. 3. **Qualified-path call shape.** Prior prototype only handled `ExprKind::MethodCall` (`cmd.spawn()`) and missed `Command::spawn(&mut cmd)` written as a qualified-path call. Added an `ExprKind::Path` branch that uses `qpath_res` to resolve the path's DefId — mirrors zccache's `ban_unrooted_tempdir` pattern. Other pieces: - `dylints/ban_raw_subprocess/` — the dylint crate. Pinned to the same `nightly-2026-03-26` channel + `trailofbits/dylint` git rev as zccache's sibling dylints. `[workspace.exclude]`'d so the stable workspace build isn't affected by the nightly pin. - `src/allowlist.txt` — 7 files: the wrappers themselves (`subprocess.rs`, `containment.rs`), `containment_harness.rs` test harness, daemon-bootstrap from CLI (`daemon_client.rs`) and Python (`fbuild-python/src/daemon.rs`), cross-tool zccache daemon launch (`fbuild-build/src/zccache.rs`), and CLI async fan-out (`clang_tools.rs`). - 3 unit tests for scope / allowlist matching logic. - `[workspace.metadata.dylint]` in root Cargo.toml so `cargo dylint --all` discovers the lint. - `.github/workflows/dylint.yml` — Ubuntu-only CI job. Installs the pinned nightly + components, installs cargo-dylint 5.0.0, builds a matching driver via the python script, then runs the dylint crate's own unit tests plus `cargo dylint --all -- --workspace --all-targets`. The pre-existing `ci/find_direct_subprocess.py` string-matching guard stays — it catches `Command::new(` constructors (the import side); this dylint catches `.spawn()` / `.output()` / `.status()` methods (the call side). They're complementary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 700bc69 commit 759e734

12 files changed

Lines changed: 759 additions & 1 deletion

File tree

.github/workflows/dylint.yml

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
name: Dylint
2+
3+
# Runs the custom `ban_raw_subprocess` dylint over the workspace.
4+
# Lives in its own job (and uses its own nightly toolchain pin) so the
5+
# stable workspace check on `check-ubuntu.yml` isn't blocked by dylint
6+
# infrastructure issues. See #264.
7+
8+
on:
9+
push:
10+
branches: [main]
11+
pull_request:
12+
branches: [main]
13+
14+
env:
15+
CARGO_TERM_COLOR: always
16+
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true"
17+
18+
jobs:
19+
dylint:
20+
name: Dylint
21+
runs-on: ubuntu-latest
22+
steps:
23+
- uses: actions/checkout@v6
24+
- uses: astral-sh/setup-uv@v3
25+
- uses: zackees/setup-soldr@v0
26+
with:
27+
cache: true
28+
toolchain: 1.94.1
29+
cache-key-suffix: dylint
30+
prebuild-deps: none
31+
- name: Install Dylint nightly toolchain
32+
# Pin matches `dylints/ban_raw_subprocess/rust-toolchain.toml`
33+
# and zccache's `ci/build_dylint_driver.py::TOOLCHAIN_CHANNEL`.
34+
run: rustup toolchain install nightly-2026-03-26 --component llvm-tools-preview --component rust-src --component rustc-dev --profile minimal
35+
- name: Install Dylint
36+
run: soldr cargo install cargo-dylint dylint-link --version 5.0.0 --locked
37+
- name: Build compatible Dylint driver
38+
# Published dylint_driver 5.0.0 does not build against the
39+
# nightly toolchain pinned by dylint_linting 5.0.0; this script
40+
# builds a matching driver from the same git rev. Exports
41+
# DYLINT_DRIVER_PATH into $GITHUB_ENV for subsequent steps.
42+
run: uv run python ci/build_dylint_driver.py
43+
- name: Check dylint crate formatting
44+
run: soldr cargo fmt --manifest-path dylints/ban_raw_subprocess/Cargo.toml --all -- --check
45+
- name: Test dylint crate
46+
run: |
47+
export PATH="${CARGO_HOME}/bin:${PATH}"
48+
cargo +nightly-2026-03-26 test --manifest-path dylints/ban_raw_subprocess/Cargo.toml
49+
- name: Run dylint over workspace
50+
run: |
51+
export PATH="${CARGO_HOME}/bin:${PATH}"
52+
cargo dylint --all -- --workspace --all-targets

Cargo.toml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,14 @@ members = [
1616
"crates/fbuild-library-select",
1717
"bench/fastled-examples",
1818
]
19+
# Custom dylints have their own nightly toolchain pin (see
20+
# `dylints/*/rust-toolchain.toml`) and must not be part of the
21+
# stable workspace build — `cargo dylint --all` builds them with
22+
# the pinned nightly via the discovery below. See #264.
23+
exclude = ["dylints/ban_raw_subprocess"]
24+
25+
[workspace.metadata.dylint]
26+
libraries = [{ path = "dylints/*" }]
1927

2028
[workspace.package]
2129
version = "2.2.6"

ci/build_dylint_driver.py

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
"""Build a Dylint driver from the git revision used by the lint crate.
2+
3+
The published `dylint_driver` 5.0.0 crate does not build against the
4+
nightly toolchain pinned by CI. The lint crate already pins Dylint's git
5+
revision for `dylint_linting` and `dylint_testing`; this script builds the
6+
matching driver from that checkout and exports DYLINT_DRIVER_PATH.
7+
"""
8+
9+
from __future__ import annotations
10+
11+
import os
12+
import shutil
13+
import subprocess
14+
import sys
15+
import tempfile
16+
from pathlib import Path
17+
18+
19+
DYLINT_REPO = "https://github.com/trailofbits/dylint"
20+
DYLINT_REV = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09"
21+
TOOLCHAIN_CHANNEL = "nightly-2026-03-26"
22+
23+
24+
def run(args: list[str], **kwargs) -> subprocess.CompletedProcess[str]: # noqa: ANN003
25+
print("+", " ".join(args), flush=True)
26+
return subprocess.run(args, check=True, text=True, **kwargs)
27+
28+
29+
def rustc_host() -> str:
30+
# Invoke rustc through `rustup run` so the call works even when PATH
31+
# is fronted by shims (e.g. soldr) that do not understand the
32+
# `+<toolchain>` directive that only the rustup `cargo`/`rustc`
33+
# wrappers parse.
34+
output = subprocess.check_output(
35+
["rustup", "run", TOOLCHAIN_CHANNEL, "rustc", "-vV"],
36+
text=True,
37+
)
38+
for line in output.splitlines():
39+
if line.startswith("host: "):
40+
return line.split("host: ", 1)[1]
41+
raise RuntimeError("could not determine rustc host triple")
42+
43+
44+
def rustc_toolchain_root(full_toolchain: str) -> Path:
45+
rustc = subprocess.check_output(
46+
["rustup", "which", "--toolchain", full_toolchain, "rustc"],
47+
text=True,
48+
).strip()
49+
return Path(rustc).resolve().parent.parent
50+
51+
52+
def write_driver_package(package: Path, dylint_checkout: Path, full_toolchain: str) -> None:
53+
src = package / "src"
54+
src.mkdir(parents=True)
55+
56+
driver_path = str((dylint_checkout / "driver").resolve()).replace("\\", "\\\\")
57+
(package / "Cargo.toml").write_text(
58+
f"""
59+
[package]
60+
name = "dylint_driver-{full_toolchain}"
61+
version = "0.1.0"
62+
edition = "2018"
63+
64+
[dependencies]
65+
anyhow = "1.0"
66+
env_logger = "0.11"
67+
dylint_driver = {{ path = "{driver_path}" }}
68+
""".lstrip(),
69+
encoding="utf-8",
70+
)
71+
# Use `.toml` extension so rustup unambiguously parses as TOML.
72+
# The extensionless `rust-toolchain` form is ambiguous (single-line
73+
# vs TOML) and on Windows hosts has been observed to silently fall
74+
# through to the default toolchain, leaving the build script's
75+
# `#![feature(...)]` rejected as "stable channel".
76+
(package / "rust-toolchain.toml").write_text(
77+
f"""
78+
[toolchain]
79+
channel = "{full_toolchain}"
80+
components = ["llvm-tools-preview", "rustc-dev"]
81+
""".lstrip(),
82+
encoding="utf-8",
83+
)
84+
(src / "main.rs").write_text(
85+
"""
86+
#![feature(rustc_private)]
87+
88+
use anyhow::Result;
89+
use std::env;
90+
91+
pub fn main() -> Result<()> {
92+
env_logger::init();
93+
94+
let args: Vec<_> = env::args_os().collect();
95+
96+
dylint_driver::dylint_driver(&args)
97+
}
98+
""".lstrip(),
99+
encoding="utf-8",
100+
)
101+
102+
103+
def append_github_env(name: str, value: Path) -> None:
104+
github_env = os.environ.get("GITHUB_ENV")
105+
if github_env:
106+
with open(github_env, "a", encoding="utf-8") as file:
107+
file.write(f"{name}={value}\n")
108+
109+
110+
def main() -> int:
111+
full_toolchain = f"{TOOLCHAIN_CHANNEL}-{rustc_host()}"
112+
runner_temp = Path(os.environ.get("RUNNER_TEMP", tempfile.gettempdir())).resolve()
113+
driver_root = runner_temp / "dylint-drivers"
114+
driver_dir = driver_root / full_toolchain
115+
driver_dir.mkdir(parents=True, exist_ok=True)
116+
117+
with tempfile.TemporaryDirectory(prefix="fbuild-dylint-") as temp:
118+
temp_path = Path(temp)
119+
checkout = temp_path / "dylint"
120+
package = temp_path / "driver-package"
121+
122+
run(["git", "clone", "--filter=blob:none", DYLINT_REPO, str(checkout)])
123+
run(["git", "-C", str(checkout), "checkout", DYLINT_REV])
124+
125+
package.mkdir()
126+
write_driver_package(package, checkout, full_toolchain)
127+
128+
env = os.environ.copy()
129+
# Force the rustup toolchain in the env so it propagates into
130+
# nested cargo/rustc invocations (e.g. build-script compilation
131+
# of dylint_driver which uses `#![feature(...)]` and requires
132+
# nightly). Setting via env is more reliable than relying solely
133+
# on the `rust-toolchain.toml` lookup, especially on Windows.
134+
env["RUSTUP_TOOLCHAIN"] = full_toolchain
135+
# Anchor RUSTC and CARGO to the specific nightly binaries to
136+
# defeat shadowing by any stable `rustc`/`cargo` that may appear
137+
# earlier in PATH (e.g. a Chocolatey-installed stable on
138+
# Windows). Without this, cargo's build-script rustc invocation
139+
# may resolve to a stable rustc and fail on `#![feature(...)]`
140+
# with E0554.
141+
nightly_bin = rustc_toolchain_root(full_toolchain) / "bin"
142+
rustc_exe = nightly_bin / ("rustc.exe" if os.name == "nt" else "rustc")
143+
cargo_exe = nightly_bin / ("cargo.exe" if os.name == "nt" else "cargo")
144+
if rustc_exe.exists():
145+
env["RUSTC"] = str(rustc_exe)
146+
if cargo_exe.exists():
147+
env["CARGO"] = str(cargo_exe)
148+
if os.name != "nt":
149+
toolchain_root = rustc_toolchain_root(full_toolchain)
150+
rpath = f"-C link-args=-Wl,-rpath,{toolchain_root / 'lib'}"
151+
env["RUSTFLAGS"] = f"{env.get('RUSTFLAGS', '')} {rpath}".strip()
152+
153+
# Use `rustup run` instead of `cargo +<toolchain>` because the
154+
# cargo on PATH may be a shim (e.g. soldr's) that does not parse
155+
# the `+<toolchain>` directive — that directive is only honored
156+
# by the rustup-managed cargo wrapper. `rustup run` selects the
157+
# toolchain explicitly and works regardless of which `cargo`
158+
# comes first on PATH.
159+
run(
160+
["rustup", "run", TOOLCHAIN_CHANNEL, "cargo", "build"],
161+
cwd=package,
162+
env=env,
163+
)
164+
165+
exe_suffix = ".exe" if os.name == "nt" else ""
166+
built_driver = package / "target" / "debug" / f"dylint_driver-{full_toolchain}{exe_suffix}"
167+
installed_driver = driver_dir / f"dylint-driver{exe_suffix}"
168+
shutil.copy2(built_driver, installed_driver)
169+
170+
append_github_env("DYLINT_DRIVER_PATH", driver_root)
171+
print(f"DYLINT_DRIVER_PATH={driver_root}")
172+
return 0
173+
174+
175+
if __name__ == "__main__":
176+
sys.exit(main())

dylints/README.md

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
# `dylints/`
2+
3+
Custom [dylint](https://github.com/trailofbits/dylint) lints for fbuild
4+
production code. Each lint lives in its own crate so it can pin its own
5+
nightly toolchain (the rustc internal API moves fast; the workspace
6+
itself stays on stable 1.94.1).
7+
8+
## Crates
9+
10+
- **`ban_raw_subprocess/`** — forbids `Command::{spawn, output, status}`
11+
on `std::process::Command` and `tokio::process::Command` in production
12+
code (`crates/*/src/`). All subprocess spawns must flow through
13+
`fbuild_core::subprocess::run_command` /
14+
`fbuild_core::containment::*`. See #264.
15+
16+
## Running locally
17+
18+
```bash
19+
# One-time setup
20+
rustup toolchain install nightly-2026-03-26 \
21+
--component llvm-tools-preview --component rust-src --component rustc-dev \
22+
--profile minimal
23+
soldr cargo install cargo-dylint dylint-link --version 5.0.0 --locked
24+
uv run python ci/build_dylint_driver.py # builds a matching driver
25+
26+
# Run all dylints over the workspace
27+
export PATH="${CARGO_HOME:-$HOME/.cargo}/bin:${PATH}"
28+
cargo dylint --all -- --workspace --all-targets
29+
```
30+
31+
CI runs this on every push/PR via `.github/workflows/dylint.yml`.
32+
33+
## Why a separate toolchain pin
34+
35+
`dylint_linting` builds against a specific nightly rustc; the rustc
36+
internal API (`rustc_lint`, `rustc_hir`, `rustc_span`) changes between
37+
nightlies. Keeping the dylint crate in `[workspace.exclude]` lets it
38+
pin `nightly-2026-03-26` in its own `rust-toolchain.toml` without
39+
forcing the entire workspace to nightly.
40+
41+
The workspace registers the lint directory via:
42+
43+
```toml
44+
[workspace.metadata.dylint]
45+
libraries = [{ path = "dylints/*" }]
46+
```
47+
48+
so `cargo dylint --all` picks it up automatically.
49+
50+
## Why `build_dylint_driver.py`
51+
52+
Published `dylint_driver` 5.0.0 doesn't compile against the
53+
nightly-2026-03-26 toolchain (rustc internals drift). `cargo-dylint` would
54+
try to build it from crates.io and fail with `E0609: no field
55+
env_depinfo`. The script clones the dylint repo at the same git rev
56+
`dylint_linting` is pinned to (`4bd91ce…`) and builds a matching driver
57+
from that source, installing it where `cargo-dylint` expects.
58+
59+
This mirrors zccache's approach 1:1; the script is a direct port.
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
/target
2+
/Cargo.lock
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
[package]
2+
name = "ban_raw_subprocess"
3+
version = "0.1.0"
4+
description = "Ban raw std/tokio Command spawn/output/status in fbuild production code"
5+
edition = "2021"
6+
publish = false
7+
8+
[lib]
9+
crate-type = ["cdylib"]
10+
11+
[dependencies]
12+
# Pinned to the same dylint_linting rev zccache's dylints use — same
13+
# rustup channel in rust-toolchain.toml, same upstream commit.
14+
dylint_linting = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" }
15+
16+
[dev-dependencies]
17+
dylint_testing = { git = "https://github.com/trailofbits/dylint", rev = "4bd91ce7729b74c7ee5664bbb588f7baf30b4a09" }
18+
19+
[package.metadata.rust-analyzer]
20+
rustc_private = true

0 commit comments

Comments
 (0)