mirror of https://github.com/lotabout/skim.git synced 2026-09-10 07:16:23 -04:00

feat: add cargo-fuzz targets for hand-rolled text parsers (#1106 )

* feat: add cargo-fuzz targets for hand-rolled text parsers

Skim's most panic-prone code is the hand-written byte/char-index
bookkeeping over untrusted input: ANSI stripping, --nth/--with-nth field
extraction, the fuzzy matching algorithms, the query->engine->match
pipeline, and the --bind key-map parser. Add five cargo-fuzz (libFuzzer)
targets covering each, asserting real invariants (char-boundary safety,
monotonic index mappings, match indices in bounds) rather than just
catching panics, plus a CI workflow that runs them on every push/PR
touching src/ or fuzz/ and a longer nightly session via cron.

* ci: wire fuzz targets into the existing test matrix

Replace the standalone fuzz.yml workflow with a `fuzz` job in the main
test.yml matrix, running each of the 5 fuzz targets for 60s (5 minutes
total per CI run) alongside nextest/clippy/msrv.

* ci: remove standalone fuzz workflow

Superseded by the fuzz job now in test.yml.

* ci: run fuzz job as a single job on the platform matrix

Reuse the existing linux/macos/windows matrix instead of a separate
per-target matrix; run all 5 fuzz targets sequentially in one step
(60s each, 5 minutes total). cargo-fuzz doesn't support Windows, so
the fuzzing step is skipped there while still installing the
toolchain for consistency with the rest of the matrix.

* ci: reuse existing yaml anchors in the fuzz job

Use *toolchain instead of a bespoke nightly-install step, matching
the coverage job's pattern of letting `cargo +nightly` auto-provision
the toolchain on demand.

* nix: add cargo-fuzz to the tests devShell

Makes cargo-fuzz available via `nix develop` alongside the other test
tooling, matching what CI installs for the fuzz job.

* ci: install cargo-fuzz via taiki-e/install-action

Matches how the other CI-only cargo subcommands (nextest, cargo-msrv,
cargo-llvm-cov) are installed, and is faster than compiling it from
source with cargo install.

* ci: force the native host target for cargo-fuzz

cargo-fuzz was picking a statically-linked musl target on the runner,
which fails since ASan can't link against a static libc. Pass the
actual host triple (from `rustc -vV`) explicitly so the sanitizer
build always targets the dynamically-linked gnu/darwin toolchain.

* ci: skip the whole fuzz job on windows

Rather than skipping just the fuzzing step, exclude the job entirely
for the windows-latest matrix entry via a job-level `if`, since
cargo-fuzz/libFuzzer has no Windows support.

* ci: gate the fuzz job with runner.os instead of matrix.os

Matches the runner.os-based conditionals already used elsewhere in
this workflow (the linux/macos dependency install steps) rather than
comparing matrix.os directly.

* ci: enable the fuzz job on windows without ASan

cargo-fuzz does support Windows, but AddressSanitizer on the MSVC
target needs the separate "C++ AddressSanitizer" VS component plus a
PATH tweak for its DLL, which this runner doesn't have configured.
Rather than skip the job, disable the sanitizer on Windows only
(--sanitizer none) and keep coverage-guided fuzzing there; our
targets assert via plain Rust panics so they don't depend on ASan.

* test: assert exact char_idx correctness in ansi_strip fuzz target

Replace the bounds-only char_idx check with an exact-equality check
against the char position of byte_pos in the original string. This
subsumes (and is stronger than) the monotonicity CodeRabbit flagged,
since strictly-increasing byte positions on char boundaries always
imply strictly-increasing char positions.

* ci: skip windows in fuzz job, scope job permissions

CI showed the Windows fuzz build fails with a real MSVC linker error
(LNK2001: unresolved __start/__stop___sancov_pcs) even with
--sanitizer none: MSVC's linker doesn't synthesize the section
boundary symbols that libFuzzer's coverage instrumentation requires,
so this is unrelated to the earlier ASan/PATH discussion and isn't
fixable by a sanitizer flag. Skip Windows via step-level `if`
(job-level `if` can't reference runner/matrix contexts). Also add an
explicit contents:read permissions block to the job.

* fix(fzy): fix unicode case-folding inconsistency causing overflow panic

The new fuzzy_match fuzz target found a real crash: FzyMatcher panicked
with "attempt to multiply with overflow" on choice="ű\0\0\0\u{1e}ű",
pattern="Űű".

Root cause: fzy_score's case-insensitive comparison used
char::to_ascii_lowercase (a no-op on non-ASCII letters like Ű/ű), while
the shared cheap_matches() prefilter (and the other matchers) use the
Unicode-aware char_equal(). This let cheap_matches accept a pattern
that fzy_score's own DP could then never actually align, since needle
char 'Ű' never matched any haystack position under ASCII-only folding.
The DP's SCORE_MIN sentinel ("impossible") isn't an absorbing element
under plain integer addition, so the broken alignment accumulated to a
value close to, but not exactly, SCORE_MIN, which then overflowed on
the final *SCORE_TO_SKIM conversion since only the exact sentinel was
special-cased.

Fix is_match to use the shared char_equal() so fzy.rs's case folding
matches cheap_matches and the other two matchers (skim.rs, clangd.rs
already do this). Also switch internal_to_skim_score to saturating_mul
as defense in depth, since fzy_score structurally always returns
Some(..) and has no other way to signal "no valid alignment" to the
caller.

* ci: try lld-link to get windows fuzzing working (no ASan)

MSVC ASan is documented broken on GitHub-hosted Windows runners
(actions/runner-images#8891 — ASan binaries crash with
STATUS_DLL_INIT_FAILED even with the runtime DLL on PATH, unresolved
upstream), so it's not viable here regardless of our config. Separately,
the sancov coverage instrumentation cargo-fuzz needs doesn't link with
MSVC's link.exe at all (missing __start/__stop section symbols).
Try switching the Windows leg to rustc's bundled LLD linker
(-C linker-flavor=lld-link -C link-self-contained=+linker) with
--sanitizer none, to at least get coverage-guided fuzzing (no ASan)
working there. Validating live against this PR's CI.

* ci: revert windows fuzzing attempt, exclude it again

The lld-link experiment ruled out the remaining option: LLD's COFF
driver hit the exact same missing __start/__stop___sancov_* symbols as
MSVC's link.exe. This confirms the section-boundary-symbol synthesis
libFuzzer's coverage instrumentation needs simply isn't implemented
for the COFF/Windows target in current LLVM/rustc — an upstream gap,
not a linker choice or CI config problem. Combined with MSVC ASan
being separately documented broken on GH-hosted Windows runners
(actions/runner-images#8891), there's no remaining avenue to try from
the workflow side. Back to excluding Windows from the fuzz job.

* ci: try windows fuzzing with default sanitizer + msvc dev env

Previous Windows attempts both used --sanitizer none, which removes
the ASan runtime that (on Windows) supplies the __start/__stop section
symbol shims libFuzzer's coverage instrumentation needs -- neither
linker synthesizes those on COFF. That's very likely why they failed
to link. Revert to the default sanitizer (address) and add
ilammy/msvc-dev-cmd to put the MSVC ASan DLL directory on PATH, per
the cargo-fuzz Windows setup guide and actions/runner-images#8891.
Testing live whether this builds, and whether the previously-reported
STATUS_DLL_INIT_FAILED runtime crash still reproduces on this runner
image.

* ci: point cargo at the real MSVC linker on windows

msvc-dev-cmd correctly set up Path, but Git Bash prepends its own
usr/bin ahead of it, so cargo picked up Git's coreutils `link`
(hardlink tool) instead of MSVC's link.exe. Set
CARGO_TARGET_X86_64_PC_WINDOWS_MSVC_LINKER explicitly using
VCToolsInstallDir (set by msvc-dev-cmd) to sidestep PATH ordering
entirely.

* fix(event): parse_action returns None instead of panicking on missing args

The keymap_parse fuzz target found a real crash: KeyMap::from("/:if-")
panicked ("no arg specified for event if-") since parse_action's
documented behavior was to panic on if-* actions missing their
argument, even though the function already returns Option<Action> and
every other malformed/unrecognized action already resolves to None
via the surrounding parse_action_chain/KeyMap plumbing.

Fixed that case, and while checking for the same pattern elsewhere in
the function found four more reachable panics of the same kind
(add-char, execute, execute-silent, set-preview-cmd, set-query parsed
without their required argument), confirmed each panics via a small
repro before fixing. All now return None like every other malformed
action, consistent with the function's existing contract, instead of
panicking on user-supplied --bind strings.

* ci: add a single aggregate status check for branch rulesets

Add a ci-success job that depends on every other job in the workflow
and fails if any of them failed or were cancelled (tolerating
deploy-coverage-page's expected skip off master). This gives branch
protection / repository rulesets one stable check name to require,
instead of enumerating every matrix leg (nextest (linux), fuzz
(windows), etc.) individually.

---------

Co-authored-by: Claude <noreply@anthropic.com>

2026-07-04 18:01:46 +02:00

2.2 KiB

Raw Permalink Blame History

Fuzzing

This directory contains cargo-fuzz (libFuzzer) targets for skim's hand-written, untrusted-input-facing parsers: text that flows in from stdin, --ansi sequences, --nth/--with-nth field specs, the search query syntax, and --bind key maps. These are exactly the places where skim does manual byte/char-index bookkeeping on attacker- or data-controlled strings, which is the most panic-prone code in the project.

Targets

Target	Exercises
`ansi_strip`	`helper::item::strip_ansi` — ANSI escape stripping & byte/char index map
`field_extract`	`field::{FieldRange, get_string_by_field, parse_matching_fields, parse_transform_fields}` — `--nth`/`--with-nth`
`fuzzy_match`	`fuzzy_matcher::{skim, fzy, clangd}` — the fuzzy matching algorithms
`query_match`	`Matcher::create_engine_factory` + `DefaultSkimItem` — the full query → engine → match pipeline (exact/regex/AND-OR/fuzzy, with ANSI)
`keymap_parse`	`binds::KeyMap` — the `--bind` key-map parser

Each target asserts more than "doesn't panic" where a cheap invariant is available (e.g. reported match indices must be valid char indices into the matched text, index mappings must stay monotonic and land on char boundaries).

Running

Install cargo-fuzz (requires a nightly toolchain):

cargo install cargo-fuzz

Run a target:

cargo +nightly fuzz run ansi_strip

Run for a bounded time (useful in CI or for a quick check):

cargo +nightly fuzz run query_match -- -max_total_time=60

Reproducing a crash

cargo fuzz run writes failing inputs to fuzz/artifacts/<target>/. Replay one with:

cargo +nightly fuzz run <target> fuzz/artifacts/<target>/crash-<hash>

Adding a target

Add a new fuzz_targets/<name>.rs, register it in fuzz/Cargo.toml's [[bin]] list, and prefer asserting a real invariant of the function under test (bounds, monotonicity, round-tripping) rather than only catching panics.

2.2 KiB Raw Permalink Blame History