Repository: quickwit-oss/tantivy (language: rust)

Issue:
keys should be increasing panic
```
Jun 21 10:27:18 quickwit-indexer-34 quickwit-indexer thread 'blocking-7' panicked at /usr/local/cargo/git/checkouts/tantivy-f70b7ea03dadae9a/b91c37a/sstable/src/lib.rs:257:9:
Jun 21 10:27:18 quickwit-indexer-34 quickwit-indexer Keys should be increasing. ([111, 95, 112, 97, 114, 97, 109, 115, 1, 48, 34, 62, 60, 115, 99, 114, 105, 112, 116, 62, 97, 108, 101, 114, 116, 40, 51, 48, 57, 41, 60, 47, 115, 99, 114, 105, 112, 116, 62, 0, 115, 49] > [111, 95, 112, 97, 114, 97, 109, 115, 1, 32, 65, 68, 119, 45, 115, 99, 114, 105, 112, 116, 32, 65, 68, 52, 45, 97, 108, 101, 114, 116, 40, 51, 49, 50, 41, 32, 65, 68, 119, 45, 47, 115, 99, 114, 105, 112, 116, 32, 65, 68, 52, 45, 0, 115, 49])
```

I don't know if it is the inverted index or the columnar.

The two strings are respectively:
```
o_params0"><script>alert(309)</script>s1
o_params ADw-script AD4-alert(312) ADw-/script AD4-s1
```


Discussion / hints:
The two document indeed contains the following:

```
{"o_params":   {"\u0000\"><script>alert(309)</script>": "1"}}
{"o_params":   {" ADw-script AD4-alert(312) ADw-/script AD4-": "1"}}
```
 The "0" and " " at the beginning look suspicious.

A stacktrace would be really helpful (Shouldn't we have them on by default?)

It cannot be reproduced like this:

```rust
    #[test]
    fn test_bug_2442() -> crate::Result<()> {
        let mut schema_builder = schema::Schema::builder();
        let json_field = schema_builder.add_json_field("json", TEXT | FAST);

        let schema = schema_builder.build();
        let index = Index::builder().schema(schema).create_in_ram()?;
        let mut index_writer = index.writer_for_tests()?;
        index_writer.set_merge_policy(Box::new(NoMergePolicy));

        let path1 = String::from_utf8(vec![
            48, 34, 62, 60, 115, 99, 114, 105, 112, 116, 62, 97, 108, 101, 114, 116, 40, 51, 48,
            57, 41, 60, 47, 115, 99, 114, 105, 112, 116, 62,
        ])
        .unwrap();

        let path2 = String::from_utf8(vec![
            32, 65, 68, 119, 45, 115, 99, 114, 105, 112, 116, 32, 65, 68, 52, 45, 97, 108, 101,
            114, 116, 40, 51, 49, 50, 41, 32, 65, 68, 119, 45, 47, 115, 99, 114, 105, 112, 116, 32,
            65, 68, 52, 45,
        ])
        .unwrap();

        let get_doc_1 = || json!({"o_params": { path1.clone(): "s" }});
        let get_doc_2 = || json!({"o_params": { path2.clone(): "s" }});

        let add_doc_1 = |index_writer: &mut IndexWriter| {
            index_writer
                .add_document(doc!(
                    json_field=>get_doc_1()
                ))
                .unwrap()
        };
        let add_doc_2 = |index_writer: &mut IndexWriter| {
            index_writer
                .add_document(doc!(
                    json_field=>get_doc_2()
                ))
                .unwrap()
        };

        add_doc_1(&mut index_writer);
        add_doc_2(&mut index_writer);
        index_writer.commit()?;
        add_doc_2(&mut index_writer);
        add_doc_1(&mut index_writer);
        index_writer.commit()?;

        add_doc_1(&mut index_writer);
        index_writer.commit()?;
        add_doc_2(&mut index_writer);
        index_writer.commit()?;
        add_doc_2(&mut index_writer);
        index_writer.commit()?;
        add_doc_1(&mut index_writer);
        index_writer.commit()?;

        // Merge
        {
            assert!(index_writer.wait_merging_threads().is_ok());
            let mut index_writer: IndexWriter = index.writer_for_tests()?;
            let segment_ids = index
                .searchable_segment_ids()
                .expect("Searchable segments failed.");
            index_writer.merge(&segment_ids).wait().unwrap();
            assert!(index_writer.wait_merging_threads().is_ok());
        }

        Ok(())
    }
```
```
thread 'blocking-5' panicked at /Users/fulmicoton/.cargo/git/checkouts/tantivy-f70b7ea03dadae9a/b960e40/sstable/src/lib.rs:257:9:
Keys should be increasing. ([111, 95, 112, 97, 114, 97, 109, 115, 1, 48, 34, 62, 60, 115, 99, 114, 105, 112, 116, 62, 97, 108, 101, 114, 116, 40, 51, 48, 57, 41, 60, 47, 115, 99, 114, 105, 112, 116, 62, 0, 115, 49] > [111, 95, 112, 97, 114, 97, 109, 115, 1, 32, 65, 68, 119, 45, 115, 99, 114, 105, 112, 116, 32, 65, 68, 52, 45, 97, 108, 101, 114, 116, 40, 51, 49, 50, 41, 32, 65, 68, 119, 45, 47, 115, 99, 114, 105, 112, 116, 32, 65, 68, 52, 45, 0, 115, 49])
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: tantivy::postings::serializer::FieldSerializer::new_term
   3: <tantivy::postings::json_postings_writer::JsonPostingsWriter<Rec> as tantivy::postings::postings_writer::PostingsWriter>::serialize
   4: tantivy::postings::postings_writer::serialize_postings
   5: tantivy::indexer::segment_writer::SegmentWriter::finalize
   6: quickwit_indexing::models::indexed_split::IndexedSplitBuilder::finalize
   7: <tracing::instrument::Instrumented<T> as core::future::future::Future>::poll
   8: <quickwit_indexing::actors::index_serializer::IndexSerializer as quickwit_actors::actor::Handler<quickwit_indexing::models::indexed_split::IndexedSplitBatchBuilder>>::handle::{{closure}}
   9: <H as quickwit_actors::actor::DeferableReplyHandler<M>>::handle_message::{{closure}}
  10: <core::option::Option<(tokio::sync::oneshot::Sender<<A as quickwit_actors::actor::DeferableReplyHandler<M>>::Reply>,M)> as quickwit_actors::envelope::EnvelopeT<A>>::handle_message::{{closure}}
  11: quickwit_actors::spawn_builder::ActorExecutionEnv<A>::process_one_message::{{closure}}
  12: quickwit_actors::spawn_builder::SpawnBuilder<A>::spawn::{{closure}}
  13: tokio::runtime::task::core::Core<T,S>::poll
  14: tokio::runtime::task::harness::Harness<T,S>::poll
  15: tokio::runtime::scheduler::multi_thread::worker::Context::run_task
  16: tokio::runtime::scheduler::multi_thread::worker::Context::run
  17: tokio::runtime::context::set_scheduler
  18: tokio::runtime::scheduler::multi_thread::worker::run
  19: <tokio::runtime::blocking::task::BlockingTask<T> as core::future::future::Future>::poll
  20: tokio::runtime::task::core::Core<T,S>::poll
  21: tokio::runtime::task::raw::poll
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
```
(Almost) minimum reproducible example on quickwit

2 documents:
```json
{"\u0000B":"1"}
{" A":"1"}
```

Index config:
```json
{
  "version": "0.7",
  "index_id": "airmail",
  "indexing_settings": {
    "commit_timeout_secs": 30
  },
  "doc_mapping": {
    "mode": "dynamic",
    "dynamic_mapping": {
      "tokenizer": "raw",
      "fast": true
    }
  }
}
```

Fix patch:
diff --git a/columnar/src/columnar/writer/mod.rs b/columnar/src/columnar/writer/mod.rs
index 239a7422d6..9b118e2117 100644
--- a/columnar/src/columnar/writer/mod.rs
+++ b/columnar/src/columnar/writer/mod.rs
@@ -8,6 +8,7 @@ use std::net::Ipv6Addr;
 
 use column_operation::ColumnOperation;
 pub(crate) use column_writers::CompatibleNumericalTypes;
+use common::json_path_writer::JSON_END_OF_PATH;
 use common::CountingWriter;
 pub(crate) use serializer::ColumnarSerializer;
 use stacker::{Addr, ArenaHashMap, MemoryArena};
@@ -247,6 +248,7 @@ impl ColumnarWriter {
     }
     pub fn serialize(&mut self, num_docs: RowId, wrt: &mut dyn io::Write) -> io::Result<()> {
         let mut serializer = ColumnarSerializer::new(wrt);
+
         let mut columns: Vec<(&[u8], ColumnType, Addr)> = self
             .numerical_field_hash_map
             .iter()
@@ -260,7 +262,7 @@ impl ColumnarWriter {
         columns.extend(
             self.bytes_field_hash_map
                 .iter()
-                .map(|(term, addr)| (term, ColumnType::Bytes, addr)),
+                .map(|(column_name, addr)| (column_name, ColumnType::Bytes, addr)),
         );
         columns.extend(
             self.str_field_hash_map
@@ -287,6 +289,12 @@ impl ColumnarWriter {
         let (arena, buffers, dictionaries) = (&self.arena, &mut self.buffers, &self.dictionaries);
         let mut symbol_byte_buffer: Vec<u8> = Vec::new();
         for (column_name, column_type, addr) in columns {
+            if column_name.contains(&JSON_END_OF_PATH) {
+                // Tantivy uses b'0' as a separator for nested fields in JSON.
+                // Column names with a b'0' are not simply ignored by the columnar (and the inverted
+                // index).
+                continue;
+            }
             match column_type {
                 ColumnType::Bool => {
                     let column_writer: ColumnWriter = self.bool_field_hash_map.read(addr);
diff --git a/columnar/src/columnar/writer/serializer.rs b/columnar/src/columnar/writer/serializer.rs
index 394e61cd96..0e5a1e2b24 100644
--- a/columnar/src/columnar/writer/serializer.rs
+++ b/columnar/src/columnar/writer/serializer.rs
@@ -1,6 +1,7 @@
 use std::io;
 use std::io::Write;
 
+use common::json_path_writer::JSON_END_OF_PATH;
 use common::{BinarySerializable, CountingWriter};
 use sstable::value::RangeValueWriter;
 use sstable::RangeSSTable;
@@ -18,13 +19,8 @@ pub struct ColumnarSerializer<W: io::Write> {
 /// code.
 fn prepare_key(key: &[u8], column_type: ColumnType, buffer: &mut Vec<u8>) {
     buffer.clear();
-    // Convert 0 bytes to '0' string, as 0 bytes are reserved for the end of the path.
-    if key.contains(&0u8) {
-        buffer.extend(key.iter().map(|&b| if b == 0 { b'0' } else { b }));
-    } else {
-        buffer.extend_from_slice(key);
-    }
-    buffer.push(0u8);
+    buffer.extend_from_slice(key);
+    buffer.push(JSON_END_OF_PATH);
     buffer.push(column_type.to_code());
 }
 
@@ -97,18 +93,3 @@ impl<'a, W: io::Write> io::Write for ColumnSerializer<'a, W> {
         self.columnar_serializer.wrt.write_all(buf)
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn test_prepare_key_bytes() {
-        let mut buffer: Vec<u8> = b"somegarbage".to_vec();
-        prepare_key(b"root\0child", ColumnType::Str, &mut buffer);
-        assert_eq!(buffer.len(), 12);
-        assert_eq!(&buffer[..10], b"root0child");
-        assert_eq!(buffer[10], 0u8);
-        assert_eq!(buffer[11], ColumnType::Str.to_code());
-    }
-}
diff --git a/src/core/json_utils.rs b/src/core/json_utils.rs
index ee75bb354a..2767cd6d93 100644
--- a/src/core/json_utils.rs
+++ b/src/core/json_utils.rs
@@ -1,4 +1,4 @@
-use common::json_path_writer::JSON_PATH_SEGMENT_SEP;
+use common::json_path_writer::{JSON_END_OF_PATH, JSON_PATH_SEGMENT_SEP};
 use common::{replace_in_place, JsonPathWriter};
 use rustc_hash::FxHashMap;
 
@@ -83,6 +83,9 @@ fn index_json_object<'a, V: Value<'a>>(
     positions_per_path: &mut IndexingPositionsPerPath,
 ) {
     for (json_path_segment, json_value_visitor) in json_visitor {
+        if json_path_segment.as_bytes().contains(&JSON_END_OF_PATH) {
+            continue;
+        }
         json_path_writer.push(json_path_segment);
         index_json_value(
             doc,
diff --git a/src/indexer/index_writer.rs b/src/indexer/index_writer.rs
index 4792574d06..3149127a08 100644
--- a/src/indexer/index_writer.rs
+++ b/src/indexer/index_writer.rs
@@ -815,8 +815,9 @@ mod tests {
     use crate::indexer::NoMergePolicy;
     use crate::query::{QueryParser, TermQuery};
     use crate::schema::{
-        self, Facet, FacetOptions, IndexRecordOption, IpAddrOptions, NumericOptions,
-        TextFieldIndexing, TextOptions, Value, FAST, INDEXED, STORED, STRING, TEXT,
+        self, Facet, FacetOptions, IndexRecordOption, IpAddrOptions, JsonObjectOptions,
+        NumericOptions, Schema, TextFieldIndexing, TextOptions, Value, FAST, INDEXED, STORED,
+        STRING, TEXT,
     };
     use crate::store::DOCSTORE_CACHE_CAPACITY;
     use crate::{
@@ -2378,11 +2379,11 @@ mod tests {
 
     #[test]
     fn test_bug_1617_2() {
-        assert!(test_operation_strategy(
+        test_operation_strategy(
             &[
                 IndexingOp::AddDoc {
                     id: 13,
-                    value: Default::default()
+                    value: Default::default(),
                 },
                 IndexingOp::DeleteDoc { id: 13 },
                 IndexingOp::Commit,
@@ -2390,9 +2391,9 @@ mod tests {
                 IndexingOp::Commit,
                 IndexingOp::Merge,
             ],
-            true
+            true,
         )
-        .is_ok());
+        .unwrap();
     }
 
     #[test]
@@ -2490,4 +2491,46 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_bug_2442_reserved_character_fast_field() -> crate::Result<()> {
+        let mut schema_builder = schema::Schema::builder();
+        let json_field = schema_builder.add_json_field("json", FAST | TEXT);
+
+        let schema = schema_builder.build();
+        let index = Index::builder().schema(schema).create_in_ram()?;
+        let mut index_writer = index.writer_for_tests()?;
+        index_writer.set_merge_policy(Box::new(NoMergePolicy));
+
+        index_writer
+            .add_document(doc!(
+                json_field=>json!({"\u{0000}B":"1"})
+            ))
+            .unwrap();
+        index_writer
+            .add_document(doc!(
+                json_field=>json!({" A":"1"})
+            ))
+            .unwrap();
+        index_writer.commit()?;
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_bug_2442_reserved_character_columnar() -> crate::Result<()> {
+        let mut schema_builder = Schema::builder();
+        let options = JsonObjectOptions::from(FAST).set_expand_dots_enabled();
+        let field = schema_builder.add_json_field("json", options);
+        let index = Index::create_in_ram(schema_builder.build());
+        let mut index_writer = index.writer_for_tests().unwrap();
+        index_writer
+            .add_document(doc!(field=>json!({"\u{0000}": "A"})))
+            .unwrap();
+        index_writer
+            .add_document(doc!(field=>json!({format!("\u{0000}\u{0000}"): "A"})))
+            .unwrap();
+        index_writer.commit().unwrap();
+        Ok(())
+    }
 }
diff --git a/src/indexer/mod.rs b/src/indexer/mod.rs
index b3dfb2a6f8..1a83ccca17 100644
--- a/src/indexer/mod.rs
+++ b/src/indexer/mod.rs
@@ -145,15 +145,27 @@ mod tests_mmap {
         }
     }
     #[test]
-    fn test_json_field_null_byte() {
-        // Test when field name contains a zero byte, which has special meaning in tantivy.
-        // As a workaround, we convert the zero byte to the ASCII character '0'.
-        // https://github.com/quickwit-oss/tantivy/issues/2340
-        // https://github.com/quickwit-oss/tantivy/issues/2193
-        let field_name_in = "\u{0000}";
-        let field_name_out = "0";
-        test_json_field_name(field_name_in, field_name_out);
+    fn test_json_field_null_byte_is_ignored() {
+        let mut schema_builder = Schema::builder();
+        let options = JsonObjectOptions::from(TEXT | FAST).set_expand_dots_enabled();
+        let field = schema_builder.add_json_field("json", options);
+        let index = Index::create_in_ram(schema_builder.build());
+        let mut index_writer = index.writer_for_tests().unwrap();
+        index_writer
+            .add_document(doc!(field=>json!({"key": "test1", "invalidkey\u{0000}": "test2"})))
+            .unwrap();
+        index_writer.commit().unwrap();
+        let reader = index.reader().unwrap();
+        let searcher = reader.searcher();
+        let segment_reader = searcher.segment_reader(0);
+        let inv_indexer = segment_reader.inverted_index(field).unwrap();
+        let term_dict = inv_indexer.terms();
+        assert_eq!(term_dict.num_terms(), 1);
+        let mut term_bytes = Vec::new();
+        term_dict.ord_to_term(0, &mut term_bytes).unwrap();
+        assert_eq!(term_bytes, b"key\0stest1");
     }
+
     #[test]
     fn test_json_field_1byte() {
         // Test when field name contains a '1' byte, which has special meaning in tantivy.
diff --git a/src/indexer/path_to_unordered_id.rs b/src/indexer/path_to_unordered_id.rs
index 054654f948..545a5a6861 100644
--- a/src/indexer/path_to_unordered_id.rs
+++ b/src/indexer/path_to_unordered_id.rs
@@ -38,7 +38,8 @@ impl PathToUnorderedId {
     #[cold]
     fn insert_new_path(&mut self, path: &str) -> u32 {
         let next_id = self.map.len() as u32;
-        self.map.insert(path.to_string(), next_id);
+        let new_path = path.to_string();
+        self.map.insert(new_path, next_id);
         next_id
     }
 
diff --git a/src/postings/json_postings_writer.rs b/src/postings/json_postings_writer.rs
index 2fab8efa35..6d4e8addc8 100644
--- a/src/postings/json_postings_writer.rs
+++ b/src/postings/json_postings_writer.rs
@@ -59,7 +59,7 @@ impl<Rec: Recorder> PostingsWriter for JsonPostingsWriter<Rec> {
     /// The actual serialization format is handled by the `PostingsSerializer`.
     fn serialize(
         &self,
-        term_addrs: &[(Field, OrderedPathId, &[u8], Addr)],
+        ordered_term_addrs: &[(Field, OrderedPathId, &[u8], Addr)],
         ordered_id_to_path: &[&str],
         ctx: &IndexingContext,
         serializer: &mut FieldSerializer,
@@ -69,7 +69,7 @@ impl<Rec: Recorder> PostingsWriter for JsonPostingsWriter<Rec> {
         term_buffer.clear_with_field_and_type(Type::Json, Field::from_field_id(0));
         let mut prev_term_id = u32::MAX;
         let mut term_path_len = 0; // this will be set in the first iteration
-        for (_field, path_id, term, addr) in term_addrs {
+        for (_field, path_id, term, addr) in ordered_term_addrs {
             if prev_term_id != path_id.path_id() {
                 term_buffer.truncate_value_bytes(0);
                 term_buffer.append_path(ordered_id_to_path[path_id.path_id() as usize].as_bytes());
diff --git a/src/schema/term.rs b/src/schema/term.rs
index b4d0d6288b..a0d1a8f677 100644
--- a/src/schema/term.rs
+++ b/src/schema/term.rs
@@ -249,12 +249,8 @@ impl Term {
     #[inline]
     pub fn append_path(&mut self, bytes: &[u8]) -> &mut [u8] {
         let len_before = self.0.len();
-        if bytes.contains(&0u8) {
-            self.0
-                .extend(bytes.iter().map(|&b| if b == 0 { b'0' } else { b }));
-        } else {
-            self.0.extend_from_slice(bytes);
-        }
+        assert!(!bytes.contains(&JSON_END_OF_PATH));
+        self.0.extend_from_slice(bytes);
         &mut self.0[len_before..]
     }
 }


Test patch: