From c010d2dfced6aca913d0d1200145f473cabb12a5 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 16:52:56 +1000 Subject: [PATCH 1/7] fix(jq): reject a raw control character in the document splitter 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 --- src/bin/succinctly/jq_runner.rs | 28 +++---- src/json/light.rs | 137 ++++++++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 17 deletions(-) diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 862f7ecde..d2feff427 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -3284,8 +3284,13 @@ fn scan_one_json_token(bytes: &[u8], pos: usize) -> Option { match bytes[pos] { // Object or array - find matching close b'{' | b'[' => find_matching_close(bytes, pos), - // String - find end quote - b'"' => find_string_end(bytes, pos), + // String. `string_literal_end` (shared with `light.rs`, same + // one-validated-implementation reasoning as `number_literal_end` + // below) both finds the end of and validates the token: it rejects + // a raw, unescaped control character `U+0000`-`U+001F` outright, + // matching real jq on every input path, while still accepting + // `0x7F` the way jq's own signed-char check does (#2878). + b'"' => succinctly::json::light::string_literal_end(bytes, pos), // true, false, null b't' | b'f' | b'n' => find_literal_end(bytes, pos), // Number. `number_literal_end` (shared with `light.rs`'s own @@ -3312,8 +3317,10 @@ fn find_matching_close(bytes: &[u8], pos: usize) -> Option { while i < bytes.len() && depth > 0 { match bytes[i] { b'"' => { - // Skip string - let end = find_string_end(bytes, i)?; + // Skip string. Same scanner as the top-level arm, so a raw + // control character is rejected just as readily nested + // inside a container -- in a key as well as a value (#2878). + let end = succinctly::json::light::string_literal_end(bytes, i)?; i = end; continue; } @@ -3331,19 +3338,6 @@ fn find_matching_close(bytes: &[u8], pos: usize) -> Option { } } -/// Find the end of a string starting at `pos` (which points to opening quote). -fn find_string_end(bytes: &[u8], pos: usize) -> Option { - let mut i = pos + 1; - while i < bytes.len() { - match bytes[i] { - b'"' => return Some(i + 1), - b'\\' => i += 2, // Skip escaped character - _ => i += 1, - } - } - None -} - /// Find the end of a literal (true, false, null) starting at `pos`. fn find_literal_end(bytes: &[u8], pos: usize) -> Option { let mut i = pos; diff --git a/src/json/light.rs b/src/json/light.rs index 5735fe35b..7ef6f28ad 100644 --- a/src/json/light.rs +++ b/src/json/light.rs @@ -2062,6 +2062,60 @@ pub fn number_literal_end(text: &[u8], start: usize) -> Option { Some(i) } +/// Where the JSON string literal starting at `start` ends. +/// +/// `start` must index the opening `"`; the returned index is one past the +/// closing quote. `None` means the string is unterminated **or contains a +/// raw, unescaped control character** (`U+0000`-`U+001F`). +/// +/// Rejecting the control character here is what makes the CLI's document +/// splitter match real jq, which refuses `["ab"]` on every input path +/// with `Invalid string: control characters from U+0000 through U+001F must +/// be escaped` (jq 1.7.1). `0x7F` (DEL) is deliberately *accepted*: jq's own +/// check compares a signed char, so only `0x00`-`0x1F` fail it -- a full +/// `0x00`-`0x7F` sweep against jq 1.7.1 confirms exactly that split, with no +/// per-byte exceptions to model (#2878). +/// +/// Companion to [`number_literal_end`], and used by the same **top-level +/// document splitting** caller (`find_json_values` / +/// `scan_one_json_token` / `find_matching_close`, in +/// `src/bin/succinctly/jq_runner.rs`), for the same reason: one validated +/// implementation instead of independently-maintained copies. It lives in +/// the library rather than beside its caller because `crate::util` is +/// `pub(crate)`, so the SIMD scanner below is not reachable from the binary. +/// +/// This does **not** make the splitter a validator. "Ends" stays structural +/// everywhere else -- `{"a":1 xyz}` still scans as one token and `{invalid}` +/// is still accepted document-wide (a deliberate, cost-driven divergence +/// recorded in `docs/compliance/jq/limitations.md`). The control-character +/// rule is affordable precisely because it rides a scan that already +/// inspects every byte of every string, so it costs no extra pass. +/// +/// The scan delegates to [`crate::util::simd::escape::find_json_escape`], +/// whose predicate (`"`, `\`, or `< 0x20`) is already exactly this +/// function's three-way question, at 16-32 bytes per iteration instead of +/// the byte-at-a-time loop this replaced. +pub fn string_literal_end(bytes: &[u8], start: usize) -> Option { + debug_assert_eq!(bytes.get(start), Some(&b'"')); + let mut i = start + 1; + loop { + // `find_json_escape` returns `bytes.len()` when it finds nothing and + // when `i` is already past the end, so the `\` -at-EOF case below + // lands here as "unterminated" without a separate bounds check. + let hit = crate::util::simd::escape::find_json_escape(bytes, i); + match bytes.get(hit) { + Some(b'"') => return Some(hit + 1), + // Skip the escaped byte. A `\` as the final byte pushes `i` past + // the end, which the fast-exit above turns into `None`. + Some(b'\\') => i = hit + 2, + // A raw control character: the whole string is rejected. + Some(_) => return None, + // Ran off the end without a closing quote. + None => return None, + } + } +} + /// Find the end of a number-*shaped* span starting at `start` in `text` /// (a byte that begins a candidate number: `-`, an ASCII digit, or a /// leading `.`), for a value reached while materializing an @@ -6243,6 +6297,89 @@ mod tests { assert_eq!(number_literal_end(b"5e1", 0), Some(3)); } + /// `string_literal_end` rejects every raw `U+0000`-`U+001F` byte and + /// accepts `0x7F`, which is the exact split real jq draws (its check is + /// on a signed char, so DEL passes). Sweeping the whole range rather + /// than spot-checking TAB is deliberate: a hand-picked matrix would not + /// catch an off-by-one at either boundary, and `0x20`/`0x7F` are the two + /// bytes that prove the rule is "< 0x20" and not "non-printable" (#2878). + #[test] + fn test_string_literal_end_rejects_only_raw_c0_controls_2878() { + for byte in 0u8..=0x7F { + let mut doc = Vec::from(*b"\"a"); + doc.push(byte); + doc.extend_from_slice(b"b\""); + let got = string_literal_end(&doc, 0); + if byte < 0x20 { + assert_eq!(got, None, "raw control byte 0x{byte:02X} must be rejected"); + } else if byte == b'"' || byte == b'\\' { + // Not a control character: these two end or escape the + // string rather than sitting in it, so they are not part of + // this rule and are covered by the tests below. + continue; + } else { + assert_eq!( + got, + Some(doc.len()), + "byte 0x{byte:02X} is not a C0 control and must be accepted" + ); + } + } + } + + /// The escaped spelling of the same character stays valid -- the rule is + /// about *raw* bytes, so `\\t` (two bytes: backslash, `t`) must still + /// scan, or the fix would break every well-formed document (#2878). + #[test] + fn test_string_literal_end_accepts_escaped_control_characters_2878() { + // Each of these is one complete string literal, so the answer is + // always the whole slice -- spelled `.len()` rather than a counted + // constant so the assertion cannot drift from the literal above it. + for doc in [ + &br#""a\tb""#[..], + &br#""a\u0009b""#[..], + // An escaped quote does not end the string. + &br#""a\"b""#[..], + ] { + assert_eq!( + string_literal_end(doc, 0), + Some(doc.len()), + "{:?} is one whole string literal", + core::str::from_utf8(doc).unwrap() + ); + } + } + + /// Unterminated strings still return `None` -- the pre-existing contract + /// this function inherited from the scalar loop it replaced. The final + /// case pushes the escape-skip past the end of the buffer, which the + /// SIMD scanner's own past-the-end fast exit has to turn into `None` + /// rather than panicking on an out-of-bounds index (#2878). + #[test] + fn test_string_literal_end_rejects_unterminated_2878() { + assert_eq!(string_literal_end(b"\"abc", 0), None); + assert_eq!(string_literal_end(b"\"", 0), None); + assert_eq!(string_literal_end(br#""abc\"#, 0), None); + } + + /// The scan starts at `start`, not at zero, and a control character + /// *before* the string it is asked about is none of its business -- + /// pinning that `find_json_escape`'s `start` argument is threaded + /// through rather than dropped (#2878). + #[test] + fn test_string_literal_end_honours_its_start_offset_2878() { + // The first string holds a raw TAB, so scanning *it* must fail -- + // and scanning from the *second* string must not notice it at all. + // `start` always indexes an opening quote, never the `[` before it. + let doc = b"[\"a\tb\", \"ok\"]"; + assert_eq!(string_literal_end(doc, 1), None); + let second = doc + .windows(4) + .position(|w| w == b"\"ok\"") + .expect("second literal present"); + assert_eq!(string_literal_end(doc, second), Some(second + 4)); + } + /// Companion to the test above: `nested_number_span` -- unlike /// `number_literal_end` -- absorbs the same dangling exponent marker /// into one span rather than rejecting it, by design (#966, #1218). From 168d6a326d66be854893dda2aa317fb668ace3ac Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 16:53:05 +1000 Subject: [PATCH 2/7] fix(jq): reject a raw control character in fromjson, jq mode only `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 --- src/jq/eval.rs | 43 +++++++++++++++++++++++++++++++++--------- src/jq/eval_generic.rs | 2 +- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/src/jq/eval.rs b/src/jq/eval.rs index df32e7cab..36fda1b49 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -10366,7 +10366,7 @@ fn eval_builtin<'a, W: Clone + AsRef<[u64]>, S: EvalSemantics>( // Phase 6: Type Conversions Builtin::ToString => builtin_tostring::(value, optional), - Builtin::ToNumber => builtin_tonumber::(value, optional), + Builtin::ToNumber => builtin_tonumber::(value, optional), Builtin::ToJson => builtin_tojson::(value, optional), Builtin::FromJson => builtin_fromjson::(value, optional), @@ -15646,7 +15646,7 @@ fn builtin_tostring, S: EvalSemantics>( } /// Builtin: tonumber - convert string to number -fn builtin_tonumber>( +fn builtin_tonumber, S: EvalSemantics>( value: StandardJson<'_, W>, optional: bool, ) -> QueryResult<'_, W> { @@ -15658,7 +15658,7 @@ fn builtin_tonumber>( QueryResult::Owned(OwnedValue::from_number_bytes(n.raw_bytes())) } StandardJson::String(s) => match s.as_str() { - Ok(cow) => match tonumber_from_str(cow.as_ref()) { + Ok(cow) => match tonumber_from_str(cow.as_ref(), S::TAG == EvalTag::Yq) { Ok(n) => QueryResult::Owned(n), Err(_) if optional => QueryResult::None, Err(e) => e.into(), @@ -15681,7 +15681,7 @@ fn builtin_tonumber>( /// Kept in one place because the two evaluators previously had *different* /// wording here — "cannot parse 'a' as number" against "cannot convert 'a' to /// number" — which is exactly the drift #356 is about. -pub(super) fn tonumber_from_str(s: &str) -> Result { +pub(super) fn tonumber_from_str(s: &str, yq_mode: bool) -> Result { // jq's JSON parser skips surrounding whitespace, so `" 1 "` is 1. let trimmed = s.trim(); // Materialize a JSON-shaped number as a `NumberLiteral` so it keeps its @@ -15747,11 +15747,20 @@ pub(super) fn tonumber_from_str(s: &str) -> Result { if let Ok(f) = trimmed.parse::() { return Ok(OwnedValue::Float(f)); } - // Mode-blind on purpose: this call only distinguishes "valid JSON but - // not a number" from "not valid JSON at all" for a nicer error message, - // never returns the parsed value, so `fromjson`'s jq/yq surrogate-mode - // split (#2008) has nothing to plumb through here. - if parse_complete_json(trimmed, false).is_ok() { + // Mode-*sensitive*, despite only picking between two error messages and + // never returning the parsed value. It was mode-blind while the only + // jq/yq split in `parse_complete_json` was `fromjson`'s surrogate one + // (#2008), which errors in both modes and so could never change this + // verdict. #2878's control-character rule can: a string like + // `["ab"]` is "valid JSON" to yq and "not valid JSON at all" to jq, + // and each oracle's own wording follows its own answer -- + // 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` branches respectively. Passing `false` here + // regardless would hand yq mode jq's answer (verified: it flips yq's + // message to the wrong family). + if parse_complete_json(trimmed, yq_mode).is_ok() { Err(EvalError::cannot_parse_as_number(&OwnedValue::String( s.to_string(), ))) @@ -16131,6 +16140,22 @@ fn parse_json_string_value( } *pos += 1; } + c if c < 0x20 && !yq_mode => { + // #2878: a raw, unescaped control character is rejected by + // real jq everywhere, `fromjson` included: + // jq -nc '"[\"a\tb\"]" | fromjson' + // -> Invalid string: control characters from U+0000 + // through U+001F must be escaped + // Gated on `yq_mode` because real yq *accepts* it here 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 above + // (#2008/#2013). `0x7F` is deliberately not covered: jq's + // own check is on a signed char, so DEL passes it. + return Err(format!( + "Invalid string: control character 0x{c:02X} from U+0000 through U+001F must be escaped" + )); + } c => { // Regular character - handle UTF-8 let remaining = &bytes[*pos..]; diff --git a/src/jq/eval_generic.rs b/src/jq/eval_generic.rs index 4bf9ca3e1..bcd1df3d0 100644 --- a/src/jq/eval_generic.rs +++ b/src/jq/eval_generic.rs @@ -20987,7 +20987,7 @@ fn eval_builtin( // -- see `OwnedValue::from_document_float`. GenericResult::Owned(OwnedValue::from_document_float(f)) } else if let Some(s) = value.as_str() { - match tonumber_from_str(s.as_ref()) { + match tonumber_from_str(s.as_ref(), S::TAG == EvalTag::Yq) { Ok(n) => GenericResult::Owned(n), Err(_) if optional => GenericResult::None, Err(e) => GenericResult::Error(e), From d969e5dd16220f68e15db61002bb4abb86576990 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 16:53:12 +1000 Subject: [PATCH 3/7] test(jq): pin the control-character rule and its two negative controls 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 --- tests/jq_cli_tests.rs | 157 ++++++++++++++++++++++++++++++++++++++++++ tests/yq_cli_tests.rs | 45 ++++++++++++ 2 files changed, 202 insertions(+) diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 7970ef095..1f1b1859c 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -54344,3 +54344,160 @@ fn test_bare_recurse_untracked_seed_and_catch_2761() -> Result<()> { assert_eq!(alias, recurse); Ok(()) } + +// ---- #2878: a raw control character is not valid JSON on any input path ---- + +/// Writes `["ab"]` to a temp file and returns it. +/// +/// A file, not stdin, because the primary document path is the one under +/// test and `--slurpfile` can only take a path anyway -- so every arm of the +/// matrix below can share one fixture. +fn ctl_char_doc(byte: u8) -> Result { + let mut file = NamedTempFile::new()?; + file.write_all(br#"["a"#)?; + file.write_all(&[byte])?; + file.write_all(br#"b"]"#)?; + file.flush()?; + Ok(file) +} + +/// Real jq rejects a raw, unescaped `U+0000`-`U+001F` inside a string on +/// every input path; `succinctly` used to accept it on the primary document +/// path, `-s`, and `--slurpfile` (#2878). +/// +/// The sweep runs the whole `0x00`-`0x1F` range plus `0x7F`, rather than +/// spot-checking TAB, because `0x7F` is the byte that proves which rule is +/// implemented: jq's own check compares a *signed* char, so DEL is accepted +/// and "reject anything non-printable" would be the wrong generalisation. A +/// hand-picked matrix of a few control bytes cannot tell those two apart. +/// +/// Verdicts captured live from `/usr/bin/jq` 1.7.1: exit 5 for `0x00`-`0x1F`, +/// exit 0 for `0x7F`. +#[test] +fn test_raw_control_character_rejected_on_every_input_path_2878() -> Result<()> { + for byte in (0x00u8..=0x1F).chain(core::iter::once(0x7F)) { + let file = ctl_char_doc(byte)?; + let path = file.path().to_str().expect("temp path is utf-8"); + let jq_rejects = byte < 0x20; + + for args in [ + vec![".", path], + vec!["-s", ".", path], + vec!["-n", "--slurpfile", "x", path, "$x"], + ] { + let (_stdout, _stderr, code) = run_jq_full(&args, None)?; + if jq_rejects { + assert_ne!( + code, 0, + "raw control byte 0x{byte:02X} must be rejected by {args:?}" + ); + } else { + assert_eq!( + code, 0, + "byte 0x{byte:02X} is not a C0 control and must be accepted by {args:?}" + ); + } + } + } + Ok(()) +} + +/// The rule reaches a control character in an object *key* and one nested +/// inside a container, not just a top-level string value. +/// +/// Both arrive through `find_matching_close`'s recursive string skip rather +/// than the top-level `scan_one_json_token` arm, so they are a genuinely +/// different call site -- and a key is a different site again from a value +/// within that (#2878). +#[test] +fn test_raw_control_character_rejected_in_keys_and_nested_values_2878() -> Result<()> { + for body in [ + &b"{\"a\x09b\": 1}"[..], + &b"{\"x\": [1, {\"y\": \"a\x09b\"}]}"[..], + // Two top-level values: the malformed one is the second, so the + // per-value loop has to keep checking after a clean first value. + &b"{\"ok\":1}\n[\"a\x09b\"]\n"[..], + ] { + let mut file = NamedTempFile::new()?; + file.write_all(body)?; + file.flush()?; + let path = file.path().to_str().expect("temp path is utf-8"); + let (_stdout, _stderr, code) = run_jq_full(&[".", path], None)?; + assert_ne!( + code, + 0, + "raw control character must be rejected in {:?}", + String::from_utf8_lossy(body) + ); + } + Ok(()) +} + +/// The negative control for the whole rule: the *escaped* spellings of the +/// same characters are well-formed JSON and must keep working, and so must a +/// raw `0x7F`. Without this, a fix that rejected every tab -- raw or escaped +/// -- would pass every assertion above (#2878). +#[test] +fn test_escaped_control_characters_still_accepted_2878() -> Result<()> { + for body in [ + &b"[\"a\\tb\"]"[..], + &b"[\"a\\u0009b\"]"[..], + &b"[\"a\x7Fb\"]"[..], + &b"{\"a\\tb\": [1, {\"y\": \"\\u001f\"}]}"[..], + ] { + let mut file = NamedTempFile::new()?; + file.write_all(body)?; + file.flush()?; + let path = file.path().to_str().expect("temp path is utf-8"); + let (_stdout, _stderr, code) = run_jq_full(&["-c", ".", path], None)?; + assert_eq!( + code, + 0, + "escaped control character must stay valid in {:?}", + String::from_utf8_lossy(body) + ); + } + Ok(()) +} + +/// `fromjson` is a second, independent decoder (`eval.rs`), not reached +/// through the document splitter at all -- so it needs its own arm of the +/// same rule. jq rejects; jq mode must now too (#2878). +/// +/// The yq-mode counter-test lives in `yq_cli_tests.rs`: real yq *accepts* +/// this, so the fix is mode-gated rather than format-gated (ADR-0018). +#[test] +fn test_fromjson_rejects_raw_control_character_in_jq_mode_2878() -> Result<()> { + let raw = format!("\"[\\\"a{}b\\\"]\" | fromjson", '\u{9}'); + let (_stdout, _stderr, code) = run_jq_full(&["-nc", &raw], None)?; + assert_ne!(code, 0, "jq mode `fromjson` must reject a raw control char"); + + // DEL as the negative control: jq accepts it here too. + let del = format!("\"[\\\"a{}b\\\"]\" | fromjson", '\u{7f}'); + let (_stdout, _stderr, code) = run_jq_full(&["-nc", &del], None)?; + assert_eq!(code, 0, "jq mode `fromjson` must accept a raw 0x7F"); + + // And the escaped spelling still decodes. + let (stdout, _stderr, code) = run_jq_full(&["-nc", "\"[\\\"a\\\\tb\\\"]\" | fromjson"], None)?; + assert_eq!(code, 0, "escaped tab must still decode"); + assert_eq!(stdout.trim(), "[\"a\\tb\"]"); + Ok(()) +} + +/// `tonumber` shares `fromjson`'s decoder, and its "valid JSON but not a +/// number" vs "not valid JSON at all" probe picks between two *different* +/// error messages. #2878's rule moves a control-character string across that +/// boundary -- in jq mode only, because each oracle classifies it its own way +/// (jq: a parse error; yq: a tag-conversion error). Pins that the probe is +/// mode-sensitive, which it did not need to be before this (#2878). +#[test] +fn test_tonumber_control_character_message_family_is_jq_mode_only_2878() -> Result<()> { + let prog = format!("\"[\\\"a{}b\\\"]\" | tonumber", '\u{9}'); + let (_stdout, stderr, code) = run_jq_full(&["-nc", &prog], None)?; + assert_ne!(code, 0, "tonumber must still fail"); + assert!( + stderr.contains("Invalid numeric literal"), + "jq mode should report the parse-error family, got: {stderr}" + ); + Ok(()) +} diff --git a/tests/yq_cli_tests.rs b/tests/yq_cli_tests.rs index ca4de2cfd..8a63958a8 100644 --- a/tests/yq_cli_tests.rs +++ b/tests/yq_cli_tests.rs @@ -46716,3 +46716,48 @@ fn test_comma_count_argument_extensions_2863() -> Result<()> { } Ok(()) } + +// ---- #2878: yq mode must NOT inherit jq's control-character rule ---- + +/// Real yq *accepts* a raw, unescaped control character inside a string that +/// `fromjson` decodes -- `yq -n '"[\"ab\"]" | fromjson'` answers +/// `["a\tb"]` (confirmed live against yq v4.53.3). jq rejects the same input. +/// +/// So #2878's fix is gated on the *mode*, not the format (ADR-0018), and this +/// is the counter-test that keeps a future "make both modes consistent" sweep +/// from quietly importing jq's stricter rule into yq. The jq-mode half lives +/// in `jq_cli_tests.rs`. +#[test] +fn test_fromjson_accepts_raw_control_character_in_yq_mode_2878() -> Result<()> { + let prog = format!("\"[\\\"a{}b\\\"]\" | fromjson", '\u{9}'); + let (stdout, stderr, code) = run_yq_stdin_with_stderr(&prog, "", &["-n"])?; + assert_eq!( + code, 0, + "yq mode must still accept a raw control char in fromjson; stderr: {stderr}" + ); + assert!( + stdout.contains("a\\tb"), + "yq mode should decode the raw tab, got: {stdout:?}" + ); + Ok(()) +} + +/// `tonumber`'s "valid JSON but not a number" probe is mode-sensitive as of +/// #2878. Real yq reports a tag-conversion error here (`cannot convert node +/// value [...] of tag !!str to number`), which is this crate's +/// `cannot be parsed as a number` family -- *not* jq's parse-error family. +/// +/// Pins the half of #2878 that a jq-only fix would have silently regressed: +/// passing jq's mode into that probe unconditionally flips yq's message to +/// the wrong family. +#[test] +fn test_tonumber_control_character_keeps_yq_message_family_2878() -> Result<()> { + let prog = format!("\"[\\\"a{}b\\\"]\" | tonumber", '\u{9}'); + let (_stdout, stderr, code) = run_yq_stdin_with_stderr(&prog, "", &["-n"])?; + assert_ne!(code, 0, "tonumber must still fail in yq mode"); + assert!( + stderr.contains("cannot be parsed as a number"), + "yq mode should keep the tag-conversion family, got: {stderr}" + ); + Ok(()) +} From ff559aa5793002393fdd15486412d2e779da6903 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 16:54:59 +1000 Subject: [PATCH 4/7] docs(jq): record the control-character rule's place in the streaming-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 --- docs/compliance/jq/limitations.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 7d3a68536..2658da2ed 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -4298,6 +4298,26 @@ The context that makes the loss survivable is the one #2168's entry states: succ already diverges on this whole class of document through `.` itself, deliberately, and a user who wants jq's rejection has `--validate` and `succinctly json validate`. +**A raw control character is the one fault in this class that is caught document-wide +(#2878), and it is not an exception to the rule above.** "A filter validates only what it +materializes" governs the *filter*; it has never described the **document splitter**, which +runs on every input ahead of any filter and already rejected an unterminated string or +container there (`[1,2,` and `"unterminated` both exit 5 under `1+1`, matching jq). #2878 +added an unescaped `U+0000`-`U+001F` inside a string to that same splitter class, so +`1+1` on `["ab"]` now exits 5 as jq does, rather than answering `2`. + +It is affordable where up-front validation is not for the reason that section's cost +argument turns on: `find_json_values` already inspects every byte of every string, so the +rule rides a scan the input pays for anyway — no "second index-building pass". It is also +the *stricter*, jq-matching direction, so it recovers fidelity in this table rather than +spending more of it. + +The resulting asymmetry is real and deliberate: a raw control character is caught +document-wide, while a malformed *member* still is not (`{invalid}` stays at exit 0 under +`1+1`, where jq exits 5 — the row above). The line between them is the same cost-driven one +this section already draws. `0x7F` is on the accepting side of it in both tools, since jq's +own check compares a signed char. + **The materializing flag routes still validate whatever the filter — down to `-s` and `-n`/`input` now, closed by [#2662](https://github.com/rust-works/succinctly/issues/2662) for `-S`/`-a`/`-C`.** `-S` (sort keys), `-a` (ASCII output) and `-C` (color) used to force From 7c21bbc5cf8e338f06a1a155939691054daef73c Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 17:20:27 +1000 Subject: [PATCH 5/7] fix(jq): address /code-review findings on #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 --- docs/compliance/jq/limitations.md | 9 ++++++ src/bin/succinctly/jq_runner.rs | 18 ++++++++--- src/jq/eval.rs | 46 +++++++++++++++++++--------- tests/jq_cli_tests.rs | 45 +++++++++++++++++++++++++-- tests/yq_cli_tests.rs | 51 ++++++++++++++++++++----------- 5 files changed, 131 insertions(+), 38 deletions(-) diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 2658da2ed..e5547ad20 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -4318,6 +4318,15 @@ document-wide, while a malformed *member* still is not (`{invalid}` stays at exi this section already draws. `0x7F` is on the accepting side of it in both tools, since jq's own check compares a signed char. +One consequence is worth naming, because #2878 makes it reachable for a new input class +without introducing it: **a rejected document produces no partial output.** On +`{"ok":1}` followed by `["ab"]`, jq prints `{"ok":1}` and *then* exits 5; succinctly +prints nothing and exits 5. The exit codes agree, the streamed prefix does not. This is the +splitter's own all-or-nothing shape — `find_json_values` resolves every value's span before +any value is emitted — and it predates this rule: `{"ok":1}` followed by a truncated +`[1,2,` behaves identically, and did before #2878. Pinned on stdout (not just the exit +code) by `test_control_character_in_a_later_value_rejects_the_whole_document_2878`. + **The materializing flag routes still validate whatever the filter — down to `-s` and `-n`/`input` now, closed by [#2662](https://github.com/rust-works/succinctly/issues/2662) for `-S`/`-a`/`-C`.** `-S` (sort keys), `-a` (ASCII output) and `-C` (color) used to force diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index d2feff427..b1d10fa53 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -3276,10 +3276,20 @@ fn find_json_values(bytes: &[u8]) -> core::result::Result, u /// (#1723) can ask the identical per-token question without a second copy of /// this dispatch drifting from it. /// -/// "Ends" is structural, not a validity verdict: `{"a":1 xyz}` scans as one -/// token because its braces match, and only validation rejects it. Keeping -/// the two separate is what lets the `--seq` caller reject a whole record -/// without ever reading a value out of the middle of a malformed one. +/// "Ends" is *mostly* structural rather than a validity verdict: +/// `{"a":1 xyz}` scans as one token because its braces match, and only +/// validation rejects it. Keeping the two separate is what lets the `--seq` +/// caller reject a whole record without ever reading a value out of the +/// middle of a malformed one. +/// +/// Two arms are deliberate exceptions, both because the scan that finds the +/// token's end is already the scan that would validate it, so splitting them +/// would cost a second pass for nothing: `number_literal_end` rejects a +/// number-*shaped* span that is not a number (`-e5`, `1e`), and +/// `string_literal_end` rejects a string holding a raw `U+0000`-`U+001F` +/// (#2878). Both were verified not to move `--seq`, whose own validity +/// question (`seq_value_is_valid`) already rejected these inputs -- so the +/// two routes still agree, they just now agree earlier. fn scan_one_json_token(bytes: &[u8], pos: usize) -> Option { match bytes[pos] { // Object or array - find matching close diff --git a/src/jq/eval.rs b/src/jq/eval.rs index 36fda1b49..1e60351f5 100644 --- a/src/jq/eval.rs +++ b/src/jq/eval.rs @@ -15747,20 +15747,29 @@ pub(super) fn tonumber_from_str(s: &str, yq_mode: bool) -> Result() { return Ok(OwnedValue::Float(f)); } - // Mode-*sensitive*, despite only picking between two error messages and - // never returning the parsed value. It was mode-blind while the only - // jq/yq split in `parse_complete_json` was `fromjson`'s surrogate one - // (#2008), which errors in both modes and so could never change this - // verdict. #2878's control-character rule can: a string like - // `["ab"]` is "valid JSON" to yq and "not valid JSON at all" to jq, - // and each oracle's own wording follows its own answer -- - // 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` branches respectively. Passing `false` here - // regardless would hand yq mode jq's answer (verified: it flips yq's - // message to the wrong family). - if parse_complete_json(trimmed, yq_mode).is_ok() { + // The probe below is jq's question, and jq's alone. + // + // It distinguishes "valid JSON but not a number" from "not valid JSON at + // all" purely to pick between two error messages, never to return a + // value. **Real yq draws no such distinction**: every non-numeric string + // gets the one tag-conversion error, whether or not the text is valid + // JSON (yq 4.53.3, captured live -- `[1,2]`, `abc`, `1 2`, `""`, a lone + // `\udc00` and a raw control character all answer `cannot convert node + // value [...] of tag !!str to number`). So in yq mode there is nothing + // for the probe to decide, and asking it anyway is what makes it + // wrong: whichever mode we hand `parse_complete_json`, some input + // crosses the boundary and lands in jq's parse-error family, which yq + // has no equivalent of. `\udc00` does it under yq mode (only yq rejects + // a lone surrogate, #2008) and a raw control character does it under jq + // mode (only jq rejects one, #2878) -- opposite directions, so no single + // flag avoids both. Answering `cannot_parse_as_number` unconditionally + // does, and matches the oracle in every case above. + if yq_mode { + return Err(EvalError::cannot_parse_as_number(&OwnedValue::String( + s.to_string(), + ))); + } + if parse_complete_json(trimmed, false).is_ok() { Err(EvalError::cannot_parse_as_number(&OwnedValue::String( s.to_string(), ))) @@ -16152,6 +16161,15 @@ fn parse_json_string_value( // per-mode split precedent as the surrogate arms above // (#2008/#2013). `0x7F` is deliberately not covered: jq's // own check is on a signed char, so DEL passes it. + // + // This text is an internal signal, not output: both + // `parse_complete_json` callers discard the `Err(String)` + // and render their own diagnostic, so `fromjson` actually + // reports `Invalid numeric literal at EOF ...` here. That + // approximation of jq's per-reason wording is pre-existing + // and recorded in `docs/compliance/jq/limitations.md`; it is + // deliberately not widened by this fix, which is about + // accept-vs-reject. return Err(format!( "Invalid string: control character 0x{c:02X} from U+0000 through U+001F must be escaped" )); diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 1f1b1859c..530aa84b5 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -54414,9 +54414,6 @@ fn test_raw_control_character_rejected_in_keys_and_nested_values_2878() -> Resul for body in [ &b"{\"a\x09b\": 1}"[..], &b"{\"x\": [1, {\"y\": \"a\x09b\"}]}"[..], - // Two top-level values: the malformed one is the second, so the - // per-value loop has to keep checking after a clean first value. - &b"{\"ok\":1}\n[\"a\x09b\"]\n"[..], ] { let mut file = NamedTempFile::new()?; file.write_all(body)?; @@ -54433,6 +54430,48 @@ fn test_raw_control_character_rejected_in_keys_and_nested_values_2878() -> Resul Ok(()) } +/// A document whose *second* value holds the control character is rejected +/// whole: succinctly prints nothing, where jq prints the clean first value +/// and then exits 5. +/// +/// The exit code matches; the partial output does not. That difference is +/// the document splitter's pre-existing all-or-nothing shape, not something +/// #2878 introduced -- `{"ok":1}` followed by a *truncated* `[1,2,` behaves +/// exactly the same way on `main`, and has for as long as the splitter has +/// rejected truncated input. #2878 only routes one more input class into it. +/// +/// Asserted on stdout, not just the exit code, because an exit-code-only +/// assertion would pass whether the clean prefix were printed or not, and +/// which one happens is the whole content of this divergence (#2878 review). +#[test] +fn test_control_character_in_a_later_value_rejects_the_whole_document_2878() -> Result<()> { + let mut file = NamedTempFile::new()?; + file.write_all(b"{\"ok\":1}\n[\"a\x09b\"]\n")?; + file.flush()?; + let path = file.path().to_str().expect("temp path is utf-8"); + let (stdout, _stderr, code) = run_jq_full(&["-c", ".", path], None)?; + assert_ne!(code, 0, "the document must be rejected"); + assert_eq!( + stdout, "", + "succinctly rejects the document whole, printing no partial output" + ); + + // The pre-existing member of the same class, to keep this pinned as + // "the splitter's shape" rather than "something the control-character + // rule does". + let mut trunc = NamedTempFile::new()?; + trunc.write_all(b"{\"ok\":1}\n[1,2,")?; + trunc.flush()?; + let trunc_path = trunc.path().to_str().expect("temp path is utf-8"); + let (trunc_stdout, _stderr, trunc_code) = run_jq_full(&["-c", ".", trunc_path], None)?; + assert_ne!(trunc_code, 0); + assert_eq!( + trunc_stdout, "", + "a truncated later value behaves identically, and predates #2878" + ); + Ok(()) +} + /// The negative control for the whole rule: the *escaped* spellings of the /// same characters are well-formed JSON and must keep working, and so must a /// raw `0x7F`. Without this, a fix that rejected every tab -- raw or escaped diff --git a/tests/yq_cli_tests.rs b/tests/yq_cli_tests.rs index 8a63958a8..ca947fe75 100644 --- a/tests/yq_cli_tests.rs +++ b/tests/yq_cli_tests.rs @@ -46742,22 +46742,39 @@ fn test_fromjson_accepts_raw_control_character_in_yq_mode_2878() -> Result<()> { Ok(()) } -/// `tonumber`'s "valid JSON but not a number" probe is mode-sensitive as of -/// #2878. Real yq reports a tag-conversion error here (`cannot convert node -/// value [...] of tag !!str to number`), which is this crate's -/// `cannot be parsed as a number` family -- *not* jq's parse-error family. -/// -/// Pins the half of #2878 that a jq-only fix would have silently regressed: -/// passing jq's mode into that probe unconditionally flips yq's message to -/// the wrong family. -#[test] -fn test_tonumber_control_character_keeps_yq_message_family_2878() -> Result<()> { - let prog = format!("\"[\\\"a{}b\\\"]\" | tonumber", '\u{9}'); - let (_stdout, stderr, code) = run_yq_stdin_with_stderr(&prog, "", &["-n"])?; - assert_ne!(code, 0, "tonumber must still fail in yq mode"); - assert!( - stderr.contains("cannot be parsed as a number"), - "yq mode should keep the tag-conversion family, got: {stderr}" - ); +/// `tonumber` in yq mode reports **one** error family for every non-numeric +/// string, because real yq does: `[1,2]`, `abc`, `1 2`, `""`, a lone +/// `\udc00` and a raw control character all answer `cannot convert node +/// value [...] of tag !!str to number` (yq 4.53.3, captured live). jq's +/// "valid JSON but not a number" vs "not valid JSON at all" split has no yq +/// equivalent, so yq mode does not run that probe at all (#2878). +/// +/// Sweeping the whole list is the point, not thoroughness for its own sake: +/// the two inputs that matter sit at *opposite* ends of the mode split, so +/// any single-flag answer gets one of them wrong. A lone surrogate is +/// rejected only by yq's reader (#2008) and a raw control character only by +/// jq's (#2878) -- so a fix driven by either one alone silently flips the +/// other into jq's parse-error family. Both are pinned here. +#[test] +fn test_tonumber_reports_one_error_family_in_yq_mode_2878() -> Result<()> { + let raw_control = format!("\"[\\\"a{}b\\\"]\" | tonumber", '\u{9}'); + let cases: [&str; 6] = [ + &raw_control, + // A lone low surrogate: rejected by yq's own reader, accepted (as + // U+FFFD) by jq's -- the mirror image of the case above. + r#""\"\\udc00\"" | tonumber"#, + r#""[1,2]" | tonumber"#, + r#""abc" | tonumber"#, + r#""1 2" | tonumber"#, + r#""" | tonumber"#, + ]; + for prog in cases { + let (_stdout, stderr, code) = run_yq_stdin_with_stderr(prog, "", &["-n"])?; + assert_ne!(code, 0, "tonumber must fail in yq mode for {prog}"); + assert!( + stderr.contains("cannot be parsed as a number"), + "yq mode must use its single error family for {prog}, got: {stderr}" + ); + } Ok(()) } From f745948301947cd6286634379442c717a6c432ee Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 17:26:36 +1000 Subject: [PATCH 6/7] test(jq): drop an uncoverable call from an assertion message `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 --- src/json/light.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/json/light.rs b/src/json/light.rs index 7ef6f28ad..36dd7d8be 100644 --- a/src/json/light.rs +++ b/src/json/light.rs @@ -6344,8 +6344,7 @@ mod tests { assert_eq!( string_literal_end(doc, 0), Some(doc.len()), - "{:?} is one whole string literal", - core::str::from_utf8(doc).unwrap() + "{doc:?} is one whole string literal" ); } } From 95ddd4b9e9e2f6cca2fa30d03583b9485a0e3c39 Mon Sep 17 00:00:00 2001 From: John Ky Date: Mon, 14 Sep 2026 18:36:40 +1000 Subject: [PATCH 7/7] ci: widen users_keys_unsorted's perf-guard threshold for #2878's scan 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 -- 9d555f3f0 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 --- scripts/perf-guard.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/scripts/perf-guard.py b/scripts/perf-guard.py index ad5d399c0..40652266f 100755 --- a/scripts/perf-guard.py +++ b/scripts/perf-guard.py @@ -214,8 +214,37 @@ # comment for the +16%-to-noise fix that predates this). 10% leaves headroom # above the observed ARM64 number while still catching a *further* # regression on top of this one. +# +# `users_keys_unsorted` (#2878): the same row, for the opposite reason -- a +# drift that is *faster*, on one architecture only. The guard is `abs(drift)`, +# so direction is irrelevant to whether an override is needed (see 9d555f3f0, +# which corrected exactly that misreading). +# +# #2878 routed the CLI's document splitter through `find_json_escape`, whose +# `"`/`\`/`< 0x20` predicate is the control-character rule it had to add +# anyway, replacing a byte-at-a-time scalar loop. Measured by this guard on +# its own runners against the PR's merge-base (#1582), the two architectures +# disagree in sign: +# +# 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 a fixture whose strings are short (the `users` +# shape is small records). `users_keys_unsorted` is the smallest query in the +# matrix, so it shows the largest relative swing in both directions. Only the +# ARM64 number exceeds `DEFAULT_THRESHOLD`; 10% clears it with headroom while +# still catching a further move on top of it, in either direction. +# +# **Remove this entry once `main` has moved past #2878.** Both overrides here +# are permanent loosenings of a row whose only job is to be watched -- with +# `--baseline-binary` on every PR and push run the checked-in file is never +# consulted, so once the merge-base includes #2878 this row reads ~0% again +# and the override only blinds it. Tracked by the follow-up filed on #2878. QUERY_THRESHOLDS = { "wide_keys_unsorted": 10.0, + "users_keys_unsorted": 10.0, } # argparse wants a plain string for `epilog`; keeping it as a real constant