From 80c246903ccd66d4fd641765ea46d1be5a8a19cc Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Fri, 9 Jan 2026 04:10:10 +0100 Subject: [PATCH 1/2] Backport #93663 to stable-25.8: Fix trash in `TokenExtractor` `NgramTokenExtractor::nextInString` adds `UTF8::seqLength` of the lead byte without clamping to the string length. A string that ends with a truncated multibyte UTF-8 character makes the token extend past the end of the string, so `ngrambf_v1` hashes bytes beyond it. For the last row of a block those bytes are uninitialized column padding, so the index content becomes nondeterministic. Replicas then build different `skp_idx_*.idx` files for the same merge, which shows up as `CHECKSUM_DOESNT_MATCH` / `DataAfterMergeDiffersFromReplica` and a re-fetch of the merged part. Upstream cherry-pick to 25.8 (https://github.com/ClickHouse/ClickHouse/pull/93766) was closed on conflicts. This applies the same bounds checks to the 25.8 code: `NgramTokenExtractor::nextInString`, `NgramTokenExtractor::nextInStringLike` and `DefaultTokenExtractor::nextInStringLike`. `SparseGramsTokenExtractor` does not exist in this file in 25.8. Original PR: https://github.com/ClickHouse/ClickHouse/pull/93663 Closes upstream issue: https://github.com/ClickHouse/ClickHouse/issues/92576 (cherry picked from commit f9fc3b2322d, conflicts resolved manually) Co-Authored-By: Claude Opus 5.5 --- src/Interpreters/ITokenExtractor.cpp | 8 ++++++-- .../0_stateless/03784_msan_token_iterator.reference | 0 tests/queries/0_stateless/03784_msan_token_iterator.sql | 4 ++++ 3 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 tests/queries/0_stateless/03784_msan_token_iterator.reference create mode 100644 tests/queries/0_stateless/03784_msan_token_iterator.sql diff --git a/src/Interpreters/ITokenExtractor.cpp b/src/Interpreters/ITokenExtractor.cpp index fc77a7507cbcb..4faa7dd5578fe 100644 --- a/src/Interpreters/ITokenExtractor.cpp +++ b/src/Interpreters/ITokenExtractor.cpp @@ -55,6 +55,8 @@ bool NgramTokenExtractor::nextInString(const char * data, size_t length, size_t size_t sz = UTF8::seqLength(static_cast(data[*token_start + *token_length])); *token_length += sz; } + /// The sequence length is determined by the first character, but we should always check it does not go beyond the buffer length. + *token_length = std::min(*token_length, length - *token_start); *pos += UTF8::seqLength(static_cast(data[*pos])); return code_points == n; } @@ -90,7 +92,8 @@ bool NgramTokenExtractor::nextInStringLike(const char * data, size_t length, siz else { const size_t sz = UTF8::seqLength(static_cast(data[i])); - for (size_t j = 0; j < sz; ++j) + /// The sequence length is determined by the first character, but we should always check it does not go beyond the buffer length. + for (size_t j = 0; j < sz && i + j < length; ++j) token += data[i + j]; i += sz; ++code_points; @@ -254,7 +257,8 @@ bool DefaultTokenExtractor::nextInStringLike(const char * data, size_t length, s else { const size_t sz = UTF8::seqLength(static_cast(data[*pos])); - for (size_t j = 0; j < sz; ++j) + /// The sequence length is determined by the first character, but we should always check it does not go beyond the buffer length. + for (size_t j = 0; j < sz && *pos < length; ++j) { token += data[*pos]; ++*pos; diff --git a/tests/queries/0_stateless/03784_msan_token_iterator.reference b/tests/queries/0_stateless/03784_msan_token_iterator.reference new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/tests/queries/0_stateless/03784_msan_token_iterator.sql b/tests/queries/0_stateless/03784_msan_token_iterator.sql new file mode 100644 index 0000000000000..2ca62efb42beb --- /dev/null +++ b/tests/queries/0_stateless/03784_msan_token_iterator.sql @@ -0,0 +1,4 @@ +DROP TABLE IF EXISTS t0; +CREATE TABLE t0 (c0 Int, INDEX i1 substring(sipHash128(c0), 1, 4) TYPE ngrambf_v1(1, 8, 1, 0)); +INSERT INTO TABLE t0 (c0) SELECT number FROM numbers(28); +DROP TABLE t0; From 7100b309f07831814de170075d207397bfc45cfe Mon Sep 17 00:00:00 2001 From: VighneshPath Date: Mon, 19 Oct 2026 13:06:42 +0530 Subject: [PATCH 2/2] Add deterministic test for `ngrambf_v1` reading past a truncated UTF-8 tail `05317_ngrambf_truncated_utf8_overrun` uses one row per granule, so the bytes after the first string are the second row. Without the fix the n-gram `ab\xE4xx` lands in the first granule's filter and `LIKE '%ab\xE4xx%'` reads `Granules: 1/2`; with the fix both granules are skipped (`0/2`). Verified on this branch: fails without the `NgramTokenExtractor::nextInString` change, passes with it. `03784_msan_token_iterator` from upstream relies on `create_table_empty_primary_key_by_default`, which the 25.8 test infrastructure does not set, so add `ENGINE = MergeTree ORDER BY ()`. Co-Authored-By: Claude Opus 5.5 --- .../0_stateless/03784_msan_token_iterator.sql | 2 +- ...5317_ngrambf_truncated_utf8_overrun.reference | 2 ++ .../05317_ngrambf_truncated_utf8_overrun.sql | 16 ++++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.reference create mode 100644 tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.sql diff --git a/tests/queries/0_stateless/03784_msan_token_iterator.sql b/tests/queries/0_stateless/03784_msan_token_iterator.sql index 2ca62efb42beb..925abbf847a8b 100644 --- a/tests/queries/0_stateless/03784_msan_token_iterator.sql +++ b/tests/queries/0_stateless/03784_msan_token_iterator.sql @@ -1,4 +1,4 @@ DROP TABLE IF EXISTS t0; -CREATE TABLE t0 (c0 Int, INDEX i1 substring(sipHash128(c0), 1, 4) TYPE ngrambf_v1(1, 8, 1, 0)); +CREATE TABLE t0 (c0 Int, INDEX i1 substring(sipHash128(c0), 1, 4) TYPE ngrambf_v1(1, 8, 1, 0)) ENGINE = MergeTree ORDER BY (); INSERT INTO TABLE t0 (c0) SELECT number FROM numbers(28); DROP TABLE t0; diff --git a/tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.reference b/tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.reference new file mode 100644 index 0000000000000..cd577cb0e1682 --- /dev/null +++ b/tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.reference @@ -0,0 +1,2 @@ +0 +Granules: 0/2 diff --git a/tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.sql b/tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.sql new file mode 100644 index 0000000000000..c7c9305be4b74 --- /dev/null +++ b/tests/queries/0_stateless/05317_ngrambf_truncated_utf8_overrun.sql @@ -0,0 +1,16 @@ +-- `ngrambf_v1` must not hash bytes past the end of a string that ends with a truncated UTF-8 sequence. +-- With `index_granularity = 1` the bytes after the first row are the second row, so a buggy index +-- contains the n-gram 'ab\xE4xx' and cannot skip the first granule for the search below. +SET use_skip_indexes = 1; +SET use_query_condition_cache = 0; + +DROP TABLE IF EXISTS t_ngram_truncated_utf8; +CREATE TABLE t_ngram_truncated_utf8 (s String, INDEX idx s TYPE ngrambf_v1(3, 256, 2, 0) GRANULARITY 1) +ENGINE = MergeTree ORDER BY tuple() SETTINGS index_granularity = 1, index_granularity_bytes = '10Mi'; +INSERT INTO t_ngram_truncated_utf8 SELECT arrayJoin(['ab\xE4', 'xx']); + +SELECT count() FROM t_ngram_truncated_utf8 WHERE s LIKE '%ab\xE4xx%'; +SELECT trimBoth(explain) FROM (EXPLAIN indexes = 1 SELECT count() FROM t_ngram_truncated_utf8 WHERE s LIKE '%ab\xE4xx%') +WHERE explain LIKE '%Granules:%'; + +DROP TABLE t_ngram_truncated_utf8;