Skip to content

Commit fcdb9cc

Browse files
committed
fix(pager): detect free-list cycles in bounded memory
Free-list cycle detection compared the chain's step count against next_page_id, the allocator's whole page-id space, so a caller with no tighter bound would walk up to 2^64 links before concluding anything. Switch to Floyd's tortoise-and-hare: the hare laps the tortoise within one turn of any cycle, and a well-formed chain still terminates as soon as the tortoise reaches the end. This also drops the now-unused page_id_bound argument from count_chain.
1 parent 4a2bfc1 commit fcdb9cc

2 files changed

Lines changed: 82 additions & 30 deletions

File tree

src/pager/freelist.rs

Lines changed: 80 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -82,35 +82,61 @@ fn decode_chain_header(body: &[u8]) -> Result<(u64, usize)> {
8282
/// The counting counterpart to [`read_chain`], for callers that need only the
8383
/// depth. Collecting the entries to take their length would size an allocation
8484
/// by how many pages the durable free list is carrying, which grows with the
85-
/// database; this keeps the resident cost at one page guard.
85+
/// database; this keeps the resident cost at O(1) — two page cursors, no
86+
/// visited set at all.
8687
///
87-
/// `page_id_bound` is the allocator's `next_page_id`. Chain pages are real
88-
/// allocated page ids, so a chain that visits more than `page_id_bound` pages
89-
/// must by pigeonhole have revisited one — that bound stands in for the visited
90-
/// set [`read_chain`] keeps, which is itself proportional to the chain length.
88+
/// Termination on a cyclic chain comes from Floyd's tortoise-and-hare: the hare
89+
/// takes two links for the tortoise's one, so on a chain that loops it laps the
90+
/// tortoise within one turn of the cycle, and on a chain that ends the tortoise
91+
/// simply reaches the terminator. Both bounds are properties of the chain
92+
/// itself. That is the whole point of the shape: any guard phrased as "stop
93+
/// after N steps" is only as good as the N its caller happened to pass, and a
94+
/// caller with no better number than the page-id space would spin for 2^64
95+
/// links before concluding anything.
9196
pub async fn count_chain<V: Vfs + Clone>(
9297
pager: &Pager<V>,
9398
realm_id: RealmId,
9499
head: u64,
95-
page_id_bound: u64,
96100
) -> Result<u64> {
97101
let mut total: u64 = 0;
98-
let mut steps: u64 = 0;
99-
let mut page = head;
100-
while page != 0 {
101-
if steps >= page_id_bound {
102-
return Err(PagedbError::page_chain_cycle("free_list", page));
103-
}
104-
steps += 1;
105-
let guard = pager.read_main_page(page, realm_id, PageKind::Free).await?;
106-
let (next, count) = decode_chain_header(guard.body_ref())?;
102+
let mut tortoise = head;
103+
let mut hare = head;
104+
105+
while tortoise != 0 {
106+
let (next, count) = read_chain_link(pager, realm_id, tortoise).await?;
107107
total = total.saturating_add(count as u64);
108-
drop(guard);
109-
page = next;
108+
tortoise = next;
109+
110+
// Two hare links per tortoise link. The hare reaching the chain's
111+
// terminator is not a result on its own — the tortoise still has to
112+
// walk the rest to finish counting — so it just parks at 0.
113+
for _ in 0..2 {
114+
if hare == 0 {
115+
break;
116+
}
117+
let (next, _) = read_chain_link(pager, realm_id, hare).await?;
118+
hare = next;
119+
}
120+
121+
if hare != 0 && hare == tortoise {
122+
return Err(PagedbError::page_chain_cycle("free_list", hare));
123+
}
110124
}
111125
Ok(total)
112126
}
113127

128+
/// Read one chain page and return its `(next, entry count)` header.
129+
async fn read_chain_link<V: Vfs + Clone>(
130+
pager: &Pager<V>,
131+
realm_id: RealmId,
132+
page_id: u64,
133+
) -> Result<(u64, usize)> {
134+
let guard = pager
135+
.read_main_page(page_id, realm_id, PageKind::Free)
136+
.await?;
137+
decode_chain_header(guard.body_ref())
138+
}
139+
114140
/// Walk the free-list chain from `head`, returning all `(commit_id, page_id)`
115141
/// entries and the list of page ids the chain itself occupies. `head == 0` is
116142
/// an empty chain.
@@ -314,11 +340,10 @@ mod tests {
314340
.await
315341
.unwrap();
316342

317-
let error =
318-
tokio::time::timeout(Duration::from_secs(1), count_chain(&pager, REALM, 10, 11))
319-
.await
320-
.expect("the page-id bound should end the walk before timeout")
321-
.expect_err("free-list cycles must be corruption");
343+
let error = tokio::time::timeout(Duration::from_secs(1), count_chain(&pager, REALM, 10))
344+
.await
345+
.expect("cycle detection should end the walk before timeout")
346+
.expect_err("free-list cycles must be corruption");
322347
assert!(
323348
matches!(
324349
error,
@@ -331,6 +356,38 @@ mod tests {
331356
);
332357
}
333358

359+
/// Rho shape: `20 → 21 → 22 → 21`. The head is not itself on the cycle, so
360+
/// comparing every link against the head never fires, and the cycle is
361+
/// longer than one link, so a self-loop check never fires either. Only a
362+
/// detector that tracks relative progress ends this walk.
363+
#[tokio::test(flavor = "current_thread")]
364+
async fn count_chain_rejects_a_cycle_reached_through_a_tail() {
365+
let pager = test_pager().await;
366+
for (page_id, next) in [(20u64, 21u64), (21, 22), (22, 21)] {
367+
let mut body = vec![0u8; body_capacity(PAGE)];
368+
body[0..8].copy_from_slice(&next.to_le_bytes());
369+
pager
370+
.write_main_page(page_id, REALM, PageKind::Free, &body)
371+
.await
372+
.unwrap();
373+
}
374+
375+
let error = tokio::time::timeout(Duration::from_secs(1), count_chain(&pager, REALM, 20))
376+
.await
377+
.expect("cycle detection should end the walk before timeout")
378+
.expect_err("free-list cycles must be corruption");
379+
assert!(
380+
matches!(
381+
error,
382+
PagedbError::Corruption(crate::errors::CorruptionDetail::PageChainCycle {
383+
structure: "free_list",
384+
..
385+
})
386+
),
387+
"expected a free-list PageChainCycle, got {error:?}"
388+
);
389+
}
390+
334391
#[tokio::test(flavor = "current_thread")]
335392
async fn count_chain_matches_read_chain_across_pages() {
336393
let pager = test_pager().await;
@@ -342,7 +399,7 @@ mod tests {
342399
.unwrap();
343400

344401
let (read, _) = read_chain(&pager, REALM, head).await.unwrap();
345-
let counted = count_chain(&pager, REALM, head, 64).await.unwrap();
402+
let counted = count_chain(&pager, REALM, head).await.unwrap();
346403
assert_eq!(counted, read.len() as u64);
347404
assert_eq!(counted, entries.len() as u64);
348405
}

src/txn/db/misc.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,8 @@ impl<V: Vfs + Clone> Db<V> {
122122
// Counted in place rather than collected: how many pages the free list
123123
// is carrying grows with the database, and a metrics call must not size
124124
// an allocation by it.
125-
let free_list_pending_entries = crate::pager::freelist::count_chain(
126-
&self.pager,
127-
self.realm_id,
128-
free_list_root,
129-
next_page_id,
130-
)
131-
.await?;
125+
let free_list_pending_entries =
126+
crate::pager::freelist::count_chain(&self.pager, self.realm_id, free_list_root).await?;
132127

133128
// Main database file size.
134129
let main_db_bytes = self

0 commit comments

Comments
 (0)