From 16d4208f5794436b7378f498ba11d3d06189a2f3 Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Thu, 9 Jul 2026 16:20:52 +0000 Subject: [PATCH 1/3] fix: reject data/segment file sizes over 4 GiB in ValidateOptions In-file byte offsets are computed as uint32 (WriteTask::ConvFilePageId, IouringMgr::ConvFilePageId/ConvFileSegmentId): offset = in-file page/ segment index * page/segment size. DataFileSize() (data_page_size << pages_per_file_shift) and SegmentFileSize() are uncapped, so a file larger than 4 GiB makes the offset wrap modulo 2^32 back onto earlier pages -- pwrite lands on an already-mapped page and silently corrupts previously committed data. Wrapped offsets stay page-aligned, so the alignment assert never fires. EloqStore is not designed for files beyond 4 GiB. Reject such configs at startup: ValidateOptions now fails when DataFileSize() or SegmentFileSize() exceeds 4 GiB (with a shift guard to avoid overflowing the size_t computation for pathological shift values). The 4 GiB boundary itself is allowed -- the largest in-file offset is then file_size minus one page, which still fits a uint32. Adds an eloq_store_test case covering the boundary (4 GiB accepted) and over-limit data/segment configs (rejected). --- src/eloq_store.cpp | 27 +++++++++++++++++++++++++++ tests/eloq_store_test.cpp | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/src/eloq_store.cpp b/src/eloq_store.cpp index deaaeeba..ab324b67 100644 --- a/src/eloq_store.cpp +++ b/src/eloq_store.cpp @@ -157,6 +157,33 @@ bool EloqStore::ValidateOptions(KvOptions &opts) << "-byte aligned"; return false; } + // A data/segment file's in-file byte offset is computed as a uint32 + // (ConvFilePageId / ConvFileSegmentId), so a single file may span at most + // 4 GiB. EloqStore is not designed for files beyond that; a larger file + // wraps offsets back onto earlier pages and silently corrupts previously + // written data, so reject such configs at startup. The shift guard also + // avoids overflowing the size_t computation in DataFileSize() / + // SegmentFileSize() for pathological shift values. + constexpr uint64_t kMaxFileSize = uint64_t{1} << 32; // 4 GiB + if (opts.pages_per_file_shift >= 32 || opts.DataFileSize() > kMaxFileSize) + { + LOG(ERROR) << "Option data_page_size << pages_per_file_shift exceeds " + "the 4 GiB per-file limit imposed by 32-bit file offsets " + "(data_page_size=" + << opts.data_page_size << ", pages_per_file_shift=" + << int(opts.pages_per_file_shift) << ")"; + return false; + } + if (opts.segments_per_file_shift >= 32 || + opts.SegmentFileSize() > kMaxFileSize) + { + LOG(ERROR) << "Option segment_size << segments_per_file_shift exceeds " + "the 4 GiB per-file limit imposed by 32-bit file offsets " + "(segment_size=" + << opts.segment_size << ", segments_per_file_shift=" + << int(opts.segments_per_file_shift) << ")"; + return false; + } if ((opts.coroutine_stack_size & (page_align - 1)) != 0) { LOG(ERROR) << "Option coroutine_stack_size is not page aligned"; diff --git a/tests/eloq_store_test.cpp b/tests/eloq_store_test.cpp index 391143a4..fc379be2 100644 --- a/tests/eloq_store_test.cpp +++ b/tests/eloq_store_test.cpp @@ -149,6 +149,42 @@ TEST_CASE("EloqStore ValidateOptions validates all parameters", "[eloq_store]") CleanupTestDir(test_dir); } +// Audit finding rank 15: in-file byte offsets are computed as uint32 +// (ConvFilePageId / ConvFileSegmentId), so a data/segment file may span at +// most 4 GiB. ValidateOptions must reject configs whose file size exceeds that, +// otherwise offsets wrap onto earlier pages and silently corrupt data. +TEST_CASE("EloqStore ValidateOptions rejects >4GiB data/segment files", + "[eloq_store]") +{ + auto test_dir = CreateTestDir("_validate_file_size"); + auto options = CreateValidOptions(test_dir); + + // Boundary: data_page_size << pages_per_file_shift == 4 GiB is allowed + // (the largest in-file offset is 4 GiB - data_page_size, which fits + // uint32). + options.data_page_size = 4 * 1024; // 2^12 + options.pages_per_file_shift = 20; // 2^12 << 20 == 2^32 == 4 GiB + REQUIRE(eloqstore::EloqStore::ValidateOptions(options) == true); + + // Just over 4 GiB -> reject. + options.pages_per_file_shift = 21; // 8 GiB + REQUIRE(eloqstore::EloqStore::ValidateOptions(options) == false); + options = CreateValidOptions(test_dir); + + // Same limit reached via a larger page size. + options.data_page_size = 32 * 1024; // 2^15 + options.pages_per_file_shift = 18; // 2^15 << 18 == 2^33 == 8 GiB + REQUIRE(eloqstore::EloqStore::ValidateOptions(options) == false); + options = CreateValidOptions(test_dir); + + // Segment file over 4 GiB -> reject. + options.segment_size = 256 * 1024; // 2^18 + options.segments_per_file_shift = 15; // 2^18 << 15 == 2^33 == 8 GiB + REQUIRE(eloqstore::EloqStore::ValidateOptions(options) == false); + + CleanupTestDir(test_dir); +} + TEST_CASE( "EloqStore ValidateOptions rejects bad pinned-memory / GC-pool configs", "[eloq_store]") From 3cb82789d9d26edcfab388837f74373a94770a59 Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Fri, 10 Jul 2026 03:05:03 +0000 Subject: [PATCH 2/3] fix: reject out-of-range data_page_size (uint16 truncation / zero) data_page_size is a uint16 field. KvOptions::LoadFromIni parsed the INI value into a uint64 and assigned it with only a >0 guard on the pre-truncation value: 64KB (65536) wrapped to 0 -> SIGFPE on the data_file_size / data_page_size division and later in the PageManager constructor; 68KB silently became 4KB, ignoring the operator's intent. - LoadFromIni now rejects data_page_size that is 0 or does not fit in uint16 (>= 64KB), returning an error instead of truncating. - ValidateOptions now rejects data_page_size == 0 (it satisfies the page-alignment mask, so it slipped through and divided by zero in the PageManager constructor) -- defense in depth for the programmatic path. Adds eloq_store_test coverage: LoadFromIni rejects 64KB/68KB and accepts 8KB; ValidateOptions rejects data_page_size == 0. --- src/eloq_store.cpp | 8 +++++-- src/kv_options.cpp | 13 +++++++++++- tests/eloq_store_test.cpp | 44 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/src/eloq_store.cpp b/src/eloq_store.cpp index ab324b67..1fc3bd7b 100644 --- a/src/eloq_store.cpp +++ b/src/eloq_store.cpp @@ -142,9 +142,13 @@ bool EloqStore::ValidateOptions(KvOptions &opts) LOG(ERROR) << "Option max_global_request_batch cannot be zero"; return false; } - if ((opts.data_page_size & (page_align - 1)) != 0) + if (opts.data_page_size == 0 || + (opts.data_page_size & (page_align - 1)) != 0) { - LOG(ERROR) << "Option data_page_size is not page aligned"; + // data_page_size == 0 divides by zero in the PageManager constructor; + // it also satisfies the alignment mask, so guard it explicitly. + LOG(ERROR) << "Option data_page_size (" << opts.data_page_size + << ") must be non-zero and page aligned"; return false; } // segment_size: must be page-aligned. Enforced here so release builds diff --git a/src/kv_options.cpp b/src/kv_options.cpp index 96b73b2b..12178142 100644 --- a/src/kv_options.cpp +++ b/src/kv_options.cpp @@ -336,7 +336,18 @@ int KvOptions::LoadFromIni(const char *path) std::string value_str = reader.Get(sec_permanent, "data_page_size", "4KB"); uint64_t parsed_size = ParseSizeWithUnit(value_str); - data_page_size = (parsed_size > 0) ? parsed_size : (1 << 12); + // data_page_size is a uint16 field, so anything at/above 64KB would + // silently truncate: 64KB -> 0 (SIGFPE on the divisions below and in + // PageManager), 68KB -> 4KB. Reject out-of-range values instead of + // corrupting the config or crashing at startup. + if (parsed_size == 0 || + parsed_size > std::numeric_limits::max()) + { + LOG(ERROR) << "Invalid data_page_size '" << value_str + << "': must be non-zero and below 64KB (uint16)"; + return -3; + } + data_page_size = static_cast(parsed_size); } if (reader.HasValue(sec_permanent, "data_file_size")) { diff --git a/tests/eloq_store_test.cpp b/tests/eloq_store_test.cpp index fc379be2..cff7e8f2 100644 --- a/tests/eloq_store_test.cpp +++ b/tests/eloq_store_test.cpp @@ -54,6 +54,12 @@ TEST_CASE("EloqStore ValidateOptions validates all parameters", "[eloq_store]") REQUIRE(eloqstore::EloqStore::ValidateOptions(options) == false); options = CreateValidOptions(test_dir); // restore valid value + // Test data_page_size == 0 (satisfies the alignment mask but divides by + // zero in the PageManager constructor). + options.data_page_size = 0; + REQUIRE(eloqstore::EloqStore::ValidateOptions(options) == false); + options = CreateValidOptions(test_dir); // restore valid value + // Test coroutine_stack_size that is not page-aligned options.coroutine_stack_size = 8193; // not page-aligned REQUIRE(eloqstore::EloqStore::ValidateOptions(options) == false); @@ -185,6 +191,44 @@ TEST_CASE("EloqStore ValidateOptions rejects >4GiB data/segment files", CleanupTestDir(test_dir); } +// Audit finding rank 23: data_page_size is a uint16 field, but LoadFromIni +// parsed it into a uint64 and assigned with only a >0 guard on the +// pre-truncation value -- 64KB wrapped to 0 (SIGFPE at startup), 68KB silently +// became 4KB. LoadFromIni must reject out-of-range values instead. +TEST_CASE("KvOptions LoadFromIni rejects out-of-range data_page_size", + "[eloq_store]") +{ + auto test_dir = CreateTestDir("_ini_data_page_size"); + auto write_ini = [&](const std::string &name, const std::string &page_size) + { + fs::path p = test_dir / name; + std::ofstream f(p); + f << "[run]\nnum_threads = 2\n[permanent]\ndata_page_size=" << page_size + << "\n"; + f.close(); + return p; + }; + + // 64KB overflows the uint16 field to 0 -> reject, don't truncate to 0. + { + eloqstore::KvOptions opts; + REQUIRE(opts.LoadFromIni(write_ini("bad64k.ini", "64KB").c_str()) < 0); + } + // 68KB would silently truncate to 4KB -> reject. + { + eloqstore::KvOptions opts; + REQUIRE(opts.LoadFromIni(write_ini("bad68k.ini", "68KB").c_str()) < 0); + } + // A valid in-range page size loads correctly. + { + eloqstore::KvOptions opts; + REQUIRE(opts.LoadFromIni(write_ini("ok8k.ini", "8KB").c_str()) == 0); + REQUIRE(opts.data_page_size == 8192); + } + + CleanupTestDir(test_dir); +} + TEST_CASE( "EloqStore ValidateOptions rejects bad pinned-memory / GC-pool configs", "[eloq_store]") From 0326addecdeba5a3539a170fbea2a013d08aba34 Mon Sep 17 00:00:00 2001 From: Jerry Zhao Date: Fri, 10 Jul 2026 04:22:00 +0000 Subject: [PATCH 3/3] test: describe the bug directly, without an internal audit reference --- tests/eloq_store_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/eloq_store_test.cpp b/tests/eloq_store_test.cpp index cff7e8f2..b69f2f97 100644 --- a/tests/eloq_store_test.cpp +++ b/tests/eloq_store_test.cpp @@ -155,7 +155,7 @@ TEST_CASE("EloqStore ValidateOptions validates all parameters", "[eloq_store]") CleanupTestDir(test_dir); } -// Audit finding rank 15: in-file byte offsets are computed as uint32 +// In-file byte offsets are computed as uint32 // (ConvFilePageId / ConvFileSegmentId), so a data/segment file may span at // most 4 GiB. ValidateOptions must reject configs whose file size exceeds that, // otherwise offsets wrap onto earlier pages and silently corrupt data. @@ -191,7 +191,7 @@ TEST_CASE("EloqStore ValidateOptions rejects >4GiB data/segment files", CleanupTestDir(test_dir); } -// Audit finding rank 23: data_page_size is a uint16 field, but LoadFromIni +// data_page_size is a uint16 field, but LoadFromIni // parsed it into a uint64 and assigned with only a >0 guard on the // pre-truncation value -- 64KB wrapped to 0 (SIGFPE at startup), 68KB silently // became 4KB. LoadFromIni must reject out-of-range values instead.