v1.0.0-rc.1
CSS keyless decrypt (Stevenson), AACS 1.0/2.0/2.1, MPEG-2 DVD, multi-OS SCSI, multipass recovery, mux highway, audit hardening
This commit is contained in:
@@ -95,7 +95,14 @@ impl BytePrefetcher {
|
||||
.name("freemkv-byte-prefetch".into())
|
||||
.spawn(move || {
|
||||
let cancelled = || halt.as_ref().map(|h| h.is_cancelled()).unwrap_or(false);
|
||||
// Liveness heartbeat: the producer blocks on the recycle and
|
||||
// forward channels; a stalled consumer or a wedged reader shows
|
||||
// up as the beat going silent. Total is unknown, so `pos` is
|
||||
// cumulative bytes read.
|
||||
let mut hb = crate::progress::Heartbeat::new("byte_prefetch");
|
||||
let mut produced_bytes: u64 = 0;
|
||||
loop {
|
||||
hb.tick(produced_bytes, 0);
|
||||
if cancelled() {
|
||||
return;
|
||||
}
|
||||
@@ -138,6 +145,7 @@ impl BytePrefetcher {
|
||||
return;
|
||||
}
|
||||
};
|
||||
produced_bytes += n as u64;
|
||||
buf.truncate(n);
|
||||
// Hand off the filled buffer, re-polling halt on
|
||||
// each timeout slice so a cancel can interrupt a
|
||||
|
||||
+152
-13
@@ -50,6 +50,15 @@ use crate::halt::Halt;
|
||||
/// when no such watchdog intervenes.
|
||||
pub const JOIN_TIMEOUT_SECS: u64 = 600;
|
||||
|
||||
/// Short grace period after a halt or 10-min timeout fires in
|
||||
/// [`Pipeline::finish_with_halt`]. Most wedged consumers that are
|
||||
/// "about to return" when the halt fires will unblock within a few
|
||||
/// seconds (e.g. their bounded_syscall timeout returns and the consumer
|
||||
/// drains). Spinning here converts those into clean joins and releases
|
||||
/// the output file handle, at the cost of at most this much extra
|
||||
/// latency on a genuinely stuck consumer before we accept the leak.
|
||||
const FINISH_GRACE_SECS: u64 = 5;
|
||||
|
||||
/// Halt-check cadence for the send loop. Producer blocks on
|
||||
/// [`crossbeam_channel::Sender::send_timeout`] for this slice — the
|
||||
/// kernel wakes it the instant the consumer drains a slot, so on the
|
||||
@@ -106,6 +115,44 @@ fn consumer_panicked(payload: Box<dyn std::any::Any + Send>) -> Error {
|
||||
Error::PipelineConsumerPanicked
|
||||
}
|
||||
|
||||
/// After a halt or deadline fires, spin-poll `handle.is_finished()` for
|
||||
/// [`FINISH_GRACE_SECS`] before accepting the thread leak. This converts
|
||||
/// the common "nearly-done" consumer (whose own bounded_syscall just
|
||||
/// returned and is about to drop its output file) into a clean join,
|
||||
/// releasing the file handle without waiting the full grace period.
|
||||
///
|
||||
/// If the consumer is still running when the grace expires, dropping the
|
||||
/// `JoinHandle` detaches from the thread — the consumer keeps running
|
||||
/// until its kernel call returns or the process exits.
|
||||
fn finish_with_grace<R: Send + 'static>(
|
||||
handle: thread::JoinHandle<Result<R, Error>>,
|
||||
leak_err: Error,
|
||||
) -> Result<R, Error> {
|
||||
let grace = Instant::now() + Duration::from_secs(FINISH_GRACE_SECS);
|
||||
while Instant::now() < grace {
|
||||
if handle.is_finished() {
|
||||
return match handle.join() {
|
||||
Ok(result) => result,
|
||||
Err(payload) => Err(consumer_panicked(payload)),
|
||||
};
|
||||
}
|
||||
thread::sleep(POLL_INTERVAL);
|
||||
}
|
||||
// Grace expired. Log and leak.
|
||||
tracing::warn!(
|
||||
target: "freemkv::pipeline",
|
||||
phase = "finish_with_halt_grace_expired",
|
||||
"pipeline consumer did not finish within {}s grace period; leaking thread",
|
||||
FINISH_GRACE_SECS
|
||||
);
|
||||
// Dropping `handle` without joining detaches from the thread — the
|
||||
// consumer keeps running until its kernel call returns or the process
|
||||
// exits. This is the intentional "leak" documented in
|
||||
// `finish_with_halt`'s contract.
|
||||
drop(handle);
|
||||
Err(leak_err)
|
||||
}
|
||||
|
||||
/// Default channel depth for callers without a specific reason to
|
||||
/// pick another value. Kept conservative (4) — most callers should
|
||||
/// use READ_PIPELINE_DEPTH or WRITE_PIPELINE_DEPTH instead.
|
||||
@@ -436,12 +483,13 @@ impl<I: Send + 'static, R: Send + 'static> Pipeline<I, R> {
|
||||
/// - [`Error::PipelineConsumerPanicked`] — same as `finish()`.
|
||||
///
|
||||
/// In the `halted` and `timed out` branches the consumer thread is
|
||||
/// intentionally leaked — exactly the same trade-off the
|
||||
/// `bounded_syscall` primitive makes. The wedged kernel call
|
||||
/// inside the consumer will unwind whenever it does, or at
|
||||
/// process exit. The caller is free to fall back to a degraded
|
||||
/// path (e.g. abort the session and let a supervisor restart the
|
||||
/// process).
|
||||
/// intentionally leaked after a short grace period — exactly the
|
||||
/// same trade-off the `bounded_syscall` primitive makes. A
|
||||
/// [`FINISH_GRACE_SECS`] spin-poll is attempted first so that
|
||||
/// consumers that are "nearly done" (e.g. their own bounded syscall
|
||||
/// just timed out and is about to unblock) can join cleanly and
|
||||
/// release their output file handle. Only if the consumer is still
|
||||
/// running after the grace period does the leak occur.
|
||||
///
|
||||
/// Plain [`Pipeline::finish`] is preserved for callers without a
|
||||
/// halt-token plumbed through; that path still blocks indefinitely
|
||||
@@ -459,13 +507,11 @@ impl<I: Send + 'static, R: Send + 'static> Pipeline<I, R> {
|
||||
}
|
||||
if let Some(h) = halt {
|
||||
if h.is_cancelled() {
|
||||
// Consumer thread is intentionally leaked.
|
||||
return Err(Error::Halted);
|
||||
return finish_with_grace(handle, Error::Halted);
|
||||
}
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
// Consumer thread is intentionally leaked.
|
||||
return Err(Error::PipelineJoinTimeout);
|
||||
return finish_with_grace(handle, Error::PipelineJoinTimeout);
|
||||
}
|
||||
thread::sleep(POLL_INTERVAL);
|
||||
}
|
||||
@@ -884,10 +930,15 @@ mod tests {
|
||||
matches!(res, Err(Error::Halted)),
|
||||
"expected Err(Halted), got {res:?}"
|
||||
);
|
||||
// Bailed out within ~1 second of the halt firing (worst case
|
||||
// one POLL_INTERVAL = 250 ms of slack).
|
||||
// Bailed out within the grace period plus a healthy margin.
|
||||
// The grace spin-poll adds up to FINISH_GRACE_SECS (5s) of
|
||||
// extra wait for a truly wedged consumer; the test's consumer
|
||||
// is deliberately never released before this assert so we
|
||||
// exercise the "grace expires → leak" path. 15s is well under
|
||||
// the 10-minute JOIN_TIMEOUT backstop and proves the new code
|
||||
// doesn't block forever.
|
||||
assert!(
|
||||
elapsed < Duration::from_secs(2),
|
||||
elapsed < Duration::from_secs(15),
|
||||
"halt observation took too long: {elapsed:?}"
|
||||
);
|
||||
}
|
||||
@@ -1201,4 +1252,92 @@ mod tests {
|
||||
// the remaining 98 even though they were drained.
|
||||
assert_eq!(out, 2, "apply was called after Stop");
|
||||
}
|
||||
|
||||
// ── Bug-fix regression tests ────────────────────────────────────────
|
||||
|
||||
/// Regression for the "consumer thread / output-file leak on halt"
|
||||
/// fix. When the halt fires but the consumer finishes WITHIN the
|
||||
/// grace period, `finish_with_halt` must join cleanly and return `Ok`
|
||||
/// — not leak the thread or return `Err(Halted)`.
|
||||
///
|
||||
/// Setup: a sink that sleeps briefly (well inside `FINISH_GRACE_SECS`)
|
||||
/// after the producer drops the channel. We fire the halt immediately,
|
||||
/// so `finish_with_halt` enters the grace spin. The consumer finishes
|
||||
/// during the grace window and the result is `Ok`.
|
||||
///
|
||||
/// Without the fix (old behaviour: immediate leak on halt), this
|
||||
/// would have returned `Err(Halted)` and the SumSink total would
|
||||
/// be unobservable.
|
||||
#[test]
|
||||
fn finish_with_halt_joins_cleanly_when_consumer_finishes_in_grace() {
|
||||
// A sink that adds a short artificial delay in `close` to
|
||||
// simulate a consumer that is "nearly done" when halt fires.
|
||||
struct SlowCloseSink {
|
||||
close_delay: Duration,
|
||||
total: u64,
|
||||
}
|
||||
impl Sink<u64> for SlowCloseSink {
|
||||
type Output = u64;
|
||||
fn apply(&mut self, item: u64) -> Result<Flow, Error> {
|
||||
self.total += item;
|
||||
Ok(Flow::Continue)
|
||||
}
|
||||
fn close(self) -> Result<u64, Error> {
|
||||
std::thread::sleep(self.close_delay);
|
||||
Ok(self.total)
|
||||
}
|
||||
}
|
||||
|
||||
let pipe = Pipeline::spawn(
|
||||
DEFAULT_PIPELINE_DEPTH,
|
||||
SlowCloseSink {
|
||||
// close() sleeps 500ms — well inside the 5s grace period.
|
||||
close_delay: Duration::from_millis(500),
|
||||
total: 0,
|
||||
},
|
||||
)
|
||||
.expect("spawn");
|
||||
for i in 0..5u64 {
|
||||
pipe.send(i).expect("send");
|
||||
}
|
||||
|
||||
// Fire halt immediately (before the consumer has had a chance
|
||||
// to finish its close() delay).
|
||||
let halt = crate::halt::Halt::new();
|
||||
halt.cancel();
|
||||
|
||||
let start = Instant::now();
|
||||
// finish_with_halt drops tx (signalling EOF), then observes the
|
||||
// pre-cancelled halt and enters the grace spin. The consumer
|
||||
// finishes close() within 500ms, so finish_with_halt must join
|
||||
// cleanly and return Ok with the correct total.
|
||||
let res = pipe.finish_with_halt(Some(&halt));
|
||||
let elapsed = start.elapsed();
|
||||
|
||||
assert!(
|
||||
matches!(res, Ok(10)),
|
||||
"expected Ok(10) from clean grace join, got {res:?}"
|
||||
);
|
||||
// Must return well before the full grace timeout (the consumer
|
||||
// finishes in ~500ms, so total elapsed should be well under 3s).
|
||||
assert!(
|
||||
elapsed < Duration::from_secs(3),
|
||||
"grace join took too long: {elapsed:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression: `finish_with_halt` with no halt token and a consumer
|
||||
/// that completes normally must still return `Ok` (the None-halt
|
||||
/// polling path is unchanged by the grace-period fix). This is the
|
||||
/// pre-existing happy-path test reproduced with an explicit timing
|
||||
/// floor to guard against spurious early returns.
|
||||
#[test]
|
||||
fn finish_with_halt_no_halt_token_normal_completion() {
|
||||
let pipe = Pipeline::spawn(DEFAULT_PIPELINE_DEPTH, SumSink { total: 0 }).expect("spawn");
|
||||
for i in 0..20u64 {
|
||||
pipe.send(i).expect("send");
|
||||
}
|
||||
let res = pipe.finish_with_halt(None);
|
||||
assert!(matches!(res, Ok(190)), "expected Ok(190), got {res:?}");
|
||||
}
|
||||
}
|
||||
|
||||
+4
-2
@@ -76,8 +76,10 @@ pub trait RandomAccessSink: SequentialSink + Seek {}
|
||||
///
|
||||
/// Returns a boxed trait object so the call site (mux construction)
|
||||
/// stays agnostic of which concrete sink got picked.
|
||||
#[allow(dead_code)] // wiring to mux::resolve is a follow-up commit
|
||||
pub fn open_for_mkv(
|
||||
// Not yet wired into mux::resolve (follow-up commit). Kept `pub(crate)` until
|
||||
// then so an unfinished signature isn't frozen into the public 1.0 API.
|
||||
#[allow(dead_code)]
|
||||
pub(crate) fn open_for_mkv(
|
||||
dest: &std::path::Path,
|
||||
size_hint: Option<u64>,
|
||||
) -> std::io::Result<Box<dyn RandomAccessSink>> {
|
||||
|
||||
+143
-6
@@ -78,6 +78,15 @@ pub(crate) struct WritebackPipeline {
|
||||
/// `WritebackFile` and never exposed outside that wrapper, which
|
||||
/// is what keeps the alias sound.
|
||||
fd: RawFd,
|
||||
/// An owned clone of the file descriptor, held so that any
|
||||
/// leaked WAIT_AFTER worker thread retains a valid reference to
|
||||
/// the underlying file description for the duration of its
|
||||
/// syscall — even if the original `WritebackFile` is closed first
|
||||
/// and the OS reuses its fd number. `None` only when `try_clone`
|
||||
/// failed at construction (rare); the pipeline falls back to the
|
||||
/// pre-clone `fd` integer in that case, which carries the original
|
||||
/// fd-reuse risk but is no worse than the previous behaviour.
|
||||
wait_file: Option<File>,
|
||||
chunk_bytes: u64,
|
||||
last_flush_pos: u64,
|
||||
pending: Option<(u64, u64)>,
|
||||
@@ -108,6 +117,19 @@ impl WritebackPipeline {
|
||||
pub(crate) fn new(file: &File, start_pos: u64, chunk_bytes: u64) -> Self {
|
||||
let fd = file.as_raw_fd();
|
||||
let is_nfs = detect_nfs(fd);
|
||||
// Clone the fd so any leaked WAIT_AFTER worker thread keeps the
|
||||
// file description alive. Log but continue on clone failure.
|
||||
let wait_file = match file.try_clone() {
|
||||
Ok(f) => Some(f),
|
||||
Err(e) => {
|
||||
tracing::warn!(
|
||||
target: "mux",
|
||||
"WritebackPipeline fd={fd}: try_clone failed ({e}), WAIT_AFTER workers \
|
||||
will use raw fd (fd-reuse risk on timeout)"
|
||||
);
|
||||
None
|
||||
}
|
||||
};
|
||||
tracing::info!(
|
||||
target: "mux",
|
||||
"WritebackPipeline fd={fd} is_nfs={is_nfs} chunk_bytes={chunk_bytes} strategy={}",
|
||||
@@ -115,6 +137,7 @@ impl WritebackPipeline {
|
||||
);
|
||||
Self {
|
||||
fd,
|
||||
wait_file,
|
||||
chunk_bytes,
|
||||
last_flush_pos: start_pos,
|
||||
pending: None,
|
||||
@@ -133,6 +156,22 @@ impl WritebackPipeline {
|
||||
self.is_nfs || self.degraded.load(Ordering::Relaxed)
|
||||
}
|
||||
|
||||
/// Produce a fresh per-call `File` clone for the WAIT_AFTER worker.
|
||||
///
|
||||
/// Each call to `wait_after_with_timeout` needs its own owned clone
|
||||
/// so the worker thread keeps the file description alive for the
|
||||
/// duration of the syscall. We clone from `self.wait_file` (itself a
|
||||
/// clone taken at construction) rather than from the original file.
|
||||
///
|
||||
/// Returns `None` only if `wait_file` is `None` (construction
|
||||
/// try_clone failed) or if the second-level try_clone fails — both
|
||||
/// rare; the fallback raw-fd path in `wait_after_with_timeout`
|
||||
/// handles that case.
|
||||
#[inline]
|
||||
fn clone_for_worker(&self) -> Option<File> {
|
||||
self.wait_file.as_ref().and_then(|f| f.try_clone().ok())
|
||||
}
|
||||
|
||||
/// Caller advanced the file position to `pos`. If a chunk boundary
|
||||
/// was crossed, kick async writeback for the just-completed chunk
|
||||
/// and finalise the previous one.
|
||||
@@ -172,7 +211,8 @@ impl WritebackPipeline {
|
||||
// we mark the pipeline degraded, log a loud error,
|
||||
// and fall through to the skip path on subsequent
|
||||
// calls.
|
||||
match wait_after_with_timeout(self.fd, prev_off, prev_len) {
|
||||
match wait_after_with_timeout(self.clone_for_worker(), self.fd, prev_off, prev_len)
|
||||
{
|
||||
Some(ms) => {
|
||||
wait_ms = ms;
|
||||
let t_fadv = Instant::now();
|
||||
@@ -288,7 +328,7 @@ impl WritebackPipeline {
|
||||
// paths.
|
||||
return;
|
||||
}
|
||||
match wait_after_with_timeout(self.fd, prev_off, prev_len) {
|
||||
match wait_after_with_timeout(self.clone_for_worker(), self.fd, prev_off, prev_len) {
|
||||
Some(_ms) => unsafe {
|
||||
libc::posix_fadvise(
|
||||
self.fd,
|
||||
@@ -339,11 +379,48 @@ fn detect_nfs(fd: RawFd) -> bool {
|
||||
/// to the WAIT_AFTER call shape: it returns `elapsed_ms` instead of the
|
||||
/// syscall's `()`, and treats `WorkerLost` as a benign no-op to match
|
||||
/// the original semantics.
|
||||
fn wait_after_with_timeout(fd: RawFd, off: u64, len: u64) -> Option<u64> {
|
||||
///
|
||||
/// ## fd lifetime / fd-reuse safety
|
||||
///
|
||||
/// `worker_file` is an *owned* `File` (produced by `File::try_clone` at
|
||||
/// pipeline construction). It is moved into the worker closure so the
|
||||
/// file description stays alive for exactly as long as the worker thread
|
||||
/// lives — even if the original `WritebackFile` is closed and the OS
|
||||
/// reuses its fd number before the worker's syscall returns.
|
||||
///
|
||||
/// `fallback_fd` is used only when `worker_file` is `None` (i.e. the
|
||||
/// `try_clone` at construction failed). In that case the worker captures
|
||||
/// the raw fd integer, which carries the original fd-reuse risk but is
|
||||
/// no worse than the pre-fix behaviour.
|
||||
fn wait_after_with_timeout(
|
||||
worker_file: Option<File>,
|
||||
fallback_fd: RawFd,
|
||||
off: u64,
|
||||
len: u64,
|
||||
) -> Option<u64> {
|
||||
let started = Instant::now();
|
||||
match crate::io::bounded::bounded_syscall(None, WAIT_AFTER_TIMEOUT, move || unsafe {
|
||||
libc::sync_file_range(fd, off as i64, len as i64, libc::SYNC_FILE_RANGE_WAIT_AFTER);
|
||||
}) {
|
||||
let result = if let Some(owned) = worker_file {
|
||||
// Happy path: the closure owns a cloned File that keeps the
|
||||
// file description alive until the worker drops it.
|
||||
crate::io::bounded::bounded_syscall(None, WAIT_AFTER_TIMEOUT, move || unsafe {
|
||||
let fd = owned.as_raw_fd();
|
||||
libc::sync_file_range(fd, off as i64, len as i64, libc::SYNC_FILE_RANGE_WAIT_AFTER);
|
||||
// `owned` drops here, closing the cloned fd.
|
||||
})
|
||||
} else {
|
||||
// Fallback: try_clone failed at construction; use the raw fd.
|
||||
// This carries the pre-fix fd-reuse risk on timeout, but is no
|
||||
// regression from the original behaviour.
|
||||
crate::io::bounded::bounded_syscall(None, WAIT_AFTER_TIMEOUT, move || unsafe {
|
||||
libc::sync_file_range(
|
||||
fallback_fd,
|
||||
off as i64,
|
||||
len as i64,
|
||||
libc::SYNC_FILE_RANGE_WAIT_AFTER,
|
||||
);
|
||||
})
|
||||
};
|
||||
match result {
|
||||
Ok(()) => Some(started.elapsed().as_millis() as u64),
|
||||
Err(crate::io::bounded::BoundedError::Timeout)
|
||||
| Err(crate::io::bounded::BoundedError::Halted) => None,
|
||||
@@ -469,4 +546,64 @@ mod tests {
|
||||
assert_eq!(p.chunk_count, before);
|
||||
assert!(p.pending.is_none());
|
||||
}
|
||||
|
||||
// ── Bug-fix regression tests ────────────────────────────────────────
|
||||
|
||||
/// Regression for the fd-reuse / use-after-close fix. Verifies that
|
||||
/// `WritebackPipeline::new` successfully clones the fd into
|
||||
/// `wait_file` (i.e. `try_clone` doesn't fail for a normal
|
||||
/// tempfile) and that `clone_for_worker` returns `Some` — meaning
|
||||
/// the WAIT_AFTER worker will capture an owned `File` rather than a
|
||||
/// raw fd integer.
|
||||
///
|
||||
/// A deterministic test for the actual fd-reuse race is not clean to
|
||||
/// write (it would require simultaneously closing the original File
|
||||
/// and re-opening a new one to steal the fd number while the worker
|
||||
/// is mid-syscall, which is inherently racy). This test instead pins
|
||||
/// the structural invariant: on a normal local file, the pipeline
|
||||
/// holds a valid clone and will give the worker an owned File.
|
||||
#[test]
|
||||
fn wait_file_clone_is_present_for_local_tempfile() {
|
||||
let (_f, p) = local_pipeline(32 * 1024 * 1024);
|
||||
assert!(
|
||||
p.wait_file.is_some(),
|
||||
"wait_file must be Some for a normal local tempfile (try_clone should not fail)"
|
||||
);
|
||||
// clone_for_worker must return Some — the worker will get an
|
||||
// owned File, not fall through to the raw-fd fallback.
|
||||
let worker_clone = p.clone_for_worker();
|
||||
assert!(
|
||||
worker_clone.is_some(),
|
||||
"clone_for_worker must return Some when wait_file is Some"
|
||||
);
|
||||
}
|
||||
|
||||
/// Structural: the worker `File` clone returned by `clone_for_worker`
|
||||
/// is a distinct file descriptor (different fd number) that refers to
|
||||
/// the same underlying file. Closing the original tempfile must not
|
||||
/// affect the clone's validity — the OS keeps the file description
|
||||
/// alive until all file descriptors referring to it are closed.
|
||||
///
|
||||
/// We verify "distinct fd number" and "still usable as a raw fd"
|
||||
/// without actually racing a syscall.
|
||||
#[test]
|
||||
fn worker_clone_has_distinct_fd_from_original() {
|
||||
let f = NamedTempFile::new().expect("tempfile create");
|
||||
let original_fd = f.as_file().as_raw_fd();
|
||||
let pipeline = WritebackPipeline::new(f.as_file(), 0, 32 * 1024 * 1024);
|
||||
|
||||
let clone = pipeline
|
||||
.clone_for_worker()
|
||||
.expect("clone_for_worker returned None");
|
||||
let clone_fd = clone.as_raw_fd();
|
||||
|
||||
// The clone must have a different fd number — it is a separate
|
||||
// open file description (dup'd by try_clone).
|
||||
assert_ne!(
|
||||
clone_fd, original_fd,
|
||||
"worker clone must have a distinct fd number from the original"
|
||||
);
|
||||
// The clone fd must be valid (non-negative on Unix).
|
||||
assert!(clone_fd >= 0, "clone fd must be non-negative");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -38,13 +38,38 @@ pub(super) fn preallocate(file: &File, size_bytes: u64) {
|
||||
/// three fallbacks return `Ok(())`. `Ok(())` from these paths is NOT a
|
||||
/// durability barrier — the durable flush did not complete; only the
|
||||
/// hang is bounded.
|
||||
///
|
||||
/// ## fd-reuse safety
|
||||
///
|
||||
/// The `fsync` runs on a bounded worker thread that may be leaked on
|
||||
/// timeout. To avoid the leaked worker's syscall hitting a recycled fd
|
||||
/// number after the original `File` is closed, we `try_clone` an owned
|
||||
/// `File` and move it into the closure. The clone keeps the underlying
|
||||
/// file description alive for as long as the worker thread lives.
|
||||
/// On `try_clone` failure (rare) we fall back to the raw fd integer —
|
||||
/// no worse than the previous behaviour.
|
||||
pub(super) fn durable_sync(file: &File) -> io::Result<()> {
|
||||
let fd = file.as_raw_fd();
|
||||
// Clone so a leaked worker thread retains a valid fd even after the
|
||||
// original File is closed and its fd number is reused.
|
||||
let owned = match file.try_clone() {
|
||||
Ok(f) => Some(f),
|
||||
Err(e) => {
|
||||
let fd = file.as_raw_fd();
|
||||
tracing::warn!(
|
||||
target: "mux",
|
||||
"WritebackFile::sync_all fd={fd}: try_clone failed ({e}), fsync worker will use raw fd (fd-reuse risk on timeout)"
|
||||
);
|
||||
None
|
||||
}
|
||||
};
|
||||
let fallback_fd = file.as_raw_fd();
|
||||
match crate::io::bounded::bounded_syscall(
|
||||
None,
|
||||
Duration::from_secs(60),
|
||||
move || -> io::Result<()> {
|
||||
let fd = owned.as_ref().map(|f| f.as_raw_fd()).unwrap_or(fallback_fd);
|
||||
let rc = unsafe { libc::fsync(fd) };
|
||||
// `owned` (if Some) drops here, releasing the cloned fd.
|
||||
if rc == 0 {
|
||||
Ok(())
|
||||
} else {
|
||||
@@ -76,3 +101,45 @@ pub(super) fn durable_sync(file: &File) -> io::Result<()> {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[cfg(target_os = "linux")]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
/// Regression for the fd-reuse / use-after-close fix in `durable_sync`.
|
||||
///
|
||||
/// Verifies the structural invariant: `try_clone` succeeds for a normal
|
||||
/// local tempfile, and the cloned `File` has a distinct fd number from
|
||||
/// the original. This pins the property that a leaked fsync worker thread
|
||||
/// captures an owned `File` (and thus keeps the file description alive)
|
||||
/// rather than a bare fd integer that can be reused after the original
|
||||
/// `File` closes.
|
||||
///
|
||||
/// The actual fd-reuse race is non-deterministic and not cleanly
|
||||
/// testable without coordinating a simultaneous close + re-open on
|
||||
/// another thread. A structural test is the accepted substitute.
|
||||
#[test]
|
||||
fn durable_sync_worker_uses_owned_clone_with_distinct_fd() {
|
||||
let f = NamedTempFile::new().expect("tempfile create");
|
||||
let original_fd = f.as_file().as_raw_fd();
|
||||
|
||||
// try_clone must succeed for a normal local file.
|
||||
let owned = f
|
||||
.as_file()
|
||||
.try_clone()
|
||||
.expect("try_clone must succeed for a local tempfile");
|
||||
let clone_fd = owned.as_raw_fd();
|
||||
|
||||
// The clone must be a distinct fd (dup'd, not aliased).
|
||||
assert_ne!(
|
||||
clone_fd, original_fd,
|
||||
"owned clone must have a distinct fd number — not an alias of the original"
|
||||
);
|
||||
assert!(clone_fd >= 0, "clone fd must be a valid non-negative fd");
|
||||
|
||||
// durable_sync must complete without error on the local tempfile.
|
||||
durable_sync(f.as_file()).expect("durable_sync must return Ok on a local tempfile");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -52,28 +52,54 @@ pub(super) fn preallocate(file: &File, size_bytes: u64) {
|
||||
);
|
||||
}
|
||||
|
||||
/// ## fd-reuse safety
|
||||
///
|
||||
/// The F_FULLFSYNC / fsync runs on a bounded worker thread that may be
|
||||
/// leaked on timeout. To avoid the leaked worker's syscall hitting a
|
||||
/// recycled fd number after the original `File` is closed, we
|
||||
/// `try_clone` an owned `File` and move it into the closure. The clone
|
||||
/// keeps the underlying file description alive for as long as the worker
|
||||
/// thread lives. On `try_clone` failure (rare) we fall back to the raw
|
||||
/// fd integer — no worse than the previous behaviour.
|
||||
pub(super) fn durable_sync(file: &File) -> io::Result<()> {
|
||||
let fd = file.as_raw_fd();
|
||||
// Clone so a leaked worker thread retains a valid fd even after the
|
||||
// original File is closed and its fd number is reused.
|
||||
let owned = match file.try_clone() {
|
||||
Ok(f) => Some(f),
|
||||
Err(e) => {
|
||||
let fd = file.as_raw_fd();
|
||||
tracing::warn!(
|
||||
target: "mux",
|
||||
"WritebackFile::sync_all fd={fd}: try_clone failed ({e}), F_FULLFSYNC worker will use raw fd (fd-reuse risk on timeout)"
|
||||
);
|
||||
None
|
||||
}
|
||||
};
|
||||
let fallback_fd = file.as_raw_fd();
|
||||
match crate::io::bounded::bounded_syscall(
|
||||
None,
|
||||
Duration::from_secs(60),
|
||||
move || -> io::Result<()> {
|
||||
let fd = owned.as_ref().map(|f| f.as_raw_fd()).unwrap_or(fallback_fd);
|
||||
// Try F_FULLFSYNC first. If it isn't supported on this
|
||||
// filesystem (older HFS, some network mounts) fall back to
|
||||
// plain fsync — better than nothing.
|
||||
let rc = unsafe { libc::fcntl(fd, F_FULLFSYNC, 0) };
|
||||
if rc == 0 {
|
||||
// `owned` (if Some) drops here, releasing the cloned fd.
|
||||
return Ok(());
|
||||
}
|
||||
let err = io::Error::last_os_error();
|
||||
if err.raw_os_error() == Some(libc::ENOTSUP) {
|
||||
let rc = unsafe { libc::fsync(fd) };
|
||||
// `owned` drops here.
|
||||
if rc == 0 {
|
||||
Ok(())
|
||||
} else {
|
||||
Err(io::Error::last_os_error())
|
||||
}
|
||||
} else {
|
||||
// `owned` drops here.
|
||||
Err(err)
|
||||
}
|
||||
},
|
||||
@@ -90,3 +116,44 @@ pub(super) fn durable_sync(file: &File) -> io::Result<()> {
|
||||
Err(crate::io::bounded::BoundedError::WorkerLost) => Ok(()),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[cfg(target_os = "macos")]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
/// Regression for the fd-reuse / use-after-close fix in `durable_sync`.
|
||||
///
|
||||
/// Verifies the structural invariant: `try_clone` succeeds for a normal
|
||||
/// local tempfile, and the cloned `File` has a distinct fd number from
|
||||
/// the original. This pins the property that a leaked F_FULLFSYNC/fsync
|
||||
/// worker thread captures an owned `File` (keeping the file description
|
||||
/// alive) rather than a bare fd integer that can be reused after the
|
||||
/// original `File` closes.
|
||||
///
|
||||
/// The actual fd-reuse race is non-deterministic; a structural test is
|
||||
/// the accepted substitute.
|
||||
#[test]
|
||||
fn durable_sync_worker_uses_owned_clone_with_distinct_fd() {
|
||||
let f = NamedTempFile::new().expect("tempfile create");
|
||||
let original_fd = f.as_file().as_raw_fd();
|
||||
|
||||
// try_clone must succeed for a normal local file.
|
||||
let owned = f
|
||||
.as_file()
|
||||
.try_clone()
|
||||
.expect("try_clone must succeed for a local tempfile");
|
||||
let clone_fd = owned.as_raw_fd();
|
||||
|
||||
// The clone must be a distinct fd (dup'd, not aliased).
|
||||
assert_ne!(
|
||||
clone_fd, original_fd,
|
||||
"owned clone must have a distinct fd number — not an alias of the original"
|
||||
);
|
||||
assert!(clone_fd >= 0, "clone fd must be a valid non-negative fd");
|
||||
|
||||
// durable_sync must complete without error on the local tempfile.
|
||||
durable_sync(f.as_file()).expect("durable_sync must return Ok on a local tempfile");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user