Skip to content

Commit a806aef

Browse files
presempathy-awbfarhan-syah
authored andcommitted
fix(recovery): guard page-offset arithmetic against untrusted counts
page_id and page_count values reach the pager and deep walk from on-disk catalog and free-list records, so a corrupted record's count could overflow the page_id * page_size multiplication and silently wrap to a bogus offset rather than failing safely. Extract the checked multiplication pager/core.rs already performed into a shared page_space::page_offset helper, and route deep_walk's main-page and segment-page offset computations through it so an unrepresentable count now surfaces as a structured page or segment issue instead of wrapping. deep_walk also moves its page reads onto read_exact_at, which absorbs legal short reads transparently but collapses a genuine truncation into a bare UnexpectedEof; add describe_page_read_failure to turn that back into a diagnostic naming the offset and actual file length. Expand recovery and fsck test coverage to match: a deep-walk test for an impossible catalog page_count, a sweep_orphans test covering live, orphaned-live, and staged-orphan segments in one pass, journal-replay tests for the tombstone action's idempotent/resume/rename outcomes, and a ShortReadVfs fixture proving deep walk tolerates a legal short read on the underlying VFS.
1 parent d8ff86a commit a806aef

6 files changed

Lines changed: 568 additions & 35 deletions

File tree

src/pager/core.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -949,11 +949,7 @@ impl<V: Vfs> Pager<V> {
949949
// mk_epoch before constructing AAD and selecting the DEK.
950950
self.inner.record_miss(file);
951951
let page_size = self.cfg.page_size;
952-
let page_size_u64 =
953-
u64::try_from(page_size).map_err(|_| PagedbError::arithmetic_overflow("page size"))?;
954-
let page_offset = page_id
955-
.checked_mul(page_size_u64)
956-
.ok_or_else(|| PagedbError::arithmetic_overflow("page read offset"))?;
952+
let page_offset = crate::pager::page_space::page_offset(page_id, page_size, "page read")?;
957953
let file_handle = self.open_file_handle(file).await?;
958954

959955
// Observer-mode retry loop: on AEAD failure retry up to

src/pager/page_space.rs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@
1515
//! pages, so a live tree pointer that reaches one is a wild pointer or a
1616
//! use-after-free that recycled a reserved id — never a benign condition.
1717
18+
use crate::Result;
19+
use crate::errors::PagedbError;
20+
1821
/// First page id the allocator may hand out. Ids below this are reserved.
1922
pub const FIRST_ALLOCATABLE_PAGE_ID: u64 = 4;
2023

@@ -24,3 +27,40 @@ pub const FIRST_ALLOCATABLE_PAGE_ID: u64 = 4;
2427
pub const fn is_reserved(page_id: u64) -> bool {
2528
page_id < FIRST_ALLOCATABLE_PAGE_ID
2629
}
30+
31+
/// Byte offset of `page_id` in a paged file, or an arithmetic error.
32+
///
33+
/// Page ids reach this from disk — a catalog record, a free-list entry, a
34+
/// header field — so the product is not trusted to fit. `operation` names the
35+
/// caller in the resulting error, since a wrapped offset and a rejected one
36+
/// are indistinguishable by the time a diagnostic reports them.
37+
pub fn page_offset(page_id: u64, page_size: usize, operation: &'static str) -> Result<u64> {
38+
let page_size =
39+
u64::try_from(page_size).map_err(|_| PagedbError::arithmetic_overflow(operation))?;
40+
page_id
41+
.checked_mul(page_size)
42+
.ok_or_else(|| PagedbError::arithmetic_overflow(operation))
43+
}
44+
45+
#[cfg(test)]
46+
mod tests {
47+
use super::*;
48+
49+
const PAGE: usize = 4096;
50+
51+
#[test]
52+
fn offset_scales_by_page_size() {
53+
assert_eq!(page_offset(0, PAGE, "test").unwrap(), 0);
54+
assert_eq!(page_offset(4, PAGE, "test").unwrap(), 16_384);
55+
}
56+
57+
#[test]
58+
fn offset_rejects_a_page_id_that_overflows_the_address_space() {
59+
let first_unrepresentable = (u64::MAX / PAGE as u64) + 1;
60+
assert!(matches!(
61+
page_offset(first_unrepresentable, PAGE, "test"),
62+
Err(PagedbError::ArithmeticOverflow { .. })
63+
));
64+
assert!(page_offset(first_unrepresentable - 1, PAGE, "test").is_ok());
65+
}
66+
}

src/recovery/deep_walk.rs

Lines changed: 132 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,15 @@ use crate::btree::leaf::{Leaf, LeafValue};
1414
use crate::btree::overflow;
1515
use crate::catalog::codec::{Catalog, SegmentMeta};
1616
use crate::crypto::aad::{Aad, AadFields, MAIN_DB_SEGMENT_ID};
17+
use crate::errors::PagedbError;
1718
use crate::pager::format::data_page::extract_page_header_ids;
1819
use crate::pager::format::page_kind::PageKind;
19-
use crate::pager::page_space::{FIRST_ALLOCATABLE_PAGE_ID, is_reserved};
20+
use crate::pager::page_space::{FIRST_ALLOCATABLE_PAGE_ID, is_reserved, page_offset};
2021
use crate::pager::{PageGuard, Pager};
2122
use crate::segment::authenticated_metadata::authenticate_segment_metadata;
2223
use crate::txn::db::Db;
2324
use crate::vfs::types::OpenMode;
24-
use crate::vfs::{Vfs, VfsFile};
25+
use crate::vfs::{Vfs, VfsFile, read_exact_at};
2526

2627
/// A single page-level issue found during deep walk.
2728
#[non_exhaustive]
@@ -260,7 +261,7 @@ pub async fn run_deep_walk<V: Vfs + Clone>(db: &Db<V>) -> Result<DeepWalkReport>
260261

261262
let vfs: &V = &db.vfs;
262263
let main_file_res = vfs.open(main_db_path, OpenMode::Read).await;
263-
let main_file = match main_file_res {
264+
let mut main_file = match main_file_res {
264265
Ok(f) => f,
265266
Err(e) => {
266267
report.page_issues.push(PageIssue {
@@ -275,24 +276,25 @@ pub async fn run_deep_walk<V: Vfs + Clone>(db: &Db<V>) -> Result<DeepWalkReport>
275276
// (HK-MAC, cleartext) and are already verified by `Db::open`. Skip them.
276277
// Page 2 and 3 are reserved (apply-journal). Walk from page 4.
277278
for page_id in 4..next_page_id {
278-
let offset = page_id * page_size as u64;
279-
let mut buf = vec![0u8; page_size];
280-
match main_file.read_at(offset, &mut buf).await {
281-
Ok(n) if n < page_size => {
282-
// Short read at the tail — the file may be smaller than expected.
283-
// Report but continue.
279+
let offset = match page_offset(page_id, page_size, "deep-walk main page offset") {
280+
Ok(offset) => offset,
281+
Err(error) => {
284282
report.page_issues.push(PageIssue {
285283
page_id,
286-
description: format!("short read: expected {page_size} bytes, got {n}"),
284+
description: format!("{error}"),
287285
});
288286
report.pages_examined += 1;
289287
continue;
290288
}
291-
Ok(_) => {}
292-
Err(e) => {
289+
};
290+
let mut buf = vec![0u8; page_size];
291+
match read_exact_at(&mut main_file, offset, &mut buf).await {
292+
Ok(()) => {}
293+
Err(error) => {
294+
let description = describe_page_read_failure(&mut main_file, offset, error).await;
293295
report.page_issues.push(PageIssue {
294296
page_id,
295-
description: format!("read error: {e}"),
297+
description,
296298
});
297299
report.pages_examined += 1;
298300
continue;
@@ -435,7 +437,7 @@ async fn check_segment<V: Vfs + Clone>(
435437
let page_size = pager.page_size();
436438

437439
// Check file exists.
438-
let Ok(file) = vfs.open(&live, OpenMode::Read).await else {
440+
let Ok(mut file) = vfs.open(&live, OpenMode::Read).await else {
439441
report.drift_issues.push(DriftIssue {
440442
segment_id: meta.segment_id,
441443
description: "segment file missing from seg/".to_string(),
@@ -464,7 +466,16 @@ async fn check_segment<V: Vfs + Clone>(
464466
// We don't have a metadata API, but we can check via read: try reading one
465467
// byte past the expected end. If it succeeds (on some VFS) we skip the
466468
// check; if we read exactly `page_count * page_size` bytes we're consistent.
467-
let expected_size = meta.page_count * page_size as u64;
469+
let expected_size = match page_offset(meta.page_count, page_size, "segment expected size") {
470+
Ok(offset) => offset,
471+
Err(error) => {
472+
report.segment_issues.push(SegmentIssue {
473+
segment_id: meta.segment_id,
474+
description: format!("{error}"),
475+
});
476+
return;
477+
}
478+
};
468479
let mut probe = vec![0u8; 1];
469480
let over_read = file.read_at(expected_size, &mut probe).await;
470481
match over_read {
@@ -483,24 +494,24 @@ async fn check_segment<V: Vfs + Clone>(
483494
// Walk data pages (1 .. page_count - 1, skipping header=0 and footer=last).
484495
let last_data = footer_page_id;
485496
for page_id in 1..last_data {
486-
let offset = page_id * page_size as u64;
487-
let mut buf = vec![0u8; page_size];
488-
let read_res = file.read_at(offset, &mut buf).await;
489-
match read_res {
490-
Ok(n) if n < page_size => {
497+
let offset = match page_offset(page_id, page_size, "segment data page offset") {
498+
Ok(offset) => offset,
499+
Err(error) => {
491500
report.segment_issues.push(SegmentIssue {
492501
segment_id: meta.segment_id,
493-
description: format!(
494-
"short read at page {page_id}: expected {page_size} bytes, got {n}"
495-
),
502+
description: format!("page {page_id}: {error}"),
496503
});
497504
continue;
498505
}
499-
Ok(_) => {}
500-
Err(e) => {
506+
};
507+
let mut buf = vec![0u8; page_size];
508+
match read_exact_at(&mut file, offset, &mut buf).await {
509+
Ok(()) => {}
510+
Err(error) => {
511+
let description = describe_page_read_failure(&mut file, offset, error).await;
501512
report.segment_issues.push(SegmentIssue {
502513
segment_id: meta.segment_id,
503-
description: format!("read error at page {page_id}: {e}"),
514+
description: format!("page {page_id}: {description}"),
504515
});
505516
continue;
506517
}
@@ -553,6 +564,34 @@ async fn check_segment<V: Vfs + Clone>(
553564
}
554565
}
555566

567+
/// Turn a failed full-page read into a description that keeps the byte counts.
568+
///
569+
/// `read_exact_at` completes a legal short read and only gives up once the
570+
/// backend stops making progress, so the partial count it consumed never
571+
/// reaches the caller — a truncated file arrives here as a bare
572+
/// `UnexpectedEof`. On a diagnostic surface those numbers are the product: an
573+
/// operator needs to know the file is short and by how much, not merely that a
574+
/// read ended. Any other error is already self-describing and passes through.
575+
async fn describe_page_read_failure<F: VfsFile>(
576+
file: &mut F,
577+
offset: u64,
578+
error: PagedbError,
579+
) -> String {
580+
let is_eof = matches!(
581+
&error,
582+
PagedbError::Io(io) if io.kind() == std::io::ErrorKind::UnexpectedEof
583+
);
584+
if !is_eof {
585+
return format!("read error: {error}");
586+
}
587+
match file.len().await {
588+
Ok(len) => format!("truncated: page starts at offset {offset}, file is {len} bytes"),
589+
Err(len_error) => {
590+
format!("read error: {error} (file length unavailable: {len_error})")
591+
}
592+
}
593+
}
594+
556595
/// Collect the set of all page IDs reachable from the main B+ tree root,
557596
/// the catalog root, the commit-history root, and the free-list root.
558597
/// Pages 0..=3 (reserved) are always considered reachable.
@@ -877,10 +916,10 @@ async fn diagnose_overflow_chain<V: Vfs + Clone>(
877916
#[cfg(test)]
878917
mod tests {
879918
use super::*;
880-
use crate::OpenOptions;
881919
use crate::btree::node::body_capacity;
882920
use crate::pager::format::data_page::ENVELOPE_OVERHEAD;
883921
use crate::vfs::memory::MemVfs;
922+
use crate::{OpenOptions, SegmentKind, SegmentPageKind};
884923

885924
const PAGE: usize = 4096;
886925
const REALM: crate::RealmId = crate::RealmId::new([0xD3; 16]);
@@ -1074,6 +1113,72 @@ mod tests {
10741113
);
10751114
}
10761115

1116+
/// A catalog `page_count` large enough to overflow a byte offset must be
1117+
/// rejected as a structured issue, never reach the arithmetic that would
1118+
/// wrap it, and never abort the walk. Authenticated metadata validation is
1119+
/// what stops it, ahead of any footer index or loop bound; the checked
1120+
/// `page_offset` behind it is the second line, covered directly in
1121+
/// `pager::page_space`.
1122+
#[tokio::test(flavor = "current_thread")]
1123+
async fn deep_walk_rejects_impossible_catalog_page_count() {
1124+
let db = open_db().await;
1125+
let mut segment = db
1126+
.create_segment(REALM, SegmentKind::Unspecified)
1127+
.await
1128+
.unwrap();
1129+
segment
1130+
.append_page(SegmentPageKind::Data, b"deep-walk")
1131+
.await
1132+
.unwrap();
1133+
let mut meta = segment.seal().await.unwrap();
1134+
{
1135+
let mut txn = db.begin_write().await.unwrap();
1136+
txn.link_segment("overflow", &meta).await.unwrap();
1137+
txn.commit().await.unwrap();
1138+
}
1139+
1140+
meta.page_count = (u64::MAX / PAGE as u64) + 2;
1141+
let (catalog_root, next_page_id) = {
1142+
let state = db.writer.lock().await;
1143+
(state.catalog_root_page_id, state.next_page_id)
1144+
};
1145+
let mut tree = BTree::open(
1146+
db.pager.clone(),
1147+
db.realm_id,
1148+
catalog_root,
1149+
next_page_id,
1150+
db.page_size,
1151+
);
1152+
let key = Catalog::segment_key(REALM, b"overflow").unwrap();
1153+
tree.put(&key, &Catalog::encode_segment_meta(&meta))
1154+
.await
1155+
.unwrap();
1156+
tree.flush().await.unwrap();
1157+
{
1158+
let mut state = db.writer.lock().await;
1159+
state.catalog_root_page_id = tree.root_page_id();
1160+
state.next_page_id = state.next_page_id.max(tree.next_page_id());
1161+
}
1162+
1163+
let report = run_deep_walk(&db).await.unwrap();
1164+
assert!(
1165+
report.segment_issues.iter().any(|issue| {
1166+
issue.segment_id == meta.segment_id
1167+
&& issue
1168+
.description
1169+
.contains("authenticated segment metadata invalid")
1170+
}),
1171+
"impossible segment geometry must become a structured issue, got {report:?}"
1172+
);
1173+
assert!(
1174+
report.segment_issues.iter().all(|issue| {
1175+
!issue.description.contains("segment expected size")
1176+
&& !issue.description.contains("segment data page offset")
1177+
}),
1178+
"validation must reject the record before any offset arithmetic runs: {report:?}"
1179+
);
1180+
}
1181+
10771182
async fn sole_overflow_root(db: &Db<MemVfs>, leaf_page_id: u64) -> u64 {
10781183
let (guard, _) = db.pager.read_main_node(leaf_page_id, REALM).await.unwrap();
10791184
let leaf = Leaf::decode(guard.body_ref()).unwrap();

src/recovery/reconcile.rs

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,13 @@ mod tests {
277277
use crate::vfs::{Vfs, VfsFile};
278278
use crate::{RealmId, btree::BTree};
279279

280-
use super::repair_catalog;
280+
use super::{repair_catalog, sweep_orphans};
281+
282+
fn segment_id(value: u64) -> [u8; 16] {
283+
let mut id = [0; 16];
284+
id[..8].copy_from_slice(&value.to_le_bytes());
285+
id
286+
}
281287

282288
#[tokio::test(flavor = "current_thread")]
283289
async fn malformed_catalog_key_prevents_reconciliation_mutation() {
@@ -347,4 +353,57 @@ mod tests {
347353
));
348354
assert!(vfs.open(marker, OpenMode::Read).await.is_ok());
349355
}
356+
357+
/// The sweep has three outcomes and every one of them is destructive if it
358+
/// fires on the wrong file: an expected live segment must survive, a live
359+
/// segment the catalog does not name must become a tombstone rather than a
360+
/// deletion, and an unnamed staging file must be removed outright. The
361+
/// expected set is large enough that a membership test which silently
362+
/// matched on a prefix, a truncated id, or the first entry alone would
363+
/// misclassify one of the three.
364+
#[tokio::test(flavor = "current_thread")]
365+
async fn sweep_orphans_tombstones_live_orphans_and_removes_staged_orphans() {
366+
let vfs = MemVfs::new();
367+
vfs.mkdir_all("seg/.staging").await.unwrap();
368+
let expected: Vec<[u8; 16]> = (0..1024).map(segment_id).collect();
369+
370+
for id in expected.iter().take(8) {
371+
let path = crate::segment::writer::live_path(id);
372+
let mut file = vfs.open(&path, OpenMode::CreateOrOpen).await.unwrap();
373+
file.write_at(0, b"live").await.unwrap();
374+
}
375+
376+
let live_orphan = segment_id(10_000);
377+
let live_orphan_path = crate::segment::writer::live_path(&live_orphan);
378+
let mut live_file = vfs
379+
.open(&live_orphan_path, OpenMode::CreateOrOpen)
380+
.await
381+
.unwrap();
382+
live_file.write_at(0, b"orphan").await.unwrap();
383+
384+
let staged_orphan = segment_id(10_001);
385+
let staged_orphan_path = crate::segment::writer::staging_path(&staged_orphan);
386+
let mut staged_file = vfs
387+
.open(&staged_orphan_path, OpenMode::CreateOrOpen)
388+
.await
389+
.unwrap();
390+
staged_file.write_at(0, b"orphan").await.unwrap();
391+
392+
sweep_orphans(&vfs, &expected, 77).await.unwrap();
393+
394+
for id in expected.iter().take(8) {
395+
let path = crate::segment::writer::live_path(id);
396+
assert!(
397+
vfs.open(&path, OpenMode::Read).await.is_ok(),
398+
"a segment the catalog names must survive the sweep: {path}"
399+
);
400+
}
401+
assert!(vfs.open(&live_orphan_path, OpenMode::Read).await.is_err());
402+
let tombstone = format!(
403+
"seg/.tombstone/{}.77",
404+
crate::hex::to_hex_lower(&live_orphan)
405+
);
406+
assert!(vfs.open(&tombstone, OpenMode::Read).await.is_ok());
407+
assert!(vfs.open(&staged_orphan_path, OpenMode::Read).await.is_err());
408+
}
350409
}

0 commit comments

Comments
 (0)