Skip to content

Commit ccb819c

Browse files
committed
fix(vfs): reject impossible positional-I/O progress reports
A backend that reports transferring more bytes than the caller's remaining buffer has broken the read_at/write_at contract, not corrupted the on-disk format. Give that case its own error variant instead of a generic io::Error, and share the progress-checking logic between reads and writes. Also add a borrowed-handle variant of the read loop for callers that only have a &F, since a future holding &F across an await is Send only when F: Sync, which VfsFile does not require.
1 parent d9ab27e commit ccb819c

5 files changed

Lines changed: 159 additions & 23 deletions

File tree

src/errors.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,19 @@ pub enum PagedbError {
169169
#[error("recorded rekey replacement segment {replacement_segment_id:?} is missing or invalid")]
170170
RekeyReplacementMissing { replacement_segment_id: [u8; 16] },
171171

172+
/// A VFS backend reported positional-I/O progress that cannot be true —
173+
/// more bytes transferred than the caller's remaining buffer.
174+
///
175+
/// Not corruption: nothing on disk is known to be wrong. The backend broke
176+
/// the [`VfsFile`](crate::vfs::VfsFile) contract, so the reported count
177+
/// cannot be reasoned about at all and the transfer stops instead of
178+
/// advancing by a length it cannot trust.
179+
#[error("vfs backend violated the {operation} contract: {detail}")]
180+
VfsContractViolated {
181+
operation: &'static str,
182+
detail: &'static str,
183+
},
184+
172185
#[error("unsupported by backend")]
173186
Unsupported,
174187

@@ -473,6 +486,13 @@ impl PagedbError {
473486
Self::ArithmeticOverflow { operation }
474487
}
475488

489+
/// Canonical constructor for a VFS backend that broke the positional-I/O
490+
/// contract.
491+
#[must_use]
492+
pub const fn vfs_contract_violated(operation: &'static str, detail: &'static str) -> Self {
493+
Self::VfsContractViolated { operation, detail }
494+
}
495+
476496
/// Canonical constructor for a rekey admission that needs both KEKs.
477497
#[must_use]
478498
pub const fn rekey_resume_key_required(source_epoch: u64, target_epoch: u64) -> Self {

src/pager/core.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -741,6 +741,12 @@ impl<V: Vfs> Pager<V> {
741741
}
742742
let mut buf = vec![0u8; page_size];
743743
{
744+
// Returned rather than retried, unlike the AEAD failures below.
745+
// The transient this loop absorbs is a *torn* read — a
746+
// full-length buffer of mixed old and new bytes — which is why
747+
// retrying it can succeed. A transfer that ends short or errors
748+
// is not that condition, and retrying it would only mask a
749+
// one-shot backend fault.
744750
let mut f = file_handle.lock().await;
745751
read_exact_at(&mut *f, page_offset, &mut buf).await?;
746752
}

src/vfs/mod.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ pub use iouring::{IouringFile, IouringVfs};
3939
#[cfg(feature = "opfs")]
4040
pub use opfs::OpfsVfs;
4141
pub use traits::{Vfs, VfsFile};
42-
pub(crate) use traits::{checked_read_progress, read_exact_at, write_all_at};
42+
pub(crate) use traits::{
43+
checked_read_progress, read_exact_at, read_exact_at_borrowed, write_all_at,
44+
};
4345
pub use types::{OpenMode, ReadReq, WriteReq};
4446
pub use wasi::WasiVfs;
4547

src/vfs/traits.rs

Lines changed: 126 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,13 @@ pub trait VfsFile: Send {
9191
}
9292

9393
/// Read until `buf` is full, or fail if the backend stops making progress.
94+
///
95+
/// Takes `&mut F` rather than `&F` deliberately. `read_at` only needs `&self`,
96+
/// but a future holding `&F` across an await is `Send` only when `F: Sync`,
97+
/// and `VfsFile` does not require `Sync`. `&mut F` keeps the future `Send` for
98+
/// every backend. Callers that own their handle use this; those that read
99+
/// through a borrowed handle use [`read_exact_at_borrowed!`], which runs the
100+
/// same loop in place.
94101
#[inline]
95102
pub(crate) async fn read_exact_at<F: VfsFile + ?Sized>(
96103
file: &mut F,
@@ -106,27 +113,22 @@ pub(crate) async fn read_exact_at<F: VfsFile + ?Sized>(
106113
}
107114

108115
/// Validate one positional read result and advance its offset.
116+
///
117+
/// A backend may legally satisfy a request in several calls, so a short read is
118+
/// not itself failure. Zero progress is end-of-file; a count above the
119+
/// remaining buffer is a backend contract violation, not an on-disk defect.
109120
#[inline]
110121
pub(crate) fn checked_read_progress(offset: &mut u64, read: usize, remaining: usize) -> Result<()> {
111122
if read == 0 {
112123
return Err(PagedbError::Io(std::io::Error::from(
113124
std::io::ErrorKind::UnexpectedEof,
114125
)));
115126
}
116-
if read > remaining {
117-
return Err(PagedbError::Io(std::io::Error::other(
118-
"read_at overreported bytes",
119-
)));
120-
}
121-
let read_u64 = u64::try_from(read)
122-
.map_err(|_| PagedbError::Io(std::io::Error::other("read count overflow")))?;
123-
*offset = offset
124-
.checked_add(read_u64)
125-
.ok_or_else(|| PagedbError::Io(std::io::Error::other("offset overflow")))?;
126-
Ok(())
127+
checked_transfer_progress(offset, read, remaining, "read_at", "positional read offset")
127128
}
128129

129-
/// Write until `buf` is complete, or fail if the backend reports impossible progress.
130+
/// Write until `buf` is complete, or fail if the backend reports impossible
131+
/// progress. See [`read_exact_at`] for why this takes `&mut F`.
130132
#[inline]
131133
pub(crate) async fn write_all_at<F: VfsFile + ?Sized>(
132134
file: &mut F,
@@ -140,17 +142,119 @@ pub(crate) async fn write_all_at<F: VfsFile + ?Sized>(
140142
std::io::ErrorKind::WriteZero,
141143
)));
142144
}
143-
if written > buf.len() {
144-
return Err(PagedbError::Io(std::io::Error::other(
145-
"write_at overreported bytes",
146-
)));
147-
}
148-
let written_u64 = u64::try_from(written)
149-
.map_err(|_| PagedbError::Io(std::io::Error::other("write count overflow")))?;
150-
offset = offset
151-
.checked_add(written_u64)
152-
.ok_or_else(|| PagedbError::Io(std::io::Error::other("offset overflow")))?;
145+
checked_transfer_progress(
146+
&mut offset,
147+
written,
148+
buf.len(),
149+
"write_at",
150+
"positional write offset",
151+
)?;
153152
buf = &buf[written..];
154153
}
155154
Ok(())
156155
}
156+
157+
/// Run [`read_exact_at`]'s loop over a handle the caller only borrows.
158+
///
159+
/// A function taking `&F` would be `Send` only under `F: Sync`, which
160+
/// `VfsFile` does not require and which would spread as a bound through every
161+
/// segment reader. Expanding in place keeps the borrow inside the caller's own
162+
/// future, so the rule stays in one place without costing a trait bound.
163+
///
164+
/// `$buf` must be a `&mut [u8]`; the result is a `Result<()>` to be `?`-ed.
165+
macro_rules! read_exact_at_borrowed {
166+
($file:expr, $offset:expr, $buf:expr $(,)?) => {{
167+
let mut offset: u64 = $offset;
168+
let mut remaining: &mut [u8] = $buf;
169+
loop {
170+
if remaining.is_empty() {
171+
break Ok(());
172+
}
173+
match $file.read_at(offset, remaining).await {
174+
Ok(read) => {
175+
if let Err(error) =
176+
$crate::vfs::checked_read_progress(&mut offset, read, remaining.len())
177+
{
178+
break Err(error);
179+
}
180+
remaining = remaining.split_at_mut(read).1;
181+
}
182+
Err(error) => break Err(error),
183+
}
184+
}
185+
}};
186+
}
187+
pub(crate) use read_exact_at_borrowed;
188+
189+
/// Shared progress rule for both directions: reject a count the caller never
190+
/// asked for, then advance the offset without wrapping.
191+
#[inline]
192+
fn checked_transfer_progress(
193+
offset: &mut u64,
194+
transferred: usize,
195+
remaining: usize,
196+
operation: &'static str,
197+
offset_label: &'static str,
198+
) -> Result<()> {
199+
if transferred > remaining {
200+
return Err(PagedbError::vfs_contract_violated(
201+
operation,
202+
"reported more bytes than the caller requested",
203+
));
204+
}
205+
let transferred =
206+
u64::try_from(transferred).map_err(|_| PagedbError::arithmetic_overflow(offset_label))?;
207+
*offset = offset
208+
.checked_add(transferred)
209+
.ok_or_else(|| PagedbError::arithmetic_overflow(offset_label))?;
210+
Ok(())
211+
}
212+
213+
#[cfg(test)]
214+
mod tests {
215+
use super::*;
216+
217+
#[test]
218+
fn a_short_read_advances_by_exactly_what_was_transferred() {
219+
let mut offset = 4096;
220+
checked_read_progress(&mut offset, 100, 4096).unwrap();
221+
assert_eq!(offset, 4196);
222+
}
223+
224+
#[test]
225+
fn zero_progress_is_end_of_file_not_a_contract_violation() {
226+
let mut offset = 0;
227+
let err = checked_read_progress(&mut offset, 0, 4096).unwrap_err();
228+
assert!(
229+
matches!(err, PagedbError::Io(ref io) if io.kind() == std::io::ErrorKind::UnexpectedEof),
230+
"expected UnexpectedEof, got {err:?}"
231+
);
232+
assert_eq!(offset, 0, "a failed transfer must not advance the offset");
233+
}
234+
235+
#[test]
236+
fn a_count_above_the_remaining_buffer_is_a_backend_contract_violation() {
237+
let mut offset = 0;
238+
let err = checked_read_progress(&mut offset, 4097, 4096).unwrap_err();
239+
assert!(
240+
matches!(
241+
err,
242+
PagedbError::VfsContractViolated {
243+
operation: "read_at",
244+
..
245+
}
246+
),
247+
"expected VfsContractViolated, got {err:?}"
248+
);
249+
}
250+
251+
#[test]
252+
fn an_offset_that_would_wrap_is_reported_as_overflow() {
253+
let mut offset = u64::MAX;
254+
let err = checked_read_progress(&mut offset, 1, 4096).unwrap_err();
255+
assert!(
256+
matches!(err, PagedbError::ArithmeticOverflow { .. }),
257+
"expected ArithmeticOverflow, got {err:?}"
258+
);
259+
}
260+
}

tests/smoke.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,10 @@ fn every_variant_displays() {
4242
PagedbError::ChecksumFailure,
4343
PagedbError::corruption(CorruptionDetail::HeaderUnverifiable),
4444
PagedbError::structural_header_invalid("main.db", "magic"),
45+
PagedbError::vfs_contract_violated(
46+
"read_at",
47+
"reported more bytes than the caller requested",
48+
),
4549
PagedbError::footer_framing_invalid("magic"),
4650
PagedbError::node_body_malformed("slot_directory"),
4751
PagedbError::node_kind_mismatch(Some(7), "leaf", "internal"),

0 commit comments

Comments
 (0)