fix(jq): reject a raw control character in a JSON string, matching jq

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 `["a<TAB>b"]` 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 `["a<TAB>b"]`, 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

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

src/bin/succinctly/jq_runner.rs

@@ -3276,16 +3276,31 @@ fn find_json_values(bytes: &[u8]) -> core::result::Result<Vec<(usize, usize)>, 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<usize> {
     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<usize> {
     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<usize> {
     }
 }
 
-/// Find the end of a string starting at `pos` (which points to opening quote).
-fn find_string_end(bytes: &[u8], pos: usize) -> Option<usize> {
-    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<usize> {
     let mut i = pos;

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::<W, S>(value, optional),
-        Builtin::ToNumber => builtin_tonumber::<W>(value, optional),
+        Builtin::ToNumber => builtin_tonumber::<W, S>(value, optional),
         Builtin::ToJson => builtin_tojson::<W, S>(value, optional),
         Builtin::FromJson => builtin_fromjson::<W, S>(value, optional),
 
@@ -15646,7 +15646,7 @@ fn builtin_tostring<W: Clone + AsRef<[u64]>, S: EvalSemantics>(
 }
 
 /// Builtin: tonumber - convert string to number
-fn builtin_tonumber<W: Clone + AsRef<[u64]>>(
+fn builtin_tonumber<W: Clone + AsRef<[u64]>, S: EvalSemantics>(
     value: StandardJson<'_, W>,
     optional: bool,
 ) -> QueryResult<'_, W> {
@@ -15658,7 +15658,7 @@ fn builtin_tonumber<W: Clone + AsRef<[u64]>>(
             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<W: Clone + AsRef<[u64]>>(
 /// 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<OwnedValue, EvalError> {
+pub(super) fn tonumber_from_str(s: &str, yq_mode: bool) -> Result<OwnedValue, EvalError> {
     // 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<OwnedValue, EvalError> {
     if let Ok(f) = trimmed.parse::<f64>() {
         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..];

src/jq/eval_generic.rs

@@ -20987,7 +20987,7 @@ fn eval_builtin<S: EvalSemantics, V: DocumentValue>(
                 // -- 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),