Fix QueryConditionCache OOB write on part-name/layout reuse (#2342)

src/Common/ProfileEvents.cpp

@@ -96,6 +96,10 @@
     M(VectorSimilarityIndexCacheWeightLost, "Approximate number of bytes evicted from the vector index cache.", ValueType::Number) \
     M(QueryConditionCacheHits, "Number of times an entry has been found in the query condition cache (and reading of marks can be skipped). Only updated for SELECT queries with SETTING use_query_condition_cache = 1.", ValueType::Number) \
     M(QueryConditionCacheMisses, "Number of times an entry has not been found in the query condition cache (and reading of mark cannot be skipped). Only updated for SELECT queries with SETTING use_query_condition_cache = 1.", ValueType::Number) \
+    M(QueryConditionCacheLayoutMismatch, "Number of times a query condition cache entry was skipped "  \
+      "on write or read because its mark layout (marks_count/has_final_mark) did not match the "       \
+      "current part's layout (see Altinity/ClickHouse#2342).",                                         \
+      ValueType::Number) \
     M(QueryCacheHits, "Number of times a query result has been found in the query cache (and query computation was avoided). Only updated for SELECT queries with SETTING use_query_cache = 1.", ValueType::Number) \
     M(QueryCacheMisses, "Number of times a query result has not been found in the query cache (and required query computation). Only updated for SELECT queries with SETTING use_query_cache = 1.", ValueType::Number) \
     M(CreatedReadBufferOrdinary, "Number of times ordinary read buffer was created for reading data (while choosing among other read methods).", ValueType::Number) \

src/Interpreters/Cache/QueryConditionCache.cpp

@@ -9,6 +9,7 @@ namespace ProfileEvents
 {
     extern const Event QueryConditionCacheHits;
     extern const Event QueryConditionCacheMisses;
+    extern const Event QueryConditionCacheLayoutMismatch;
 }
 
 namespace CurrentMetrics
@@ -24,7 +25,9 @@ bool QueryConditionCache::Key::operator==(const Key & other) const
 {
     return table_id == other.table_id
         && part_name == other.part_name
-        && condition_hash == other.condition_hash;
+        && condition_hash == other.condition_hash
+        && marks_count == other.marks_count
+        && has_final_mark == other.has_final_mark;
 }
 
 size_t QueryConditionCache::KeyHasher::operator()(const Key & key) const
@@ -33,6 +36,8 @@ size_t QueryConditionCache::KeyHasher::operator()(const Key & key) const
     hash.update(key.table_id);
     hash.update(key.part_name);
     hash.update(key.condition_hash);
+    hash.update(key.marks_count);
+    hash.update(key.has_final_mark);
     return hash.get64();
 }
 
@@ -52,7 +57,45 @@ void QueryConditionCache::write(
     const UUID & table_id, const String & part_name, UInt64 condition_hash, const String & condition,
     const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark)
 {
-    Key key = {table_id, part_name, condition_hash, condition};
+    if (has_final_mark && marks_count == 0)
+    {
+        ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch);
+        LOG_ERROR(
+            logger,
+            "Query condition cache write for table_id: {}, part_name: {}, condition_hash: {}: "
+            "has_final_mark is set but marks_count is 0, which is not a valid mark layout. "
+            "Skipping cache update for this entry. See Altinity/ClickHouse#2342.",
+            table_id, part_name, condition_hash);
+        return;
+    }
+
+    for (const auto & mark_range : mark_ranges)
+    {
+        if (mark_range.begin > mark_range.end || mark_range.end > marks_count)
+        {
+            ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch);
+            LOG_ERROR(
+                logger,
+                "Query condition cache write for table_id: {}, part_name: {}, condition_hash: {}: "
+                "invalid mark range [{}, {}) for entry with {} marks. Skipping cache update for this "
+                "entry. See Altinity/ClickHouse#2342.",
+                table_id, part_name, condition_hash, mark_range.begin, mark_range.end, marks_count);
+            return;
+        }
+    }
+
+    /// NOTE(#2342): marks_count and has_final_mark are now part of Key (see QueryConditionCache.h).
+    /// Before this change, Key was {table_id, part_name, condition_hash} only. A part_name can be
+    /// reused after its underlying data (and therefore its mark layout) has changed -- e.g. after a
+    /// mutation, a merge that produces a part with a name collision under certain replay scenarios,
+    /// or (per the linked issue) some other part-lifecycle path we have not fully identified.
+    /// getOrSet() below could then return a pre-existing Entry sized for the OLD layout, which the
+    /// code proceeded to index using the NEW marks_count/mark_ranges. On std::vector<bool>'s
+    /// bit-packed storage this is an out-of-bounds *write*, not just a read -- see the (*) fill()
+    /// and matching_marks[marks_count - 1] writes below. Keying on the layout as well eliminates the
+    /// possibility of this mismatch entirely; the alternative (validate-before-use, kept as a defense
+    /// in depth below) only stops the write after the mismatched Entry has already been returned.
+    Key key = {table_id, part_name, condition_hash, marks_count, has_final_mark, condition};
 
     auto load_func = [&](){ return std::make_shared<Entry>(marks_count); };
     auto [entry, inserted] = cache.getOrSet(key, load_func);
@@ -61,6 +104,31 @@ void QueryConditionCache::write(
     {
         std::shared_lock shared_lock(entry->mutex); /// cheap
 
+        /// NOTE(#2342): defense in depth. With marks_count in the Key, this branch should now be
+        /// unreachable in normal operation -- cache.getOrSet() cannot return an Entry keyed to a
+        /// different marks_count than the one just constructed. If this ever fires, it means either
+        /// (a) a hash collision between two distinct Keys (extremely unlikely with SipHash64 over
+        ///     table_id + part_name + condition_hash + marks_count + has_final_mark), or
+        /// (b) a residual bug in Key equality/hashing, or
+        /// (c) the underlying corruption in #2342 has already occurred elsewhere and this Entry's
+        ///     bookkeeping is not trustworthy regardless of what the key comparison says.
+        /// Silently returning (skip the cache write) is the only safe response here -- do NOT throw
+        /// from inside the query pipeline for what may be a caching-layer inconsistency, and do NOT
+        /// proceed to index a vector we now know is a mismatched size. This is intentionally the
+        /// last line of defense, not the fix: the fix is the Key change above.
+        chassert(entry->matching_marks.size() == marks_count);
+        if (entry->matching_marks.size() != marks_count)
+        {
+            ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch);
+            LOG_ERROR(
+                logger,
+                "Query condition cache layout mismatch on write for table_id: {}, part_name: {}, "
+                "condition_hash: {}: entry has {} marks, current task has {} marks (has_final_mark: {}). "
+                "Skipping cache update for this entry. See Altinity/ClickHouse#2342.",
+                table_id, part_name, condition_hash, entry->matching_marks.size(), marks_count, has_final_mark);
+            return;
+        }
+
         bool need_not_update_marks = true;
         for (const auto & mark_range : mark_ranges)
         {
@@ -82,7 +150,23 @@ void QueryConditionCache::write(
     {
         std::lock_guard lock(entry->mutex); /// (*)
 
-        chassert(marks_count == entry->matching_marks.size());
+        /// NOTE(#2342): same defense-in-depth check as above, now under the exclusive lock. Kept
+        /// deliberately duplicated rather than factored into a helper: the shared_lock check above
+        /// is an optimization (avoid taking the exclusive lock), and the entry could theoretically
+        /// change between releasing the shared lock and acquiring the exclusive one in some future
+        /// refactor. Re-checking here costs nothing and removes that assumption.
+        chassert(entry->matching_marks.size() == marks_count);
+        if (entry->matching_marks.size() != marks_count)
+        {
+            ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch);
+            LOG_ERROR(
+                logger,
+                "Query condition cache layout mismatch on write (exclusive path) for table_id: {}, "
+                "part_name: {}, condition_hash: {}: entry has {} marks, current task has {} marks "
+                "(has_final_mark: {}). Skipping cache update for this entry. See Altinity/ClickHouse#2342.",
+                table_id, part_name, condition_hash, entry->matching_marks.size(), marks_count, has_final_mark);
+            return;
+        }
 
         /// The input mark ranges are the areas which the scan can skip later on.
         for (const auto & mark_range : mark_ranges)
@@ -104,23 +188,46 @@ void QueryConditionCache::write(
         has_final_mark);
 }
 
-std::optional<QueryConditionCache::MatchingMarks> QueryConditionCache::read(const UUID & table_id, const String & part_name, UInt64 condition_hash)
+std::optional<QueryConditionCache::MatchingMarks> QueryConditionCache::read(
+    const UUID & table_id, const String & part_name, UInt64 condition_hash, size_t marks_count, bool has_final_mark)
 {
-    Key key = {table_id, part_name, condition_hash, ""};
+    Key key = {table_id, part_name, condition_hash, marks_count, has_final_mark, ""};
 
     if (auto entry = cache.get(key))
     {
-        ProfileEvents::increment(ProfileEvents::QueryConditionCacheHits);
-
         std::shared_lock lock(entry->mutex);
 
+        /// NOTE(#2342): defense in depth, mirrors the write-path checks above. On the read path a
+        /// mismatch is comparatively low-risk -- the caller only reads matching_marks, it does not
+        /// index into it with the *caller's* marks_count anywhere we've audited (see
+        /// MergeTreeDataSelectExecutor::filterPartsByQueryConditionCache, which indexes with
+        /// mark_range.begin/end drawn from part_with_ranges.ranges, i.e. the part's own state at scan
+        /// time, not from this returned vector's size). We still guard here because we have NOT
+        /// confirmed every caller across all supported versions respects that invariant, and because
+        /// returning a mismatched-size MatchingMarks to a caller that isn't expecting one is exactly
+        /// the kind of latent hazard this whole issue is about. Treat as a cache miss on mismatch.
+        chassert(entry->matching_marks.size() == marks_count);
+        if (entry->matching_marks.size() != marks_count)
+        {
+            ProfileEvents::increment(ProfileEvents::QueryConditionCacheLayoutMismatch);
+            LOG_ERROR(
+                logger,
+                "Query condition cache layout mismatch on read for table_id: {}, part_name: {}, "
+                "condition_hash: {}: entry has {} marks, requested {} marks (has_final_mark: {}). "
+                "Treating as a cache miss. See Altinity/ClickHouse#2342.",
+                table_id, part_name, condition_hash, entry->matching_marks.size(), marks_count, has_final_mark);
+            ProfileEvents::increment(ProfileEvents::QueryConditionCacheMisses);
+            return {};
+        }
+
         LOG_TEST(
             logger,
             "Read entry for table_uuid: {}, part: {}, condition_hash: {}",
             table_id,
             part_name,
             condition_hash);
 
+        ProfileEvents::increment(ProfileEvents::QueryConditionCacheHits);
         return {entry->matching_marks};
     }
     else

src/Interpreters/Cache/QueryConditionCache.h

@@ -34,6 +34,11 @@ class QueryConditionCache
         const String part_name;
         const UInt64 condition_hash;
 
+        /// A part name can be reused after its data has been replaced. The mark layout is
+        /// part of the cached value's shape, so it must be part of the cache identity too.
+        const size_t marks_count;
+        const bool has_final_mark;
+
         /// -- Additional members, conceptually not part of the key. Only included for pretty-printing
         ///    in system.query_condition_cache:
         const String condition;
@@ -80,7 +85,12 @@ class QueryConditionCache
         const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark);
 
     /// Check the cache if it contains an entry for the given table + part id and predicate hash.
-    std::optional<MatchingMarks> read(const UUID & table_id, const String & part_name, UInt64 condition_hash);
+    /// marks_count/has_final_mark describe the caller's current mark layout for the part; they are
+    /// part of the cache key and are also checked against the found entry's stored layout, since a
+    /// part_name can be reused with a different layout after the underlying data has changed. On a
+    /// layout mismatch, the entry is treated as a cache miss (see Altinity/ClickHouse#2342).
+    std::optional<MatchingMarks> read(
+        const UUID & table_id, const String & part_name, UInt64 condition_hash, size_t marks_count, bool has_final_mark);
 
     /// For debugging and system tables
     std::vector<QueryConditionCache::Cache::KeyMapped> dump() const;

src/Interpreters/Cache/tests/gtest_query_condition_cache.cpp

@@ -0,0 +1,48 @@
+#include <Interpreters/Cache/QueryConditionCache.h>
+#include <base/UUID.h>
+#include <base/unit.h>
+#include <gtest/gtest.h>
+
+using namespace DB;
+
+TEST(QueryConditionCache, KeepsEntriesWithDifferentMarkLayoutsSeparate)
+{
+    QueryConditionCache cache("LRU", 1_MiB, 0.5);
+    const UUID table_id(1);
+    const String part_name = "part";
+    constexpr UInt64 condition_hash = 42;
+
+    MarkRanges one_mark;
+    one_mark.emplace_back(0, 1);
+    cache.write(table_id, part_name, condition_hash, "", one_mark, 1, false);
+
+    MarkRanges two_marks;
+    two_marks.emplace_back(1, 2);
+    cache.write(table_id, part_name, condition_hash, "", two_marks, 2, false);
+
+    auto first_layout = cache.read(table_id, part_name, condition_hash, 1, false);
+    ASSERT_TRUE(first_layout);
+    ASSERT_EQ(first_layout->size(), 1);
+    EXPECT_FALSE((*first_layout)[0]);
+
+    auto second_layout = cache.read(table_id, part_name, condition_hash, 2, false);
+    ASSERT_TRUE(second_layout);
+    ASSERT_EQ(second_layout->size(), 2);
+    EXPECT_TRUE((*second_layout)[0]);
+    EXPECT_FALSE((*second_layout)[1]);
+}
+
+TEST(QueryConditionCache, RejectsRangesOutsideEntryMarkLayout)
+{
+    QueryConditionCache cache("LRU", 1_MiB, 0.5);
+    MarkRanges invalid_range;
+    invalid_range.emplace_back(0, 2);
+
+    /// An invalid mark range (out of bounds for the given marks_count) is a caller bug, but write()
+    /// logs and skips rather than throwing from inside the query pipeline (see Altinity/ClickHouse#2342).
+    EXPECT_NO_THROW(cache.write(UUID(1), "part", 42, "", invalid_range, 1, false));
+
+    /// Nothing should have been cached for this key.
+    auto result = cache.read(UUID(1), "part", 42, 1, false);
+    EXPECT_FALSE(result);
+}

src/Interpreters/Cache/tests/gtest_query_condition_cache_defense_in_depth.cpp

@@ -0,0 +1,90 @@
+/// These tests exercise the defense-in-depth paths in QueryConditionCache.cpp (the log-and-skip
+/// checks kept alongside the Key fix). They cannot currently trigger a *genuine* Key-hash collision
+/// (SipHash64 over table_id + part_name + condition_hash + marks_count + has_final_mark is not
+/// practically collidable in a unit test), so "mismatch" here is necessarily synthetic. These tests
+/// document and lock in the *behavior* (log + skip, no throw, no OOB access) rather than proving the
+/// mismatch is unreachable in production -- that claim rests on the Key change itself, not on these
+/// tests.
+
+#include <Interpreters/Cache/QueryConditionCache.h>
+#include <base/UUID.h>
+#include <base/unit.h>
+#include <gtest/gtest.h>
+
+#include <thread>
+#include <vector>
+
+using namespace DB;
+
+/// A part_name reused with a completely different mark count (simulating the part-lifecycle
+/// scenario this issue hypothesizes -- e.g. a part name recycled after data changed underneath it)
+/// must not throw and must not corrupt either entry. With the Key fix this is definitionally two
+/// separate keys, so this mostly re-confirms the same property as the test above from a different
+/// angle (larger layout difference, has_final_mark also varied).
+TEST(QueryConditionCache, ReusedPartNameWithFinalMarkDoesNotThrowOrCorrupt)
+{
+    QueryConditionCache cache("LRU", 1_MiB, 0.5);
+    const UUID table_id(1);
+    const String part_name = "reused_part";
+    constexpr UInt64 condition_hash = 7;
+
+    MarkRanges small_ranges;
+    small_ranges.emplace_back(0, 3);
+    EXPECT_NO_THROW(cache.write(table_id, part_name, condition_hash, "", small_ranges, 3, false));
+
+    /// Simulates the same part_name later associated with a much larger, differently-shaped part
+    /// (e.g. after a merge or mutation), including a final mark this time.
+    MarkRanges large_ranges;
+    large_ranges.emplace_back(0, 50);
+    large_ranges.emplace_back(60, 99);
+    EXPECT_NO_THROW(cache.write(table_id, part_name, condition_hash, "", large_ranges, 100, true));
+
+    auto small_layout = cache.read(table_id, part_name, condition_hash, 3, false);
+    ASSERT_TRUE(small_layout);
+    ASSERT_EQ(small_layout->size(), 3);
+    /// small_ranges = [0, 3) covers the whole entry, so every mark must have been forced to false
+    /// ("must scan"); a corrupted entry (e.g. bleeding over from the differently-shaped large write)
+    /// could instead leave marks at their initial true default or some other value.
+    EXPECT_FALSE((*small_layout)[0]);
+    EXPECT_FALSE((*small_layout)[1]);
+    EXPECT_FALSE((*small_layout)[2]);
+
+    auto large_layout = cache.read(table_id, part_name, condition_hash, 100, true);
+    ASSERT_TRUE(large_layout);
+    ASSERT_EQ(large_layout->size(), 100);
+    /// has_final_mark=true means marks_count-1 (index 99) must be forced to false (i.e. "must scan").
+    EXPECT_FALSE((*large_layout)[99]);
+}
+
+/// Concurrent-scan case referenced in the Entry class comment (*): multiple threads writing
+/// overlapping/adjacent ranges for the same key must not race or corrupt the entry. This does not
+/// specifically target #2342, but is adjacent: the ticket raises prefer_localhost_replica's effect
+/// on crash rate as unexplained by the layout-mismatch fix alone, and increased local-read
+/// concurrency against this cache is one candidate mechanism worth having coverage for.
+TEST(QueryConditionCache, ConcurrentWritesToSameKeyDoNotCorruptEntry)
+{
+    QueryConditionCache cache("LRU", 1_MiB, 0.5);
+    const UUID table_id(2);
+    const String part_name = "concurrent_part";
+    constexpr UInt64 condition_hash = 99;
+    constexpr size_t marks_count = 1000;
+
+    std::vector<std::thread> threads;
+    for (size_t t = 0; t < 8; ++t)
+    {
+        threads.emplace_back([&, t]()
+        {
+            MarkRanges ranges;
+            ranges.emplace_back(t * 100, (t + 1) * 100);
+            cache.write(table_id, part_name, condition_hash, "", ranges, marks_count, false);
+        });
+    }
+    for (auto & th : threads)
+        th.join();
+
+    auto result = cache.read(table_id, part_name, condition_hash, marks_count, false);
+    ASSERT_TRUE(result);
+    ASSERT_EQ(result->size(), marks_count);
+    for (size_t i = 0; i < 800; ++i)
+        EXPECT_FALSE((*result)[i]) << "mark " << i << " should have been marked non-matching";
+}