From bad10b33a049a30f3972ba19217e78cbe91a2506 Mon Sep 17 00:00:00 2001 From: lxy264173 Date: Fri, 5 Jun 2026 15:19:37 +0800 Subject: [PATCH] fix: Avoid signed overflow UB in BSI index and null-deref in bloom filter index --- .../bloomfilter/bloom_filter_file_index.cpp | 10 ++++++-- .../bsi/bit_slice_index_bitmap_file_index.cpp | 25 ++++++++++++++----- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/paimon/common/file_index/bloomfilter/bloom_filter_file_index.cpp b/src/paimon/common/file_index/bloomfilter/bloom_filter_file_index.cpp index 157361f32..06cf57343 100644 --- a/src/paimon/common/file_index/bloomfilter/bloom_filter_file_index.cpp +++ b/src/paimon/common/file_index/bloomfilter/bloom_filter_file_index.cpp @@ -78,9 +78,15 @@ BloomFilterFileIndexReader::BloomFilterFileIndexReader(const FastHash::HashFunct Result> BloomFilterFileIndexReader::VisitEqual( const Literal& literal) { + // This returns `Remain` to align with the current Java implementation in BF index, even though + // its predicate semantics are inconsistent here. In practice, equality tests in predicate + // evaluation always return false when the literal is null. See + // `null_false_leaf_binary_function.h`. + if (literal.IsNull()) { + return FileIndexResult::Remain(); + } int64_t hash = hash_function_(literal); - return literal.IsNull() || filter_.TestHash(hash) ? FileIndexResult::Remain() - : FileIndexResult::Skip(); + return filter_.TestHash(hash) ? FileIndexResult::Remain() : FileIndexResult::Skip(); } } // namespace paimon diff --git a/src/paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.cpp b/src/paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.cpp index 0bbd993ec..062700898 100644 --- a/src/paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.cpp +++ b/src/paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.cpp @@ -17,7 +17,9 @@ #include "paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.h" #include +#include #include +#include #include "fmt/format.h" #include "paimon/common/file_index/bsi/bit_slice_index_roaring_bitmap.h" @@ -31,7 +33,17 @@ #include "paimon/io/data_input_stream.h" #include "paimon/memory/bytes.h" #include "paimon/utils/roaring_bitmap32.h" +namespace { +// Safe absolute value for int64_t that avoids undefined behavior when value == INT64_MIN. +// This mirrors Java's Math.abs() wrapping semantics but produces the correct magnitude. +inline int64_t SafeAbs(int64_t value) { + if (value == INT64_MIN) { + return INT64_MIN; + } + return value < 0 ? -value : value; +} +} // namespace namespace paimon { class MemoryPool; @@ -156,7 +168,7 @@ Result> BitSliceIndexBitmapFileIndexReader::Vis if (value >= 0) { return reader->positive_->GreaterThan(value); } else { - PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->negative_->LessThan(-value)); + PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->negative_->LessThan(SafeAbs(value))); RoaringBitmap32 b2 = reader->positive_->IsNotNull(); b1 |= b2; return b1; @@ -173,7 +185,8 @@ Result> BitSliceIndexBitmapFileIndexReader::Vis if (value >= 0) { return reader->positive_->GreaterOrEqual(value); } else { - PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->negative_->LessOrEqual(-value)); + PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, + reader->negative_->LessOrEqual(SafeAbs(value))); RoaringBitmap32 b2 = reader->positive_->IsNotNull(); b1 |= b2; return b1; @@ -188,7 +201,7 @@ Result> BitSliceIndexBitmapFileIndexReader::Vis [literal = literal, reader = shared_from_this()]() -> Result { PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal)); if (value < 0) { - return reader->negative_->GreaterThan(-value); + return reader->negative_->GreaterThan(SafeAbs(value)); } else { PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->positive_->LessThan(value)); RoaringBitmap32 b2 = reader->negative_->IsNotNull(); @@ -204,7 +217,7 @@ Result> BitSliceIndexBitmapFileIndexReader::Vis [literal = literal, reader = shared_from_this()]() -> Result { PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal)); if (value < 0) { - return reader->negative_->GreaterOrEqual(-value); + return reader->negative_->GreaterOrEqual(SafeAbs(value)); } else { PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->positive_->LessOrEqual(value)); RoaringBitmap32 b2 = reader->negative_->IsNotNull(); @@ -234,7 +247,7 @@ Result> BitSliceIndexBitmapFileIndexReader::Vis PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal)); RoaringBitmap32 equal; if (value < 0) { - PAIMON_ASSIGN_OR_RAISE(equal, reader->negative_->Equal(-value)); + PAIMON_ASSIGN_OR_RAISE(equal, reader->negative_->Equal(SafeAbs(value))); } else { PAIMON_ASSIGN_OR_RAISE(equal, reader->positive_->Equal(value)); } @@ -257,7 +270,7 @@ Result> BitSliceIndexBitmapFileIndexReader::Vis PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal)); RoaringBitmap32 equal; if (value < 0) { - PAIMON_ASSIGN_OR_RAISE(equal, reader->negative_->Equal(-value)); + PAIMON_ASSIGN_OR_RAISE(equal, reader->negative_->Equal(SafeAbs(value))); } else { PAIMON_ASSIGN_OR_RAISE(equal, reader->positive_->Equal(value)); }