This is an automated email from the ASF dual-hosted git repository.
airborne12 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 70cdb58e940 [fix](be) Reject overflowing lengths and indices when
decoding SNII integers (#69726)
70cdb58e940 is described below
commit 70cdb58e9403d6a46ffead0571ea27d272b76dc5
Author: Jack <[email protected]>
AuthorDate: Thu Oct 29 09:05:38 2026 +0800
[fix](be) Reject overflowing lengths and indices when decoding SNII
integers (#69726)
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Three decode primitives of the SNII index format take
numbers read from the file and do arithmetic on them without checking
for overflow.
- The FRQ prelude added the untrusted `sbdir_len` to the header length
before comparing it with the buffer, and summed the super-block lengths
into the window region length unchecked. A crafted length wraps the sum
to a small value, so the bounds check passes while a Slice of a huge
length is cut from a buffer that holds no such bytes, and the block
decoder then reads past the end of it. `memory_required` already
compared `sbdir_len` with the remaining bytes before calling
`verify_covered_crc`, but `open` did not. The overflow check now lives
in `verify_covered_crc`, which both call, the super-block sum uses the
file's own `checked_add_u64`, and the final region check compares
against the room left instead of adding to the start offset.
- `decode_varint64` accepted a tenth byte with payload above bit 63. The
extra bits were shifted out, and the encoding decoded to a silently
truncated value. The check sits on the terminating byte, so the loop
over the continuation bytes is unchanged.
- `pfor_decode` and `pfor_skip` added each exception index delta to a
32-bit running index and tested the sum afterwards, so a delta could
wrap the index back into the block and be accepted. The running index is
now 64 bits wide: the index is below the block size before every add and
a delta is at most 2^32 - 1, so the sum cannot wrap.
Nothing here is reachable from data the writers produce. The checks make
a damaged or crafted index file fail with a corruption status instead of
being decoded into a wrong value or an out-of-bounds read.
---
.../storage/index/inverted/snii/encoding/pfor.cpp | 6 +-
.../index/inverted/snii/encoding/varint.cpp | 7 +++
.../index/inverted/snii/format/frq_prelude.cpp | 17 ++++--
.../index/inverted/snii/encoding/pfor_test.cpp | 48 ++++++++++++++++
.../index/inverted/snii/encoding/varint_test.cpp | 23 ++++++++
.../index/inverted/snii/snii_frq_prelude_test.cpp | 65 ++++++++++++++++++++++
6 files changed, 160 insertions(+), 6 deletions(-)
diff --git a/be/src/storage/index/inverted/snii/encoding/pfor.cpp
b/be/src/storage/index/inverted/snii/encoding/pfor.cpp
index 30e6dc95b8a..f8ad4f15959 100644
--- a/be/src/storage/index/inverted/snii/encoding/pfor.cpp
+++ b/be/src/storage/index/inverted/snii/encoding/pfor.cpp
@@ -419,7 +419,9 @@ Status pfor_decode(ByteSource* src, size_t n, uint32_t*
out) {
uint32_t n_exc;
RETURN_IF_ERROR(src->get_varint32(&n_exc));
RETURN_IF_ERROR(bitunpack(src, n, w, out));
- uint32_t idx = 0;
+ // 64 bits wide: a delta from the file is at most 2^32 - 1 and idx is
below n before every add, so the
+ // sum cannot wrap back into [0, n) the way a 32-bit one can.
+ uint64_t idx = 0;
for (uint32_t i = 0; i < n_exc; ++i) {
uint32_t d, val;
RETURN_IF_ERROR(src->get_varint32(&d));
@@ -442,7 +444,7 @@ Status pfor_skip(ByteSource* src, size_t n) {
const size_t packed = (static_cast<size_t>(w) * n + 7) / 8;
Slice unused;
RETURN_IF_ERROR(src->get_bytes(packed, &unused));
- uint32_t idx = 0;
+ uint64_t idx = 0; // wide for the same reason as in pfor_decode
for (uint32_t i = 0; i < n_exc; ++i) {
uint32_t d = 0;
uint32_t val = 0;
diff --git a/be/src/storage/index/inverted/snii/encoding/varint.cpp
b/be/src/storage/index/inverted/snii/encoding/varint.cpp
index 183b24e7b96..6a760082f42 100644
--- a/be/src/storage/index/inverted/snii/encoding/varint.cpp
+++ b/be/src/storage/index/inverted/snii/encoding/varint.cpp
@@ -49,6 +49,13 @@ Status decode_varint64(const uint8_t* p, const uint8_t* end,
uint64_t* v, const
uint8_t b = *p++;
result |= static_cast<uint64_t>(b & 0x7F) << shift;
if ((b & 0x80) == 0) {
+ // The tenth byte only has room for bit 63. A higher payload bit
was shifted out of the
+ // result above, and the encoding would decode to a silently
truncated value. Checked
+ // here, on the last byte, so the loop over the continuation bytes
stays as it was.
+ if (shift == 63 && b > 1) {
+ return Status::Error<ErrorCode::INVERTED_INDEX_FILE_CORRUPTED,
false>(
+ "varint64 overflow");
+ }
*v = result;
*next = p;
return Status::OK();
diff --git a/be/src/storage/index/inverted/snii/format/frq_prelude.cpp
b/be/src/storage/index/inverted/snii/format/frq_prelude.cpp
index 2393c83a80e..7950262fe64 100644
--- a/be/src/storage/index/inverted/snii/format/frq_prelude.cpp
+++ b/be/src/storage/index/inverted/snii/format/frq_prelude.cpp
@@ -213,11 +213,15 @@ struct Header {
// Verifies the trailing crc covers [start of buffer .. end of
super_block_dir].
// covered_len = header bytes (up to and including sbdir_len) + sbdir_len.
Status verify_covered_crc(Slice prelude, size_t header_end, uint64_t
sbdir_len) {
- const size_t covered = header_end + static_cast<size_t>(sbdir_len);
- if (covered + sizeof(uint32_t) > prelude.size()) {
+ // sbdir_len is untrusted: a plain sum could wrap to a small offset that
points back into the header.
+ uint64_t covered_len = 0;
+ RETURN_IF_ERROR(checked_add_u64(header_end, sbdir_len, "frq_prelude:
sbdir_len overflow",
+ &covered_len));
+ if (covered_len > prelude.size() || prelude.size() - covered_len <
sizeof(uint32_t)) {
return Status::Error<ErrorCode::INVERTED_INDEX_FILE_CORRUPTED, false>(
"frq_prelude: buffer too short for crc region");
}
+ const size_t covered = static_cast<size_t>(covered_len);
uint32_t stored = 0;
ByteSource crc_src(prelude.subslice(covered, sizeof(uint32_t)));
RETURN_IF_ERROR(crc_src.get_fixed32(&stored));
@@ -287,7 +291,9 @@ Status decode_super_block_dir(Slice dir, const Header& h,
std::vector<SbDirRow>*
return Status::Error<ErrorCode::INVERTED_INDEX_FILE_CORRUPTED,
false>(
"frq_prelude: super-block dir inconsistent");
}
- expect_off += r.block_len;
+ RETURN_IF_ERROR(checked_add_u64(expect_off, r.block_len,
+ "frq_prelude: super-block region
offset overflow",
+ &expect_off));
prev_last = r.last_docid;
rows->push_back(r);
}
@@ -512,8 +518,11 @@ Status FrqPreludeReader::open(Slice prelude,
FrqPreludeReader* out) {
uint64_t window_region_len = 0;
RETURN_IF_ERROR(decode_super_block_dir(dir, h, &rows, &window_region_len));
+ // verify_covered_crc already proved the crc fits, so region_start <=
prelude.size() and the
+ // subtraction cannot underflow. window_region_len is a sum of untrusted
lengths: compare it to
+ // the room left instead of adding it to region_start.
const size_t region_start = header_end + static_cast<size_t>(h.sbdir_len)
+ sizeof(uint32_t);
- if (region_start + static_cast<size_t>(window_region_len) >
prelude.size()) {
+ if (window_region_len > prelude.size() - region_start) {
return Status::Error<ErrorCode::INVERTED_INDEX_FILE_CORRUPTED, false>(
"frq_prelude: window region past buffer");
}
diff --git a/be/test/storage/index/inverted/snii/encoding/pfor_test.cpp
b/be/test/storage/index/inverted/snii/encoding/pfor_test.cpp
index a26fe4e7209..4227e68bad6 100644
--- a/be/test/storage/index/inverted/snii/encoding/pfor_test.cpp
+++ b/be/test/storage/index/inverted/snii/encoding/pfor_test.cpp
@@ -102,6 +102,54 @@ TEST(SniiPfor, ExceptionsAtBoundaries) {
roundtrip(v);
}
+namespace {
+
+// A hand-built block: bit width 0 (no packed bytes) followed by (index delta,
value) exceptions.
+ByteSink exceptions_only(const std::vector<std::pair<uint32_t, uint32_t>>&
exceptions) {
+ ByteSink sink;
+ sink.put_u8(0);
+ sink.put_varint32(static_cast<uint32_t>(exceptions.size()));
+ for (const auto& [delta, value] : exceptions) {
+ sink.put_varint32(delta);
+ sink.put_varint32(value);
+ }
+ return sink;
+}
+
+void expect_corrupted(const ByteSink& sink, size_t n) {
+ std::vector<uint32_t> out(n);
+ ByteSource decode_src(sink.view());
+ EXPECT_TRUE(pfor_decode(&decode_src, n, out.data())
+
.is<doris::ErrorCode::INVERTED_INDEX_FILE_CORRUPTED>());
+ ByteSource skip_src(sink.view());
+ EXPECT_TRUE(pfor_skip(&skip_src,
n).is<doris::ErrorCode::INVERTED_INDEX_FILE_CORRUPTED>());
+}
+
+} // namespace
+
+// The second delta wraps the 32-bit exception index from 5 back to 3, which
is inside the block.
+TEST(SniiPfor, ExceptionIndexDeltaWrapIsCorruption) {
+ expect_corrupted(exceptions_only({{5, 7}, {0xFFFFFFFEU, 9}}), 8);
+}
+
+TEST(SniiPfor, ExceptionIndexPastBlockIsCorruption) {
+ expect_corrupted(exceptions_only({{8, 7}}), 8);
+ expect_corrupted(exceptions_only({{3, 7}, {5, 9}}), 8);
+ expect_corrupted(exceptions_only({{0, 7}}), 0);
+}
+
+// The first exception may sit at index 0, and the last at n - 1.
+TEST(SniiPfor, ExceptionIndexEdgesAreAccepted) {
+ const ByteSink sink = exceptions_only({{0, 7}, {7, 9}});
+ std::vector<uint32_t> out(8);
+ ByteSource decode_src(sink.view());
+ ASSERT_TRUE(pfor_decode(&decode_src, 8, out.data()).ok());
+ EXPECT_EQ(out[0], 7U);
+ EXPECT_EQ(out[7], 9U);
+ ByteSource skip_src(sink.view());
+ EXPECT_TRUE(pfor_skip(&skip_src, 8).ok());
+}
+
// Compare the optimized encoder with the original byte layout across varied
// inputs. The operation counter checks that each value's width is evaluated
once.
namespace {
diff --git a/be/test/storage/index/inverted/snii/encoding/varint_test.cpp
b/be/test/storage/index/inverted/snii/encoding/varint_test.cpp
index 0639aebccef..3e36dcc905e 100644
--- a/be/test/storage/index/inverted/snii/encoding/varint_test.cpp
+++ b/be/test/storage/index/inverted/snii/encoding/varint_test.cpp
@@ -56,6 +56,29 @@ TEST(SniiVarint, TruncatedFails) {
EXPECT_FALSE(decode_varint32(buf, buf + 1, &out, &next).ok());
}
+TEST(SniiVarint, TenthByteOverflowFails) {
+ // Nine continuation bytes carry bits 0..62. The tenth byte can only add
bit 63, so a larger
+ // payload would be shifted out of the result and decode to a silently
truncated value.
+ uint8_t buf[10];
+ for (int i = 0; i < 9; ++i) {
+ buf[i] = 0xFF;
+ }
+ uint64_t out = 0;
+ const uint8_t* next = nullptr;
+
+ buf[9] = 0x01; // 2^64 - 1, the largest valid encoding
+ ASSERT_TRUE(decode_varint64(buf, buf + 10, &out, &next).ok());
+ EXPECT_EQ(out, 0xFFFFFFFFFFFFFFFFULL);
+ EXPECT_EQ(next, buf + 10);
+
+ for (int tenth : {0x02, 0x40, 0x7F}) {
+ buf[9] = static_cast<uint8_t>(tenth);
+ EXPECT_FALSE(decode_varint64(buf, buf + 10, &out, &next).ok()) <<
tenth;
+ }
+ buf[9] = 0x81; // a continuation past the tenth byte
+ EXPECT_FALSE(decode_varint64(buf, buf + 10, &out, &next).ok());
+}
+
TEST(SniiVarint, Overflow32Fails) {
// encode a value > 2^32-1, decoding with decode_varint32 should fail
uint8_t buf[10];
diff --git a/be/test/storage/index/inverted/snii/snii_frq_prelude_test.cpp
b/be/test/storage/index/inverted/snii/snii_frq_prelude_test.cpp
index 6c4512878b4..f238a5ff9f2 100644
--- a/be/test/storage/index/inverted/snii/snii_frq_prelude_test.cpp
+++ b/be/test/storage/index/inverted/snii/snii_frq_prelude_test.cpp
@@ -36,6 +36,7 @@
#include "common/status.h"
#include "storage/index/inverted/snii/common/slice.h"
#include "storage/index/inverted/snii/encoding/byte_sink.h"
+#include "storage/index/inverted/snii/encoding/crc32c.h"
#include "storage/index/inverted/snii/encoding/varint.h"
#include "storage/index/inverted/snii/format/dict_entry.h"
#include "storage/index/inverted/snii/format/frq_prelude.h"
@@ -379,6 +380,70 @@ TEST(SniiFrqPreludeTest,
FP08DdDiskLenSumOverflowIsCorruption) {
EXPECT_TRUE(s.is<doris::ErrorCode::INVERTED_INDEX_FILE_CORRUPTED>()) <<
s.to_string();
}
+namespace {
+
+// Assembles a prelude by hand: header, super_block_dir rows (last_docid
delta, block_off, block_len) and
+// the crc32c over both, with no window region after it. The crc is correct,
so only the lengths are bad.
+ByteSink hand_built_prelude(uint64_t n, uint64_t group_size, uint64_t n_super,
+ const std::vector<std::vector<uint64_t>>&
dir_rows) {
+ ByteSink dir;
+ for (const auto& row : dir_rows) {
+ for (uint64_t field : row) {
+ dir.put_varint64(field);
+ }
+ }
+ ByteSink prelude;
+ prelude.put_u8(0); // flags: no prx
+ prelude.put_varint64(n);
+ prelude.put_varint64(group_size);
+ prelude.put_varint64(n_super);
+ prelude.put_varint64(dir.size());
+ prelude.put_bytes(dir.view());
+ prelude.put_fixed32(doris::snii::crc32c(prelude.view()));
+ return prelude;
+}
+
+void expect_open_corruption(const ByteSink& prelude, const std::string&
message) {
+ FrqPreludeReader reader;
+ const Status s = FrqPreludeReader::open(prelude.view(), &reader);
+ EXPECT_TRUE(s.is<doris::ErrorCode::INVERTED_INDEX_FILE_CORRUPTED>()) <<
s.to_string();
+ EXPECT_NE(s.to_string().find(message), std::string::npos) << s.to_string();
+}
+
+} // namespace
+
+// FP-10: sbdir_len is read from the file. A value near 2^64 must be rejected
as an overflow instead of
+// wrapping header_end + sbdir_len back to a small offset inside the header.
+TEST(SniiFrqPreludeTest, FP10SbdirLenWrapIsCorruption) {
+ ByteSink prelude;
+ prelude.put_u8(0);
+ prelude.put_varint64(1);
+ prelude.put_varint64(kPreludeGroupSize);
+ prelude.put_varint64(1);
+ prelude.put_varint64(std::numeric_limits<uint64_t>::max() - 2);
+ for (int i = 0; i < 16; ++i) {
+ prelude.put_u8(0); // room, so that a wrapped offset would still look
like it is in range
+ }
+ expect_open_corruption(prelude, "sbdir_len overflow");
+}
+
+// FP-11: one super-block whose block_len is 2^64 - 1. With a correct crc and
a consistent directory,
+// region_start + block_len used to wrap to a small number, and a window
region of that huge length was
+// cut from a buffer that holds no bytes for it.
+TEST(SniiFrqPreludeTest, FP11HugeWindowRegionIsCorruption) {
+ expect_open_corruption(hand_built_prelude(1, kPreludeGroupSize, 1,
+ {{1, 0,
std::numeric_limits<uint64_t>::max()}}),
+ "window region past buffer");
+}
+
+// FP-12: two super-blocks whose lengths add up to 2^64 must not wrap the
running region offset to 0.
+TEST(SniiFrqPreludeTest, FP12SuperBlockRegionOffsetWrapIsCorruption) {
+ expect_open_corruption(hand_built_prelude(2, 1, 2,
+ {{1, 0,
std::numeric_limits<uint64_t>::max()},
+ {1,
std::numeric_limits<uint64_t>::max(), 1}}),
+ "super-block region offset overflow");
+}
+
// FP-09 (E2E): a high-df windowed term ("failed", 9000 docs) queried through
the
// real reader returns the full docid set (new path == old semantics), the
docid
// path fetches exactly the (now-smaller) prelude range once, and the on-disk
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
