From e228eed51b4f8943e5b740eee7d7aad09e5da3de Mon Sep 17 00:00:00 2001
From: chyezh <chyezh@outlook.com>
Date: Mon, 2 Sep 2024 17:49:37 +0800
Subject: [PATCH] fix: SkipIndex cause segment fault

Signed-off-by: chyezh <chyezh@outlook.com>
---
 internal/core/src/index/SkipIndex.cpp |  4 +-
 internal/core/src/index/SkipIndex.h   | 68 +++++++++++++--------------
 2 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/internal/core/src/index/SkipIndex.cpp b/internal/core/src/index/SkipIndex.cpp
index 11949db2bcf9c..20780a4bbc159 100644
--- a/internal/core/src/index/SkipIndex.cpp
+++ b/internal/core/src/index/SkipIndex.cpp
@@ -119,8 +119,8 @@ SkipIndex::LoadString(milvus::FieldId field_id,
     auto chunkMetrics = std::make_unique<FieldChunkMetrics>();
     if (num_rows > 0) {
         auto info = ProcessStringFieldMetrics(var_column);
-        chunkMetrics->min_ = Metrics(info.min_);
-        chunkMetrics->max_ = Metrics(info.max_);
+        chunkMetrics->min_ = Metrics(std::move(info.min_));
+        chunkMetrics->max_ = Metrics(std::move(info.max_));
         chunkMetrics->null_count_ = info.null_count_;
     }
 
diff --git a/internal/core/src/index/SkipIndex.h b/internal/core/src/index/SkipIndex.h
index b6b2b33da6305..21acbf5da5359 100644
--- a/internal/core/src/index/SkipIndex.h
+++ b/internal/core/src/index/SkipIndex.h
@@ -19,12 +19,13 @@
 
 namespace milvus {
 
-using Metrics = std::
-    variant<int8_t, int16_t, int32_t, int64_t, float, double, std::string_view>;
+using Metrics =
+    std::variant<int8_t, int16_t, int32_t, int64_t, float, double, std::string>;
 
+// MetricsDataType is used to avoid copy when get min/max value fro FieldChunkMetrics
 template <typename T>
 using MetricsDataType =
-    std::conditional_t<std::is_same_v<T, std::string>, std::string_view, T>;
+    std::conditional_t<std::is_same_v<T, std::string_view>, std::string, T>;
 
 struct FieldChunkMetrics {
     Metrics min_;
@@ -32,7 +33,23 @@ struct FieldChunkMetrics {
     bool hasValue_;
     int64_t null_count_;
 
-    FieldChunkMetrics() : hasValue_(false){};
+    FieldChunkMetrics() : hasValue_(false) {};
+
+    template <typename T>
+    std::pair<T, T>
+    GetMinMax() const {
+        AssertInfo(hasValue_,
+                   "GetMinMax should never be called when hasValue_ is false");
+        T lower_bound;
+        T upper_bound;
+        try {
+            lower_bound = std::get<MetricsDataType<T>>(min_);
+            upper_bound = std::get<MetricsDataType<T>>(max_);
+        } catch (const std::bad_variant_access& e) {
+            return {};
+        }
+        return {lower_bound, upper_bound};
+    }
 };
 
 class SkipIndex {
@@ -99,22 +116,6 @@ class SkipIndex {
         static constexpr bool value = isAllowedType && !isDisabledType;
     };
 
-    template <typename T>
-    std::pair<MetricsDataType<T>, MetricsDataType<T>>
-    GetMinMax(const FieldChunkMetrics& field_chunk_metrics) const {
-        MetricsDataType<T> lower_bound;
-        MetricsDataType<T> upper_bound;
-        try {
-            lower_bound =
-                std::get<MetricsDataType<T>>(field_chunk_metrics.min_);
-            upper_bound =
-                std::get<MetricsDataType<T>>(field_chunk_metrics.max_);
-        } catch (const std::bad_variant_access&) {
-            return {};
-        }
-        return {lower_bound, upper_bound};
-    }
-
     template <typename T>
     std::enable_if_t<SkipIndex::IsAllowedType<T>::value, bool>
     MinMaxUnaryFilter(const FieldChunkMetrics& field_chunk_metrics,
@@ -123,13 +124,11 @@ class SkipIndex {
         if (!field_chunk_metrics.hasValue_) {
             return false;
         }
-        std::pair<MetricsDataType<T>, MetricsDataType<T>> minMax =
-            GetMinMax<T>(field_chunk_metrics);
-        if (minMax.first == MetricsDataType<T>() ||
-            minMax.second == MetricsDataType<T>()) {
+        auto [lower_bound, upper_bound] = field_chunk_metrics.GetMinMax<T>();
+        if (lower_bound == T() || upper_bound == T()) {
             return false;
         }
-        return RangeShouldSkip<T>(val, minMax.first, minMax.second, op_type);
+        return RangeShouldSkip<T>(val, lower_bound, upper_bound, op_type);
     }
 
     template <typename T>
@@ -150,15 +149,11 @@ class SkipIndex {
         if (!field_chunk_metrics.hasValue_) {
             return false;
         }
-        std::pair<MetricsDataType<T>, MetricsDataType<T>> minMax =
-            GetMinMax<T>(field_chunk_metrics);
-        if (minMax.first == MetricsDataType<T>() ||
-            minMax.second == MetricsDataType<T>()) {
+        auto [lower_bound, upper_bound] = field_chunk_metrics.GetMinMax<T>();
+        if (lower_bound == T() || upper_bound == T()) {
             return false;
         }
         bool should_skip = false;
-        MetricsDataType<T> lower_bound = minMax.first;
-        MetricsDataType<T> upper_bound = minMax.second;
         if (lower_inclusive && upper_inclusive) {
             should_skip =
                 (lower_val > upper_bound) || (upper_val < lower_bound);
@@ -188,8 +183,8 @@ class SkipIndex {
     template <typename T>
     bool
     RangeShouldSkip(const T& value,
-                    const MetricsDataType<T> lower_bound,
-                    const MetricsDataType<T> upper_bound,
+                    const T& lower_bound,
+                    const T& upper_bound,
                     OpType op_type) const {
         bool should_skip = false;
         switch (op_type) {
@@ -267,7 +262,7 @@ class SkipIndex {
         return {minValue, maxValue, null_count};
     }
 
-    metricInfo<std::string_view>
+    metricInfo<std::string>
     ProcessStringFieldMetrics(
         const milvus::VariableColumn<std::string>& var_column) {
         int num_rows = var_column.NumRows();
@@ -281,7 +276,7 @@ class SkipIndex {
             break;
         }
         if (start > num_rows - 1) {
-            return {std::string_view(), std::string_view(), num_rows};
+            return {std::string(), std::string(), num_rows};
         }
         std::string_view min_string = var_column.RawAt(start);
         std::string_view max_string = var_column.RawAt(start);
@@ -299,7 +294,8 @@ class SkipIndex {
                 max_string = val;
             }
         }
-        return {min_string, max_string, null_count};
+        // The field data may be released, so we need to copy the string to avoid invalid memory access.
+        return {std::string(min_string), std::string(max_string), null_count};
     }
 
  private: