diff --git a/docs/compliance/jq/limitations.md b/docs/compliance/jq/limitations.md index 7d3a68536..e5547ad20 100644 --- a/docs/compliance/jq/limitations.md +++ b/docs/compliance/jq/limitations.md @@ -4298,6 +4298,35 @@ 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. + +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/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 diff --git a/src/bin/succinctly/jq_runner.rs b/src/bin/succinctly/jq_runner.rs index 862f7ecde..b1d10fa53 100644 --- a/src/bin/succinctly/jq_runner.rs +++ b/src/bin/succinctly/jq_runner.rs @@ -3276,16 +3276,31 @@ 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 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 +3327,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 +3348,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/jq/eval.rs b/src/jq/eval.rs index df32e7cab..1e60351f5 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,10 +15747,28 @@ 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. + // 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(), @@ -16131,6 +16149,31 @@ 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. + // + // 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" + )); + } 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), diff --git a/src/json/light.rs b/src/json/light.rs index 5735fe35b..36dd7d8be 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,88 @@ 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()), + "{doc:?} is one whole string literal" + ); + } + } + + /// 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). diff --git a/tests/jq_cli_tests.rs b/tests/jq_cli_tests.rs index 7970ef095..530aa84b5 100644 --- a/tests/jq_cli_tests.rs +++ b/tests/jq_cli_tests.rs @@ -54344,3 +54344,199 @@ 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\"}]}"[..], + ] { + 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(()) +} + +/// 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 +/// -- 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..ca947fe75 100644 --- a/tests/yq_cli_tests.rs +++ b/tests/yq_cli_tests.rs @@ -46716,3 +46716,65 @@ 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` 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(()) +}