fix(jq): reject a raw control character in a JSON string, matching jq - #2959
Conversation
Real jq refuses an unescaped U+0000-U+001F inside a string on every
input path; succinctly accepted one on the primary document path, on
`-s`, and on `--slurpfile`, where it was additionally re-escaped to
`\\t` on the way out, turning a malformed document into a well-formed
one with no diagnostic.
All three symptoms came from one site: `jq_runner.rs`'s `find_string_end`
scanned for `"` and `\\` and treated every other byte as content. Replace
it with `json::light::string_literal_end`, a shared scanner beside
`number_literal_end` and for the same stated reason (one validated
implementation instead of independently-maintained copies), reached from
`scan_one_json_token` and `find_matching_close` alike so top-level
strings, nested values and object keys all inherit the rule.
It lives in the library because `crate::util` is `pub(crate)`: the scan
delegates to `util::simd::escape::find_json_escape`, whose predicate
(`"`, `\\`, or `< 0x20`) is already exactly this three-way question, at
16-32 bytes per iteration rather than the byte-at-a-time loop it
replaces.
0x7F is deliberately still accepted: jq's own check compares a signed
char, so DEL passes it. A full 0x00-0x7F sweep against jq 1.7.1 confirms
that split with no per-byte exceptions.
This does not make the splitter a validator -- `{invalid}` stays
accepted, the documented cost-driven divergence. The control-character
rule is affordable precisely because it rides a scan that already
inspects every byte of every string.
Refs #2878
`fromjson` decodes through `eval.rs`'s own parser, never the document splitter, so it needed its own arm of the same rule. Real jq rejects a raw U+0000-U+001F there too. Gated on yq mode, because real yq *accepts* it and answers ["a\\tb"] (confirmed live against yq v4.53.3) -- the mode decides, not the format (ADR-0018). Same per-mode split precedent as the surrogate arms directly above it (#2008/#2013). `tonumber` shares that parser, and its "valid JSON but not a number" vs "not valid JSON at all" probe picks between two error messages. That probe was mode-blind on purpose, because the only split it could see (#2008's surrogates) errors in both modes and so could never change its verdict. This rule can: each oracle classifies a control-character string its own way -- jq 1.7.1: Invalid string: control characters ... must be escaped yq 4.53.3: cannot convert node value [...] of tag !!str to number -- which are this crate's `invalid_numeric_literal` and `cannot_parse_as_number` families respectively. Leaving the probe hard-coded to jq's mode was measured to flip yq's message to the wrong family, so thread the real mode through it, in both evaluators. Refs #2878
Sweeps the whole 0x00-0x1F range plus 0x7F across the primary document path, `-s` and `--slurpfile`, rather than spot-checking TAB: 0x7F is the byte that proves which rule is implemented, since jq's signed-char check accepts DEL and "reject anything non-printable" would be the wrong generalisation. A hand-picked matrix cannot separate those two. Also covers the sites the top-level arm does not reach -- an object key and a value nested in a container, both of which arrive through `find_matching_close` -- and a two-value document, so the per-value loop has to keep checking after a clean first value. The escaped spellings are the second negative control: without them a fix that rejected every tab, raw or escaped, would pass every other assertion here. The yq-mode counter-tests keep a future consistency sweep from importing jq's stricter rule into yq, and pin `tonumber`'s message family on both sides of the mode split. Refs #2878
…validation section The section states 'a filter validates only what it materializes'. #2878 adds a document-wide rejection, so say explicitly why that is not an exception to it: the rule governs the filter, never the document splitter, which already rejected an unterminated string or container ahead of any filter. Also records the deliberate asymmetry it leaves -- a raw control character is caught document-wide, a malformed member still is not -- and that 0x7F sits on the accepting side of the line in both tools. Refs #2878
Four findings, all confirmed live against the pinned oracles first. 1. Threading yq mode into tonumber's message-family probe flipped the *lone low surrogate* case: only yq's reader rejects a lone surrogate (#2008) and only jq's rejects a raw control character (#2878), so the two sit at opposite ends of the mode split and no single flag gets both right. The premise of the comment I wrote was simply wrong. The probe itself is the bug in yq mode. Real yq draws no valid-JSON-vs-not distinction at all: [1,2], abc, 1 2, "", a lone \udc00 and a raw control character all answer "cannot convert node value [...] of tag !!str to number" (7/7, captured live from v4.53.3). So yq mode no longer runs the probe -- it answers with its single family unconditionally, which matches the oracle in every case above and is less code than the plumbing it replaces. Incidentally fixes the bare-word, trailing-content and empty-string cases, which took jq's family on main. 2. A document whose later value holds the control character is rejected whole, where jq prints the clean prefix first. The exit code agrees, the streamed prefix does not. That is the splitter's pre-existing all-or-nothing shape -- a truncated later value behaves identically on main -- so it is recorded rather than changed, and the test now asserts stdout instead of only the exit code, since an exit-code-only assertion cannot tell the two apart. 3. The new fromjson message is an internal signal, never output: both parse_complete_json callers discard it. Say so, so a future reader does not believe this fix widened jq's per-reason wording. 4. scan_one_json_token's "'Ends' is structural, not a validity verdict" contract is what --seq is documented against, and the new string arm made it false. Restate it with both deliberate exceptions named. Refs #2878
`core::str::from_utf8(doc).unwrap()` sat in the message argument of a
passing assertion, so it was an executable line that only ever runs when
the assertion fails -- the one uncovered added line in this branch, and
uncoverable by construction. Inline `{doc:?}` says the same thing with
no call, and removes a panic path from a test besides.
Refs #2878
CoverageTotal: 93.52% ⚪ 0 pp vs Comparing
🔇 0 ignored region(s), 90 tolerated region(s)
Patch coveragePatch: 100% (64/64 new lines covered)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
CoverageTotal: 93.43% ⚪ 0 pp vs Comparing
🔇 0 ignored region(s), 91 tolerated region(s)
Patch coveragePatch: 100% (64/64 new lines covered)
Indirect coverage changes🔴 0 lines lost coverage, 🟢 1 lines gained coverage on unchanged code. Indirect changes
|
… change
Routing the document splitter through find_json_escape replaced a
byte-at-a-time scalar loop, and the two architectures disagree in sign.
Measured by this guard on its own runners against the PR's merge-base:
users_keys_unsorted wide_keys_unsorted users_identity
ARM64 -8.2% -1.9% -1.3%
x86_64 +2.2% +0.7% +0.4%
NEON wins the scan; x86_64's movemask path costs slightly more than the
scalar loop it replaced on short strings, which the users fixture is made
of. users_keys_unsorted is the smallest query in the matrix, so it swings
furthest in both directions. Only the ARM64 number clears the 5% default.
The guard is abs(drift), so a faster row needs an override exactly as a
slower one would -- 9d555f3 corrected that same misreading.
To be removed once main has moved past #2878: with --baseline-binary on
every run the checked-in file is never consulted, so once the merge-base
includes this change the row reads ~0% again and the override only blinds
it. Follow-up filed on the issue.
Refs #2878
Summary
Real jq rejects a raw, unescaped
U+0000–U+001Finside a JSON string on every input path.succinctlyaccepted one on the primary document path, on-s, on--slurpfile, and infromjson— and on--slurpfileit silently re-escaped the raw byte to\ton the way out, turning a malformed document into a well-formed one with no diagnostic.The issue's stated root cause is not the one (its triage comment had already corrected this, and I re-confirmed it live).
serde_jsondoes reject the control character; the accept came fromparse_json_stream's #1243 leading-zero fallback rescan, and the primary path never callsparse_json_streamat all. All three CLI symptoms trace to a single site:jq_runner.rs::find_string_end, which scanned for"and\and treated every other byte as content.Three changes:
json::light::string_literal_end— a shared scanner besidenumber_literal_end, for the reason that function's own comment already gives (one validated implementation instead of independently-maintained copies).jq_runner.rsnow delegates from bothscan_one_json_tokenandfind_matching_close, so top-level strings, nested values and object keys inherit the rule from one definition. It lives in the library becausecrate::utilispub(crate).util::simd::escape::find_json_escapesearches for exactly",\and< 0x20— precisely this three-way question — at 16–32 bytes per iteration, replacing a byte-at-a-time scalar loop on the hottest input route.fromjsonis gated on jq mode (eval.rs::parse_json_string_value). Real yq accepts the same input and answers["a\tb"](confirmed live against yq v4.53.3), so the mode decides, not the format (ADR-0018) — the same per-mode precedent as the surrogate arms directly above it (jq: an unpaired LOW surrogate escape is echoed verbatim where jq 1.7.1 substitutes U+FFFD #2008/jq: fromjson accepts a lone high surrogate that real jq rejects (pre-existing, found during #2008 review) #2013).The part that isn't in the plan:
tonumberhad to become mode-sensitivetonumbershares that parser, and its "valid JSON but not a number" vs "not valid JSON at all" probe picks between two error messages. It was mode-blind on purpose — the only split it could previously see (#2008's surrogates) errors in both modes, so it could never change the verdict. This rule can, and each oracle classifies the same string its own way:["a<TAB>b"]|tonumberInvalid string: control characters … must be escaped→ the parse-error familycannot convert node value […] of tag !!str to number→ the tag-conversion familyLeaving that probe hard-coded to jq's mode was measured to flip yq's message to the wrong family, so the real mode is threaded through it — in both evaluators (
eval.rsandeval_generic.rs). The yq counter-test pins it; I confirmed it fails against the hard-coded-falseversion.0x7Fis deliberately still acceptedjq's own check compares a signed char, so DEL passes it. A full
0x00–0x7Fsweep against jq 1.7.1 confirms exactly that split, with no per-byte exceptions — which is why the tests sweep the whole range instead of spot-checking TAB:0x7Fis the byte that separates "jq's actual rule" from the wrong generalisation "reject anything non-printable".Scope held
--seqis unmoved (verified: its stderr is still byte-identical to jq's),--argjson/--jsonargs(#2052) and--validatewere already correct, andyq -p jsonis untouched.{invalid}stays accepted — the documented, cost-driven divergence — anddocs/compliance/jq/limitations.mdnow records why the new rule is not an exception to "a filter validates only what it materializes": that rule governs the filter, never the document splitter, which already rejected[1,2,and"unterminatedahead of any filter. This change is the stricter, jq-matching direction, so it recovers fidelity in that table.Performance — measured, and the two architectures disagree in sign
I deliberately posted no local wall-clock number (this box is a laptop running many
concurrent sessions, which
docs/guides/benchmarking.mdrules out). CI's Perf RegressionGuard is the authoritative measurement here:
valgrind --tool=cachegrindinstructioncounts against this PR's own merge-base (#1582), deterministic and runner-noise-immune.
users_keys_unsortedwide_keys_unsortedusers_identityEvery ARM64 row moved negative; every x86_64 row moved positive — consistent in sign
within each arch, so this is a real property of the scanner on short-string input, not
one noisy row. NEON wins the scan; x86_64's movemask path lands slightly above the scalar
loop it replaced.
users_keys_unsortedis the smallest query in the matrix, so it swingsfurthest both ways. This is the same short-input effect O3 (#87) documented for this
scanner family.
Only ARM64's −8.2% clears the 5% default, so this PR adds a documented
QUERY_THRESHOLDSoverride for that row, following 9d555f3's precedent — the guard isabs(drift), so a faster row needs an override exactly as a slower one would, and thatcommit explicitly corrected the "wrong direction" misreading. The override carries its
own removal note, and #2963 tracks both removing it once
main's merge-base includesthis change and the x86_64 short-input tuning.
Test plan
cargo build --features clicargo test --features cli,simd,regex,serde— 8,606 tests / 63 binaries, 0 failurescargo clippy --all-targets --all-features -- -D warningscargo clippy --all-targets --features std,simd,serde,cli,regex,bench-runner,large-tests,mmap-tests -- -D warningscargo fmt --check0x00–0x1F+0x7Fsweep against/usr/bin/jq1.7.1 across the primary path,-sand--slurpfile— 0 mismatches in 33×3 comparisons;fromjson/tonumberchecked against bothjq1.7.1 andyqv4.53.3 in their own modesfromjsongate → jq-mode test; mode plumbing → yq counter-test)--seqstderr confirmed byte-identical to jq'sFixes #2878