Skip to content

fix: Avoid signed overflow UB in BSI index and null-deref in bloom filter index #340

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lxy-9602 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Avoid signed overflow UB in BSI index and null-deref in bloom filter index #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/paimon/common/file_index/bloomfilter/bloom_filter_file_index.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,15 @@ BloomFilterFileIndexReader::BloomFilterFileIndexReader(const FastHash::HashFunct

Result<std::shared_ptr<FileIndexResult>> 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
25 changes: 19 additions & 6 deletions src/paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
#include "paimon/common/file_index/bsi/bit_slice_index_bitmap_file_index.h"

#include <cassert>
#include <climits>
#include <cstddef>
#include <cstdint>

#include "fmt/format.h"
#include "paimon/common/file_index/bsi/bit_slice_index_roaring_bitmap.h"
Expand All @@ -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;
}
Comment on lines +37 to +44

} // namespace
namespace paimon {
class MemoryPool;

Expand Down Expand Up @@ -156,7 +168,7 @@ Result<std::shared_ptr<FileIndexResult>> 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;
Comment on lines 170 to 174
Expand All @@ -173,7 +185,8 @@ Result<std::shared_ptr<FileIndexResult>> 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;
Comment on lines 187 to 192
Expand All @@ -188,7 +201,7 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
[literal = literal, reader = shared_from_this()]() -> Result<RoaringBitmap32> {
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 {
Comment on lines 202 to 205
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->positive_->LessThan(value));
RoaringBitmap32 b2 = reader->negative_->IsNotNull();
Expand All @@ -204,7 +217,7 @@ Result<std::shared_ptr<FileIndexResult>> BitSliceIndexBitmapFileIndexReader::Vis
[literal = literal, reader = shared_from_this()]() -> Result<RoaringBitmap32> {
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 {
Comment on lines 218 to 221
PAIMON_ASSIGN_OR_RAISE(RoaringBitmap32 b1, reader->positive_->LessOrEqual(value));
RoaringBitmap32 b2 = reader->negative_->IsNotNull();
Expand Down Expand Up @@ -234,7 +247,7 @@ Result<std::shared_ptr<FileIndexResult>> 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));
}
Expand All @@ -257,7 +270,7 @@ Result<std::shared_ptr<FileIndexResult>> 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));
}
Expand Down
Loading