diff --git a/CHANGELOG.md b/CHANGELOG.md index e4b0d9cd..518ab5d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,15 @@ # FUSE for Rust - Changelog ## Unreleased +* Dropping a `BackgroundSession` now unmounts the filesystem and waits for the session to + end, guaranteeing that `Filesystem::destroy` has run when drop returns (#239, #411). This + restores the pre-0.16 blocking drop behavior. Drop does not wait when the session cannot + end: sessions created via `Session::from_fd` are left detached (use `join()` after ending + them), and so are sessions whose unmount failed or whose connection is still alive a few + seconds after the unmount (e.g. a lazily unmounted filesystem still in use). + The `guard` field of `BackgroundSession` is now private - use `join()`/`umount_and_join()` +* The pure-rust and `libfuse2` mount backends now report `fusermount` unmount failures + instead of silently ignoring them * Treat `ECONNABORTED` from the FUSE device as a clean session end (#212): with `FUSE_ABORT_ERROR` negotiated, aborting the connection made `Session::run()` and `BackgroundSession::umount_and_join()` return an error instead of ending normally diff --git a/src/channel.rs b/src/channel.rs index 0bedc118..afffdbd2 100644 --- a/src/channel.rs +++ b/src/channel.rs @@ -47,6 +47,11 @@ impl Channel { } } + /// Returns the FUSE device of this channel. + pub(crate) fn device(&self) -> Arc { + self.0.clone() + } + /// Returns a sender object for this channel. The sender object can be /// used to send to the channel. Multiple sender objects can be used /// and they can safely be sent to other threads. diff --git a/src/mnt/fuse2.rs b/src/mnt/fuse2.rs index b978f363..e883940c 100644 --- a/src/mnt/fuse2.rs +++ b/src/mnt/fuse2.rs @@ -51,8 +51,8 @@ impl MountImpl { // directly, which is what osxfuse does anyway, since we already converted // to the real path when we first mounted. if let Err(err) = crate::mnt::libc_umount(&self.mountpoint) { - // Linux always returns EPERM for non-root users. We have to let the - // library go through the setuid-root "fusermount -u" to unmount. + // Linux always returns EPERM for non-root users. We have to go + // through the setuid-root "fusermount -u" to unmount. if err == nix::errno::Errno::EPERM { #[cfg(not(any( target_os = "macos", @@ -61,9 +61,12 @@ impl MountImpl { target_os = "openbsd", target_os = "netbsd" )))] - unsafe { - fuse_unmount_compat22(self.mountpoint.as_ptr()); - return Ok(()); + { + // Not libfuse's fuse_unmount_compat22: that would swallow + // fusermount failures (leaving a still-running session that + // callers then believe has been unmounted) and stat the + // mountpoint through the filesystem via realpath + return crate::mnt::fusermount_unmount("fusermount", &self.mountpoint); } } return Err(err.into()); diff --git a/src/mnt/fuse2_sys.rs b/src/mnt/fuse2_sys.rs index 2cc69dcc..55a98dbf 100644 --- a/src/mnt/fuse2_sys.rs +++ b/src/mnt/fuse2_sys.rs @@ -23,12 +23,4 @@ unsafe extern "C" { // Therefore, the minimum version requirement for *_compat25 functions is libfuse-2.6.0. pub(crate) fn fuse_mount_compat25(mountpoint: *const c_char, args: *const fuse_args) -> c_int; - #[cfg(not(any( - target_os = "macos", - target_os = "freebsd", - target_os = "dragonfly", - target_os = "openbsd", - target_os = "netbsd" - )))] - pub(crate) fn fuse_unmount_compat22(mountpoint: *const c_char); } diff --git a/src/mnt/fuse_pure.rs b/src/mnt/fuse_pure.rs index dbd2f84d..383cc6b8 100644 --- a/src/mnt/fuse_pure.rs +++ b/src/mnt/fuse_pure.rs @@ -84,7 +84,7 @@ impl MountImpl { if err == nix::errno::Errno::EPERM { // Linux always returns EPERM for non-root users. We have to let the // library go through the setuid-root "fusermount -u" to unmount. - fuse_unmount_pure(&self.mountpoint); + fuse_unmount_pure(&self.mountpoint)?; return Ok(()); } else { return Err(err.into()); @@ -120,33 +120,20 @@ fn fuse_mount_pure( fuse_mount_fusermount(mountpoint, options, acl) } -fn fuse_unmount_pure(mountpoint: &CStr) { +fn fuse_unmount_pure(mountpoint: &CStr) -> io::Result<()> { #[cfg(target_os = "linux")] { if nix::mount::umount2(mountpoint, nix::mount::MntFlags::MNT_DETACH).is_ok() { - return; + return Ok(()); } } #[cfg(target_os = "macos")] { if nix::mount::unmount(mountpoint, nix::mount::MntFlags::MNT_FORCE).is_ok() { - return; + return Ok(()); } } - - let mut builder = Command::new(detect_fusermount_bin()); - builder.stdout(Stdio::piped()).stderr(Stdio::piped()); - builder - .arg("-u") - .arg("-q") - .arg("-z") - .arg("--") - .arg(OsStr::new(&mountpoint.to_string_lossy().into_owned())); - - if let Ok(output) = builder.output() { - debug!("fusermount: {}", String::from_utf8_lossy(&output.stdout)); - debug!("fusermount: {}", String::from_utf8_lossy(&output.stderr)); - } + crate::mnt::fusermount_unmount(&detect_fusermount_bin(), mountpoint) } fn detect_fusermount_bin() -> String { diff --git a/src/mnt/mod.rs b/src/mnt/mod.rs index a243e18c..d7e4c910 100644 --- a/src/mnt/mod.rs +++ b/src/mnt/mod.rs @@ -212,6 +212,75 @@ fn libc_umount(mnt: &CStr) -> nix::Result<()> { } } +/// Waits (bounded) for the FUSE kernel connection to end after an unmount was +/// requested. Returns false if the connection is still alive when the timeout +/// expires, e.g. because a lazily unmounted filesystem is still in use, or an +/// unmount helper failed in a way that cannot be observed directly. +#[cfg(not(target_os = "macos"))] +pub(crate) fn connection_ended(fuse_device: &DevFuse, timeout: std::time::Duration) -> bool { + let deadline = std::time::Instant::now() + timeout; + while is_mounted(fuse_device) { + if std::time::Instant::now() >= deadline { + return false; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + true +} + +/// The FUSE device poll semantics are untested on macOS, so assume the unmount +/// took effect there. +#[cfg(target_os = "macos")] +pub(crate) fn connection_ended(_fuse_device: &DevFuse, _timeout: std::time::Duration) -> bool { + true +} + +/// Lazily unmount via the given fusermount binary. Helper failures are reported, +/// so that callers do not mistake a still-mounted filesystem for an unmounted +/// one - BackgroundSession would otherwise wait forever for the session to end. +#[cfg(any( + all(test, target_os = "linux"), + fuser_mount_impl = "pure-rust", + all( + fuser_mount_impl = "libfuse2", + not(any( + target_os = "macos", + target_os = "freebsd", + target_os = "dragonfly", + target_os = "openbsd", + target_os = "netbsd" + )) + ) +))] +pub(crate) fn fusermount_unmount(fusermount_bin: &str, mountpoint: &CStr) -> io::Result<()> { + use std::os::unix::ffi::OsStrExt; + + use log::debug; + + let mut builder = std::process::Command::new(fusermount_bin); + builder + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()); + builder + .arg("-u") + .arg("-q") + .arg("-z") + .arg("--") + .arg(std::ffi::OsStr::from_bytes(mountpoint.to_bytes())); + + let output = builder.output()?; + debug!("fusermount: {}", String::from_utf8_lossy(&output.stdout)); + debug!("fusermount: {}", String::from_utf8_lossy(&output.stderr)); + if !output.status.success() { + return Err(io::Error::other(format!( + "fusermount failed to unmount ({}): {}", + output.status, + String::from_utf8_lossy(&output.stderr).trim() + ))); + } + Ok(()) +} + /// Warning: This will return true if the filesystem has been detached (lazy unmounted), but not /// yet destroyed by the kernel. #[cfg(not(target_os = "macos"))] @@ -276,6 +345,23 @@ mod test { ); } + /// A fusermount helper that cannot run or exits nonzero must report an error: + /// the filesystem is then still mounted, and treating that as success would + /// make teardown wait forever for a session that never ends. + #[test] + #[cfg(target_os = "linux")] + fn fusermount_unmount_reports_failure() { + use std::ffi::CString; + + let mountpoint = CString::new("/nonexistent").unwrap(); + // Helper exits nonzero (/bin/false ignores its arguments) + assert!(fusermount_unmount("/bin/false", &mountpoint).is_err()); + // Helper cannot be executed at all + assert!(fusermount_unmount("/nonexistent/fusermount", &mountpoint).is_err()); + // Helper reports success + assert!(fusermount_unmount("/bin/true", &mountpoint).is_ok()); + } + #[cfg(not(target_os = "macos"))] fn cmd_mount() -> String { std::str::from_utf8( diff --git a/src/session.rs b/src/session.rs index 0a4a6c12..dd957119 100644 --- a/src/session.rs +++ b/src/session.rs @@ -227,17 +227,21 @@ impl Session { } /// Run the session loop in a background thread. If the returned handle is dropped, - /// the filesystem is unmounted and the given session ends. + /// the filesystem is unmounted and the session is waited for, so that + /// `Filesystem::destroy` has run when drop returns - except in the cases documented + /// on [`BackgroundSession`], where the session thread is detached instead. pub fn spawn(self) -> io::Result { let sender = self.ch.sender(); + let fuse_device = self.ch.device(); // Take the fuse_session, so that we can unmount it let mount = std::mem::take(&mut *self.mount.mount.lock()); let guard = thread::Builder::new() .name("fuser-bg".to_string()) .spawn(move || self.run())?; Ok(BackgroundSession { - guard, + guard: Some(guard), sender, + fuse_device, mount, }) } @@ -565,23 +569,51 @@ impl SessionEventLoop { } /// The background session data structure +/// +/// Dropping this unmounts the filesystem and blocks until the session has ended, +/// which guarantees that `Filesystem::destroy` has run when drop returns. Because +/// of that, it must not be dropped from within a filesystem callback, which runs +/// on the session thread being waited for. For a session created via +/// `Session::from_fd` there is no mount to remove, so dropping cannot end the +/// session and leaves its thread detached; end the session externally and use +/// `join` to wait for it instead. The thread is likewise left detached, with a +/// warning, rather than waiting for a session that may never end when the +/// unmount fails, or when the kernel connection is still alive a few seconds +/// after a successful unmount request (e.g. a lazily unmounted filesystem that +/// is still in use, or an unmount helper that failed without reporting it). #[derive(Debug)] pub struct BackgroundSession { - /// Thread guard of the background session - pub guard: JoinHandle>, + /// Thread guard of the background session. None once joined. + guard: Option>>, /// Object for creating Notifiers for client use sender: ChannelSender, + /// The FUSE device fd of the session's kernel connection + fuse_device: Arc, /// Ensures the filesystem is unmounted when the session ends mount: Option, } +/// How long teardown waits for the kernel connection to end after a successful +/// unmount request, before giving up on joining the session thread. The +/// connection can end asynchronously (auto_unmount, kernel teardown), but a few +/// seconds only pass without it ending when the session cannot be waited for, +/// e.g. a lazily unmounted filesystem still in use +const UNMOUNT_WAIT: std::time::Duration = std::time::Duration::from_secs(5); + impl BackgroundSession { - /// Unmount the filesystem and join the background thread. + /// Unmount the filesystem and join the background thread, returning the + /// session result. `Filesystem::destroy` has run when this returns. pub fn umount_and_join(mut self) -> io::Result<()> { if let Some(mount) = self.mount.take() { - mount.umount()?; + if let Err(err) = mount.umount() { + // The filesystem is still mounted and the session still running, + // so joining would block indefinitely; leave the thread detached, + // as before + self.guard.take(); + return Err(err); + } } - self.join() + self.join_impl() } /// Returns an object that can be used to send notifications to the kernel @@ -589,9 +621,17 @@ impl BackgroundSession { Notifier::new(self.sender.clone()) } - /// Join the filesystem thread. - pub fn join(self) -> io::Result<()> { - self.guard + /// Join the filesystem thread without unmounting first: blocks until + /// something else ends the session, e.g. an external unmount. + pub fn join(mut self) -> io::Result<()> { + self.join_impl() + } + + fn join_impl(&mut self) -> io::Result<()> { + let Some(guard) = self.guard.take() else { + return Ok(()); + }; + guard .join() .map_err(|_panic: Box| { io::Error::new( @@ -602,6 +642,36 @@ impl BackgroundSession { } } +impl Drop for BackgroundSession { + fn drop(&mut self) { + // Unmount and wait for the session to end, so that Filesystem::destroy + // has run by the time drop returns (issues #239 and #411). Without the + // join, the process could exit before the detached session thread got + // around to calling destroy + let Some(mount) = self.mount.take() else { + // No mount to remove (Session::from_fd): nothing here can end the + // session, so joining could block forever; leave the thread detached + return; + }; + if let Err(err) = mount.umount() { + // Still mounted, so the session is still running and joining would + // block indefinitely + warn!("Failed to unmount filesystem during drop: {err}"); + return; + } + // The connection can end asynchronously (e.g. auto_unmount), and some + // unmount helpers cannot report failures. Wait boundedly, and if the + // session lives on, detach rather than block indefinitely + if !crate::mnt::connection_ended(&self.fuse_device, UNMOUNT_WAIT) { + warn!("FUSE connection still alive after unmount; detaching session thread"); + return; + } + if let Err(err) = self.join_impl() { + warn!("Session ended with an error during drop: {err}"); + } + } +} + // The abort test uses fusectl, which only exists on Linux; on other targets the // whole module would be dead code #[cfg(all(test, target_os = "linux"))] @@ -675,6 +745,40 @@ mod test { ManuallyDrop::into_inner(tmp); } + /// Dropping a BackgroundSession must unmount the filesystem and wait for the + /// session to end, so that Filesystem::destroy has run when drop returns. + #[test] + fn drop_waits_for_destroy() { + struct DestroyFs { + destroyed: Arc, + } + impl Filesystem for DestroyFs { + fn destroy(&mut self) { + // Give drop a chance to return early: without the join, drop + // consistently wins this race and the assertion below fails + std::thread::sleep(std::time::Duration::from_millis(200)); + self.destroyed + .store(true, std::sync::atomic::Ordering::SeqCst); + } + } + + let tmp = ManuallyDrop::new(tempfile::tempdir().unwrap()); + let destroyed = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let fs = DestroyFs { + destroyed: destroyed.clone(), + }; + let bg = Session::new(fs, tmp.path(), &Config::default()) + .unwrap() + .spawn() + .unwrap(); + drop(bg); + assert!( + destroyed.load(std::sync::atomic::Ordering::SeqCst), + "destroy() must have been called by the time drop returns" + ); + ManuallyDrop::into_inner(tmp); + } + /// The fusectl abort file for the FUSE mount at `mountpoint`: the connection /// directory is named after the mount's anonymous device number. fn fusectl_abort_path(mountpoint: &Path) -> Option {