fix(vector): a truncated FT.CREATE no longer aborts the whole process (#681)

.github/workflows/fuzz.yml

@@ -42,6 +42,7 @@ jobs:
           - term_fst_sidecar
           - dump_payload
           - acl_keyspec
+          - ft_create_args
     steps:
       - uses: actions/checkout@v7
       - uses: dtolnay/rust-toolchain@nightly
@@ -109,6 +110,7 @@ jobs:
           - term_fst_sidecar
           - dump_payload
           - acl_keyspec
+          - ft_create_args
     steps:
       - uses: actions/checkout@v7
       - uses: dtolnay/rust-toolchain@nightly

CHANGELOG.md

@@ -190,6 +190,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   as an answer. That shard also logs a warning, because cold keys with no manifest will not
   survive a restart — `rebuild_from_manifest` is the only thing that re-indexes them.
 ### Fixed
+- **A truncated `FT.CREATE` no longer aborts the server** (#681). `FT.CREATE idx ON HASH
+  PREFIX 1 d: SCHEMA v VECTOR HNSW` — the argument list cut off right after the algorithm
+  keyword — indexed one past the end of argv and panicked. The panic ran on a shard
+  thread, and moon deliberately escalates a shard panic to a whole-process abort rather
+  than serve on with a dead shard, so one short line from any client took the entire
+  server down: every database, every other connection. No auth and no large payload
+  required.
+
+  The parameter loop below the fault already guarded both ends
+  (`*pos + 1 < param_end && *pos + 1 < args.len()`), so the value read for every keyword
+  was safe; the parameter *count* read was the single unguarded one. That was measured,
+  not assumed — six truncation shapes were probed against freshly spawned,
+  listener-PID-checked servers, and only this one killed the process. It reports
+  `ERR invalid param count`, the same error an unparseable count already produced, so the
+  two ways of failing to supply a count are indistinguishable to a client. There is no
+  redis oracle for the string: the `redis-server` checked against has no query engine, so
+  `FT.CREATE` is `unknown command` there.
+
+  `FT.CREATE` argument parsing had **no fuzz target** for its whole life, which is how a
+  one-line remote crash survived in it; `fuzz/fuzz_targets/ft_create_args.rs` now drives
+  the real entry point with arbitrary argv and is listed in both matrices in
+  `fuzz.yml` (an unlisted target never runs — #576).
+
 - **Lua script errors reached the client as an unparseable RESP frame** (#672). mlua's
   `Display` carries a multi-line Lua traceback, and a RESP *simple* error may not contain
   CR or LF anywhere — so every runtime error (`redis.call('INCR', k)` on a string,

fuzz/Cargo.toml

@@ -118,3 +118,8 @@ doc = false
 name = "acl_keyspec"
 path = "fuzz_targets/acl_keyspec.rs"
 doc = false
+
+[[bin]]
+name = "ft_create_args"
+path = "fuzz_targets/ft_create_args.rs"
+doc = false

fuzz/fuzz_targets/ft_create_args.rs

@@ -0,0 +1,156 @@
+#![no_main]
+use libfuzzer_sys::fuzz_target;
+
+use bytes::Bytes;
+use moon::command::vector_search::ft_create;
+use moon::protocol::Frame;
+use moon::text::store::TextStore;
+use moon::vector::store::VectorStore;
+
+/// Fuzz `FT.CREATE` argument parsing (moon#681).
+///
+/// This parser had no fuzz target for its whole life, and that is exactly how
+/// a one-line remote crash survived in it: `FT.CREATE idx ON HASH PREFIX 1 d:
+/// SCHEMA v VECTOR HNSW` -- truncated right after the algorithm keyword --
+/// indexed one past the end of argv and panicked. The panic ran on a shard
+/// thread, and moon escalates a shard panic to a whole-process abort, so a
+/// single short line from any unauthenticated client took the server down with
+/// every database and every other connection on it.
+///
+/// The property is simply "never panics". `FT.CREATE` walks attacker-supplied
+/// argv with a hand-rolled cursor (`*pos += 1` then read), and every such
+/// cursor is a bounds bug waiting to happen -- the fix for #681 added the one
+/// missing guard, and this target exists so the next one is found here rather
+/// than in production.
+///
+/// Both `VectorStore` and `TextStore` are rebuilt per input so a create that
+/// succeeds cannot make a later input take a different path; each run sees an
+/// empty registry, which is the state a fresh server is in.
+const MAX_ARGS: usize = 128;
+
+/// The vocabulary `FT.CREATE` actually branches on.
+///
+/// **This table is why the target works.** Two earlier drafts failed, and each
+/// failure was measured against a deliberately un-fixed parser rather than
+/// assumed:
+///
+///   1. Fully-arbitrary argv: 1.2M execs, nothing. `ft_create` demands the
+///      preamble `idx ON HASH PREFIX 1 d: SCHEMA v VECTOR` before the vector
+///      parser is reached, and random mutation does not synthesize a
+///      nine-keyword sequence. It fuzzed the preamble and never got past it.
+///   2. Valid skeleton + byte-level tail: 871K execs, still nothing. Reaching
+///      the parser is not enough -- the crash needs the literal ASCII `HNSW`
+///      in argv, and inventing a specific four-byte string by mutation is a
+///      2^32 search.
+///
+/// So the fuzzer picks *keywords*, not bytes: one input byte selects one argv
+/// element from this table. Now `HNSW` is one byte away and the search is over
+/// keyword sequences -- which is the actual state space the parser walks.
+const VOCAB: &[&[u8]] = &[
+    b"HNSW",
+    b"FLAT",
+    b"TYPE",
+    b"FLOAT32",
+    b"DIM",
+    b"DISTANCE_METRIC",
+    b"L2",
+    b"COSINE",
+    b"IP",
+    b"M",
+    b"EF_CONSTRUCTION",
+    b"EF_RUNTIME",
+    b"COMPACT_THRESHOLD",
+    b"QUANTIZATION",
+    b"TQ4",
+    b"SQ8",
+    b"FP32",
+    b"BUILD_MODE",
+    b"MERGE_MODE",
+    b"GRAPH_UNION",
+    b"KEEP_RAW",
+    b"WEIGHTED",
+    b"0",
+    b"1",
+    b"2",
+    b"4",
+    b"6",
+    b"8",
+    b"16",
+    b"768",
+    b"-1",
+    b"99999999999999999999",
+    b"",
+    b"NOT_A_KEYWORD",
+];
+
+fn bulk(s: &[u8]) -> Frame {
+    Frame::BulkString(Bytes::copy_from_slice(s))
+}
+
+/// A well-formed `FT.CREATE` up to and including the `VECTOR` keyword, so the
+/// fuzzed tail lands exactly where `parse_vector_field_params` starts reading.
+fn skeleton() -> Vec<Frame> {
+    [
+        b"idx".as_slice(),
+        b"ON",
+        b"HASH",
+        b"PREFIX",
+        b"1",
+        b"d:",
+        b"SCHEMA",
+        b"v",
+        b"VECTOR",
+    ]
+    .iter()
+    .map(|s| bulk(s))
+    .collect()
+}
+
+/// Decode `data` into an argv: one byte per argv element.
+///
+/// A byte selects a `VOCAB` entry; two reserved residues emit an `Integer` and
+/// a `Null` instead, because a non-string where a keyword belongs is a
+/// malformed invocation the parser still has to survive, and `extract_bulk`
+/// returning `None` drives cursor arithmetic the all-strings shape never
+/// reaches.
+///
+/// One input in four is argv with no skeleton at all, so the preamble parser
+/// -- everything before `VECTOR` -- is not left uncovered by the specialisation.
+fn decode(data: &[u8]) -> Vec<Frame> {
+    if data.is_empty() {
+        return Vec::new();
+    }
+    let tag = data[0];
+    let tail: Vec<Frame> = data[1..]
+        .iter()
+        .take(MAX_ARGS)
+        .enumerate()
+        .map(|(i, &b)| {
+            let slot = b as usize % (VOCAB.len() + 2);
+            match slot.checked_sub(VOCAB.len()) {
+                Some(0) => Frame::Integer(i as i64),
+                Some(_) => Frame::Null,
+                None => bulk(VOCAB[slot]),
+            }
+        })
+        .collect();
+
+    if tag % 4 == 0 {
+        return tail;
+    }
+    let mut args = skeleton();
+    args.extend(tail);
+    args
+}
+
+fuzz_target!(|data: &[u8]| {
+    let args = decode(data);
+    // No length guard here on purpose: `take(MAX_ARGS)` already bounds the
+    // tail, and an early `return` would be a silent skip -- inputs the target
+    // reports as covered while never running them.
+    let mut store = VectorStore::new();
+    let mut text = TextStore::new();
+    // The reply is not asserted on: FT.CREATE legitimately answers +OK or any
+    // of a dozen errors depending on argv. Surviving the call IS the property.
+    let _ = ft_create(&mut store, &mut text, &args, 0);
+});

src/command/vector_search/ft_create.rs

@@ -547,6 +547,21 @@ fn parse_vector_field_params(args: &[Frame], pos: &mut usize) -> Result<ParsedVe
     }
     *pos += 1;
 
+    // moon#681: `... VECTOR HNSW` with nothing after it left `*pos` at
+    // `args.len()`, and the read below indexed one past the end. That panic
+    // ran on a shard thread, which moon escalates to a process abort -- so a
+    // single truncated line from any client took the whole server down. The
+    // parameter loop further down already guards both ends
+    // (`*pos + 1 < args.len()`); this read was the one that did not.
+    //
+    // The error is the same one an unparseable count gets, so the two ways of
+    // failing to supply a count read identically to a client. There is no
+    // redis oracle to match: the `redis-server` this was checked against has
+    // no query engine, so `FT.CREATE` is `unknown command` there.
+    if *pos >= args.len() {
+        return Err(Frame::Error(Bytes::from_static(b"ERR invalid param count")));
+    }
+
     let num_params = match parse_u32(&args[*pos]) {
         Some(n) => n as usize,
         None => {
@@ -781,3 +796,96 @@ fn parse_vector_field_params(args: &[Frame], pos: &mut usize) -> Result<ParsedVe
         keep_raw,
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn bulk(s: &[u8]) -> Frame {
+        Frame::BulkString(Bytes::copy_from_slice(s))
+    }
+
+    fn err_text(f: &Frame) -> String {
+        match f {
+            Frame::Error(b) => String::from_utf8_lossy(b).into_owned(),
+            other => panic!("expected an error frame, got {other:?}"),
+        }
+    }
+
+    /// moon#681: `FT.CREATE idx ... SCHEMA v VECTOR HNSW` with nothing after
+    /// the algorithm keyword used to index one past the end and panic. The
+    /// panic was on a shard thread, and moon escalates a shard panic to a
+    /// process abort -- so a single short line from any client took the whole
+    /// server down, every database and every other connection with it.
+    ///
+    /// Before the fix this test does not fail an assertion, it *panics*, which
+    /// is the point: the parser must return an error frame for a truncated
+    /// argument list, never index past the end.
+    #[test]
+    fn truncated_after_the_algorithm_keyword_errors_instead_of_panicking() {
+        let args = vec![bulk(b"HNSW")];
+        let mut pos = 0usize;
+        let Err(err) = parse_vector_field_params(&args, &mut pos) else {
+            panic!("a truncated VECTOR clause must not parse");
+        };
+        assert_eq!(err_text(&err), "ERR invalid param count");
+    }
+
+    /// The neighbouring truncations were already safe -- the parameter loop
+    /// guards `*pos + 1 < args.len()` before every value read -- and this
+    /// pins that, so a future edit to the loop condition cannot quietly
+    /// reopen the same hole one keyword further in.
+    #[test]
+    fn truncations_inside_the_param_list_are_already_bounded() {
+        for tail in [
+            vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"TYPE")],
+            vec![
+                bulk(b"HNSW"),
+                bulk(b"6"),
+                bulk(b"TYPE"),
+                bulk(b"FLOAT32"),
+                bulk(b"DIM"),
+            ],
+            vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"DISTANCE_METRIC")],
+            vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"M")],
+            vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"EF_CONSTRUCTION")],
+            vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"EF_RUNTIME")],
+            vec![bulk(b"HNSW"), bulk(b"6"), bulk(b"COMPACT_THRESHOLD")],
+        ] {
+            let mut pos = 0usize;
+            // Either outcome is fine; not panicking is the assertion.
+            let _ = parse_vector_field_params(&tail, &mut pos);
+        }
+    }
+
+    /// A non-numeric count still reports the count, not something further on.
+    #[test]
+    fn a_non_numeric_param_count_is_reported_as_such() {
+        let args = vec![bulk(b"HNSW"), bulk(b"notanint")];
+        let mut pos = 0usize;
+        let Err(err) = parse_vector_field_params(&args, &mut pos) else {
+            panic!("a non-numeric param count must not parse");
+        };
+        assert_eq!(err_text(&err), "ERR invalid param count");
+    }
+
+    /// The pre-existing guard above the fix: a missing/incorrect algorithm
+    /// keyword keeps its own distinct message, so the two failures stay
+    /// distinguishable to a client.
+    #[test]
+    fn a_wrong_algorithm_keyword_keeps_its_own_message() {
+        let args = vec![bulk(b"FLAT"), bulk(b"6")];
+        let mut pos = 0usize;
+        let Err(err) = parse_vector_field_params(&args, &mut pos) else {
+            panic!("FLAT is not implemented");
+        };
+        assert_eq!(err_text(&err), "ERR expected HNSW algorithm");
+
+        let empty: Vec<Frame> = vec![];
+        let mut pos = 0usize;
+        let Err(err) = parse_vector_field_params(&empty, &mut pos) else {
+            panic!("an empty argument list must not parse");
+        };
+        assert_eq!(err_text(&err), "ERR expected HNSW algorithm");
+    }
+}