Skip to content

Commit 07118c5

Browse files
lxy-9602lxy-9602
authored
fix: Avoid signed overflow UB in BSI index and null-deref in bloom filter index (alibaba#340)
1 parent c5c0001 commit 07118c5

3 files changed

Lines changed: 107 additions & 20 deletions

File tree

‎src/paimon/common/file_index/bloomfilter/bloom_filter_file_index.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,15 @@ BloomFilterFileIndexReader::BloomFilterFileIndexReader(const FastHash::HashFunct
7878

7979
Result<std::shared_ptr<FileIndexResult>> BloomFilterFileIndexReader::VisitEqual(
8080
const Literal& literal) {
81+
// This returns `Remain` to align with the current Java implementation in BF index, even though
82+
// its predicate semantics are inconsistent here. In practice, equality tests in predicate
83+
// evaluation always return false when the literal is null. See
84+
// `null_false_leaf_binary_function.h`.
85+
if (literal.IsNull()) {
86+
return FileIndexResult::Remain();
87+
}
8188
int64_t hash = hash_function_(literal);
82-
return literal.IsNull() || filter_.TestHash(hash) ? FileIndexResult::Remain()
83-
: FileIndexResult::Skip();
89+
return filter_.TestHash(hash) ? FileIndexResult::Remain() : FileIndexResult::Skip();
8490
}
8591

8692
} // namespace paimon

‎src/paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.cpp‎

Lines changed: 53 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@
1717
#include "paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.h"
1818

1919
#include <cassert>
20+
#include <climits>
2021
#include <cstddef>
22+
#include <cstdint>
2123

2224
#include "fmt/format.h"
2325
#include "paimon/common/file_index/bsi/bit_slice_index_roaring_bitmap.h"
@@ -31,7 +33,17 @@
3133
#include "paimon/io/data_input_stream.h"
3234
#include "paimon/memory/bytes.h"
3335
#include "paimon/utils/roaring_bitmap32.h"
36+
namespace {
37+
// Safe absolute value for int64_t that avoids undefined behavior when value == INT64_MIN.
38+
// This mirrors Java's Math.abs() wrapping semantics.
39+
inline int64_t SafeAbs(int64_t value) {
40+
if (value == INT64_MIN) {
41+
return INT64_MIN;
42+
}
43+
return value < 0 ? -value : value;
44+
}
3445

46+
} // namespace
3547
namespace paimon {
3648
class MemoryPool;
3749

@@ -153,10 +165,14 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
153165
BitmapIndexResult::BitmapSupplier bitmap_supplier =
154166
[literal = literal, reader = shared_from_this()]() -> Result<RoaringBitmap32> {
155167
PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal));
156-
if (value >= 0) {
168+
if (value == INT64_MIN) {
169+
// Everything is greater than INT64_MIN (writer cannot store it)
170+
return RoaringBitmap32::Or(reader->positive_->IsNotNull(),
171+
reader->negative_->IsNotNull());
172+
} else if (value >= 0) {
157173
return reader->positive_->GreaterThan(value);
158174
} else {
159-
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->negative_->LessThan(-value));
175+
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->negative_->LessThan(SafeAbs(value)));
160176
RoaringBitmap32 b2 = reader->positive_->IsNotNull();
161177
b1 |= b2;
162178
return b1;
@@ -170,10 +186,15 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
170186
BitmapIndexResult::BitmapSupplier bitmap_supplier =
171187
[literal = literal, reader = shared_from_this()]() -> Result<RoaringBitmap32> {
172188
PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal));
173-
if (value >= 0) {
189+
if (value == INT64_MIN) {
190+
// All non-null rows satisfy x >= INT64_MIN
191+
return RoaringBitmap32::Or(reader->positive_->IsNotNull(),
192+
reader->negative_->IsNotNull());
193+
} else if (value >= 0) {
174194
return reader->positive_->GreaterOrEqual(value);
175195
} else {
176-
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->negative_->LessOrEqual(-value));
196+
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1,
197+
reader->negative_->LessOrEqual(SafeAbs(value)));
177198
RoaringBitmap32 b2 = reader->positive_->IsNotNull();
178199
b1 |= b2;
179200
return b1;
@@ -187,8 +208,11 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
187208
BitmapIndexResult::BitmapSupplier bitmap_supplier =
188209
[literal = literal, reader = shared_from_this()]() -> Result<RoaringBitmap32> {
189210
PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal));
190-
if (value < 0) {
191-
return reader->negative_->GreaterThan(-value);
211+
if (value == INT64_MIN) {
212+
// Nothing is less than INT64_MIN
213+
return RoaringBitmap32();
214+
} else if (value < 0) {
215+
return reader->negative_->GreaterThan(SafeAbs(value));
192216
} else {
193217
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->positive_->LessThan(value));
194218
RoaringBitmap32 b2 = reader->negative_->IsNotNull();
@@ -203,8 +227,11 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
203227
BitmapIndexResult::BitmapSupplier bitmap_supplier =
204228
[literal = literal, reader = shared_from_this()]() -> Result<RoaringBitmap32> {
205229
PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal));
206-
if (value < 0) {
207-
return reader->negative_->GreaterOrEqual(-value);
230+
if (value == INT64_MIN) {
231+
// Writer cannot store INT64_MIN, so no row can match
232+
return RoaringBitmap32();
233+
} else if (value < 0) {
234+
return reader->negative_->GreaterOrEqual(SafeAbs(value));
208235
} else {
209236
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->positive_->LessOrEqual(value));
210237
RoaringBitmap32 b2 = reader->negative_->IsNotNull();
@@ -232,13 +259,17 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
232259
result_bitmaps.reserve(literals.size());
233260
for (const auto& literal : literals) {
234261
PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal));
235-
RoaringBitmap32 equal;
236-
if (value < 0) {
237-
PAIMON_ASSIGN_OR_RAISE(equal, reader->negative_->Equal(-value));
262+
if (value == INT64_MIN) {
263+
// Writer cannot store INT64_MIN, so no row can match it
264+
result_bitmaps.emplace_back();
265+
} else if (value < 0) {
266+
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 equal,
267+
reader->negative_->Equal(SafeAbs(value)));
268+
result_bitmaps.emplace_back(std::move(equal));
238269
} else {
239-
PAIMON_ASSIGN_OR_RAISE(equal, reader->positive_->Equal(value));
270+
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 equal, reader->positive_->Equal(value));
271+
result_bitmaps.emplace_back(std::move(equal));
240272
}
241-
result_bitmaps.emplace_back(std::move(equal));
242273
}
243274
return RoaringBitmap32::FastUnion(result_bitmaps);
244275
};
@@ -255,13 +286,17 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
255286
result_bitmaps.reserve(literals.size());
256287
for (const auto& literal : literals) {
257288
PAIMON_ASSIGN_OR_RAISE(int64_t value, reader->value_mapper_(literal));
258-
RoaringBitmap32 equal;
259-
if (value < 0) {
260-
PAIMON_ASSIGN_OR_RAISE(equal, reader->negative_->Equal(-value));
289+
if (value == INT64_MIN) {
290+
// Writer cannot store INT64_MIN, so no row can match it
291+
result_bitmaps.emplace_back();
292+
} else if (value < 0) {
293+
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 equal,
294+
reader->negative_->Equal(SafeAbs(value)));
295+
result_bitmaps.emplace_back(std::move(equal));
261296
} else {
262-
PAIMON_ASSIGN_OR_RAISE(equal, reader->positive_->Equal(value));
297+
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 equal, reader->positive_->Equal(value));
298+
result_bitmaps.emplace_back(std::move(equal));
263299
}
264-
result_bitmaps.emplace_back(std::move(equal));
265300
}
266301
auto in = RoaringBitmap32::FastUnion(result_bitmaps);
267302
ebm -= in;

‎src/paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index_test.cpp‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#include "paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.h"
1818

19+
#include <cstdint>
1920
#include <utility>
2021

2122
#include "gtest/gtest.h"
@@ -385,4 +386,49 @@ TEST_F(BitSliceIndexBitmapIndexReaderTest, TestUnInvalidType) {
385386
"BitSliceIndexBitmapFileIndex only support TINYINT/SMALLINT/INT/BIGINT/DATE");
386387
}
387388

389+
TEST_F(BitSliceIndexBitmapIndexReaderTest, TestReaderPredicatePruningWithInt64Min) {
390+
// Reuse TestPrimitiveType's index bytes.
391+
// data: null, 1, null, 2, -1 (non-null rows: {1, 3, 4})
392+
std::vector<char> index_bytes = {
393+
1, 0, 0, 0, 5, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 58,
394+
48, 0, 0, 1, 0, 0, 0, 0, 0, 1, 0, 16, 0, 0, 0, 1, 0, 3, 0, 0, 0, 0, 2, 58,
395+
48, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 16, 0, 0, 0, 1, 0, 58, 48, 0, 0, 1, 0, 0,
396+
0, 0, 0, 0, 0, 16, 0, 0, 0, 3, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
397+
0, 0, 0, 0, 1, 58, 48, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 16, 0, 0, 0, 4, 0, 0,
398+
0, 0, 1, 58, 48, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 16, 0, 0, 0, 4, 0};
399+
auto input_stream =
400+
std::make_shared<ByteArrayInputStream>(index_bytes.data(), index_bytes.size());
401+
BitSliceIndexBitmapFileIndex file_index({});
402+
ASSERT_OK_AND_ASSIGN(
403+
auto reader,
404+
file_index.CreateReader(CreateArrowSchema(arrow::int64()).get(),
405+
/*start=*/0, /*length=*/index_bytes.size(), input_stream, pool_));
406+
407+
// x > INT64_MIN: all non-null rows (writer cannot store INT64_MIN)
408+
CheckResult(reader->VisitGreaterThan(Literal(INT64_MIN)).value(), {1, 3, 4});
409+
410+
// x >= INT64_MIN: all non-null rows
411+
CheckResult(reader->VisitGreaterOrEqual(Literal(INT64_MIN)).value(), {1, 3, 4});
412+
413+
// x < INT64_MIN: empty
414+
CheckResult(reader->VisitLessThan(Literal(INT64_MIN)).value(), {});
415+
416+
// x <= INT64_MIN: empty (no row has INT64_MIN)
417+
CheckResult(reader->VisitLessOrEqual(Literal(INT64_MIN)).value(), {});
418+
419+
// x == INT64_MIN: empty
420+
CheckResult(reader->VisitEqual(Literal(INT64_MIN)).value(), {});
421+
422+
// x != INT64_MIN: all non-null rows
423+
CheckResult(reader->VisitNotEqual(Literal(INT64_MIN)).value(), {1, 3, 4});
424+
425+
// x IN (INT64_MIN, -1): only -1 matches → row 4
426+
CheckResult(reader->VisitIn({Literal(INT64_MIN), Literal(static_cast<int64_t>(-1))}).value(),
427+
{4});
428+
429+
// x NOT IN (INT64_MIN, -1): all non-null except row 4
430+
CheckResult(reader->VisitNotIn({Literal(INT64_MIN), Literal(static_cast<int64_t>(-1))}).value(),
431+
{1, 3});
432+
}
433+
388434
} // namespace paimon::test

0 commit comments

Comments
 (0)