fix(common): return an error on malformed JSON in jq::parse_file - #661
Conversation
The JSON branch of parse_file did `parse_single(&data).unwrap()`, panicking on malformed or empty JSON — while the sibling TOML branch maps the error to JqError. A fuzzer hit it immediately (empty input). Map the JSON parse error the same way, so build-time reads of project `.json` data files fail cleanly instead of crashing the process. Regression tests cover malformed, empty, and valid input. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
…uzzing guide (#1106) * test: add cargo-fuzz harnesses for the untrusted-decode boundary Adds eight fuzz targets over the code that turns attacker-influenceable bytes into in-memory structures — the sharpest trust boundary in the tree: graph_from_bytes Graph::from_bytes (remote-execution wire format) graph_roundtrip structure-aware from_bytes(to_bytes(g)) == g remote_index_from_reader IndexFile::from_reader (index.shisha) [Linux only] spec_hash_from_hex SpecHash::from_hex target_from_str Target::from_str mfile_from_toml minimal.toml through the custom serde visitors arg_schema_parse ArgSchema::try_from jq_parse_json jq::parse_file, JSON branch Each fuzz/ dir is its own workspace so the nightly + sanitizer build cannot perturb the main one. Three small production hooks are needed: * graph: `Graph::fuzz_roundtrip` behind a new off-by-default `fuzzing` feature (graphs are not hand-constructible from outside the crate), and `insert_build` gated on `any(test, feature = "fuzzing")` rather than test alone. Default builds are unchanged. * mfile: `File::from_toml_bytes`, the pure filesystem-free core of `from_dir`, so the harness need not re-implement it. * workspace: `arbitrary`, optional and only enabled by graph's `fuzzing`. Also carries the graph corpus seeds; a valid graph-with-local-file seed is what let the fuzzer reach the local-file frame decoder. This is the tooling behind the six decoder fixes already merged in #651, #653, #656, #661 and #693. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(justfile): add fuzz-check bitrot guard and a fuzz runner Each fuzz/ dir declares its own [workspace], so no workspace-wide build ever compiles the fuzz targets and they rot silently as the crates they fuzz evolve. The first version of this suite went 253 commits before anyone rebuilt it, by which point a target referenced a renamed type. `just fuzz-check` is the guard: a plain `cargo check` over every fuzz workspace. No nightly, no sanitizer, no libFuzzer runtime — just "does this still compile against today's API", so it runs anywhere and is cheap enough to treat like a red build. `just fuzz <crate> <target> [args]` runs one target, applying the RSS cap that turns an unbounded-allocation bug into a catchable crash rather than an ambient OOM. The rcache target is excluded on macOS (rcache -> lcache -> the Linux-only common::renameat2), following the existing `scope` idiom. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: add the fuzzing guide Covers prerequisites, the eight targets and their trust levels, running via the just recipes, corpus seeding (and why seeding is what unlocks the deep decode paths), reproducing and minimizing a crash, keeping targets from bitrotting, the bugs the campaign found, and where to take it next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
The JSON branch of
jq::parse_filedidparse_single(&data).unwrap(), panicking on malformed or empty JSON — even though the sibling TOML branch right above it maps its parse error toJqError.parse_fileis called during config decode on project-supplied.jsondata files (SUPPLY trust,decode::stacks), so a broken data file crashed the process instead of surfacing an error.How it was found
The new
jq_parse_jsonfuzz target (in the fuzzing-infrastructure branch) hit it immediately — the crashing input is empty:Fix
Map the JSON parse error into
JqErrorexactly like the TOML branch does. Regression tests cover empty, malformed, and valid JSON.Validation
cargo test -p common jq— passes (malformed/empty →Err, valid →Ok);cargo clippy -p common --lib -- -D warningsclean.This is the sixth and final bug from the decoder/parser fuzzing campaign — the capstone. The others: tar traversal (#651), two graph-wire OOMs (#653), a graph-wire slice panic + filename traversal (#656).
🤖 Generated with Claude Code