Close host files when the last FD referencing them is closed

lib/virtual-fs/src/host_fs.rs

@@ -351,10 +351,10 @@ impl crate::FileOpener for FileSystem {
 pub struct File {
     #[cfg_attr(feature = "enable-serde", serde(skip, default = "default_handle"))]
     handle: Handle,
-    #[cfg_attr(feature = "enable-serde", serde(skip_serializing))]
-    inner_std: fs::File,
     #[cfg_attr(feature = "enable-serde", serde(skip))]
     inner: tfs::File,
+    #[cfg_attr(feature = "enable-serde", serde(skip_serializing))]
+    inner_std: fs::File,
     pub host_path: PathBuf,
     #[cfg(feature = "enable-serde")]
     flags: u16,
@@ -635,6 +635,12 @@ impl AsyncSeek for File {
     }
 }
 
+impl Drop for File {
+    fn drop(&mut self) {
+        tracing::trace!(?self.host_path, "Closing host file");
+    }
+}
+
 /// A wrapper type around Stdout that implements `VirtualFile`.
 #[derive(Debug)]
 #[cfg_attr(feature = "enable-serde", derive(Serialize, Deserialize))]

lib/wasix/src/fs/fd_list.rs

@@ -7,7 +7,7 @@
 use super::fd::Fd;
 use wasmer_wasix_types::wasi::Fd as WasiFd;
 
-#[derive(Debug, Clone)]
+#[derive(Debug)]
 pub struct FdList {
     fds: Vec<Option<Fd>>,
     first_free: Option<usize>,
@@ -62,6 +62,7 @@ impl FdList {
     }
 
     pub fn insert_first_free(&mut self, fd: Fd) -> WasiFd {
+        fd.inode.acquire_handle();
         match self.first_free {
             Some(free) => {
                 debug_assert!(self.fds[free].is_none());
@@ -106,6 +107,7 @@ impl FdList {
             return false;
         }
 
+        fd.inode.acquire_handle();
         self.fds[idx] = Some(fd);
         true
     }
@@ -115,18 +117,26 @@ impl FdList {
 
         let result = self.fds.get_mut(idx).and_then(|fd| fd.take());
 
-        if result.is_some() {
+        if let Some(fd) = result.as_ref() {
             match self.first_free {
                 None => self.first_free = Some(idx),
                 Some(x) if x > idx => self.first_free = Some(idx),
                 _ => (),
             }
+
+            fd.inode.drop_one_handle();
         }
 
         result
     }
 
     pub fn clear(&mut self) {
+        for fd in &self.fds {
+            if let Some(fd) = fd.as_ref() {
+                fd.inode.drop_one_handle();
+            }
+        }
+
         self.fds.clear();
         self.first_free = None;
     }
@@ -150,6 +160,21 @@ impl FdList {
     }
 }
 
+impl Clone for FdList {
+    fn clone(&self) -> Self {
+        for fd in &self.fds {
+            if let Some(fd) = fd.as_ref() {
+                fd.inode.acquire_handle();
+            }
+        }
+
+        Self {
+            fds: self.fds.clone(),
+            first_free: self.first_free.clone(),
+        }
+    }
+}
+
 impl<'a> Iterator for FdListIterator<'a> {
     type Item = (WasiFd, &'a Fd);
 
@@ -200,7 +225,10 @@ impl<'a> Iterator for FdListIteratorMut<'a> {
 mod tests {
     use std::{
         borrow::Cow,
-        sync::{atomic::AtomicU64, Arc, RwLock},
+        sync::{
+            atomic::{AtomicI32, AtomicU64},
+            Arc, RwLock,
+        },
     };
 
     use wasmer_wasix_types::wasi::{Fdflags, Rights};
@@ -221,6 +249,7 @@ mod tests {
                     name: RwLock::new(Cow::Borrowed("")),
                     stat: RwLock::new(Default::default()),
                 }),
+                open_handles: Arc::new(AtomicI32::new(0)),
             },
             is_stdio: false,
             offset: Arc::new(AtomicU64::new(0)),

lib/wasix/src/fs/mod.rs

@@ -17,7 +17,7 @@ use std::{
     path::{Component, Path, PathBuf},
     pin::Pin,
     sync::{
-        atomic::{AtomicBool, AtomicU64, Ordering},
+        atomic::{AtomicBool, AtomicI32, AtomicU64, Ordering},
         Arc, Mutex, RwLock, Weak,
     },
     task::{Context, Poll},
@@ -121,20 +121,62 @@ impl Inode {
 pub struct InodeGuard {
     ino: Inode,
     inner: Arc<InodeVal>,
+
+    // This exists because self.inner doesn't really represent the
+    // number of FDs referencing this InodeGuard. We need that number
+    // so we can know when to drop the file handle, which should result
+    // in the backing file (which may be a host file) getting closed.
+    open_handles: Arc<AtomicI32>,
 }
 impl InodeGuard {
     pub fn ino(&self) -> Inode {
         self.ino
     }
+
     pub fn downgrade(&self) -> InodeWeakGuard {
         InodeWeakGuard {
             ino: self.ino,
+            open_handles: self.open_handles.clone(),
             inner: Arc::downgrade(&self.inner),
         }
     }
+
     pub fn ref_cnt(&self) -> usize {
         Arc::strong_count(&self.inner)
     }
+
+    pub fn handle_count(&self) -> u32 {
+        self.open_handles.load(Ordering::SeqCst) as u32
+    }
+
+    pub fn acquire_handle(&self) {
+        self.open_handles.fetch_add(1, Ordering::SeqCst);
+    }
+
+    pub fn drop_one_handle(&self) {
+        if self.open_handles.fetch_sub(1, Ordering::SeqCst) != 1 {
+            return;
+        }
+
+        let mut guard = self.inner.write();
+
+        // Re-check the open handles to account for race conditions
+        if self.open_handles.load(Ordering::SeqCst) != 0 {
+            return;
+        }
+
+        let ino = self.ino.0;
+        trace!(%ino, "InodeGuard has no more open handles");
+
+        match guard.deref_mut() {
+            Kind::File { handle, .. } if handle.is_some() => {
+                let file_ref_count = Arc::strong_count(handle.as_ref().unwrap());
+                trace!(%file_ref_count, %ino, "dropping file handle");
+                drop(handle.take().unwrap());
+            }
+            _ => (),
+        }
+    }
 }
 impl std::ops::Deref for InodeGuard {
     type Target = InodeVal;
@@ -146,6 +188,10 @@ impl std::ops::Deref for InodeGuard {
 #[derive(Debug, Clone)]
 pub struct InodeWeakGuard {
     ino: Inode,
+    // Needed for when we want to upgrade back. We don't exactly
+    // care too much when the AtomicI32 is dropped, so this is
+    // a strong reference to keep things simple.
+    open_handles: Arc<AtomicI32>,
     inner: Weak<InodeVal>,
 }
 impl InodeWeakGuard {
@@ -155,6 +201,7 @@ impl InodeWeakGuard {
     pub fn upgrade(&self) -> Option<InodeGuard> {
         Weak::upgrade(&self.inner).map(|inner| InodeGuard {
             ino: self.ino,
+            open_handles: self.open_handles.clone(),
             inner,
         })
     }
@@ -198,7 +245,13 @@ impl WasiInodes {
             guard.lookup.retain(|_, v| Weak::strong_count(v) > 0);
         }
 
-        InodeGuard { ino, inner: val }
+        let open_handles = Arc::new(AtomicI32::new(0));
+
+        InodeGuard {
+            ino,
+            open_handles,
+            inner: val,
+        }
     }
 
     /// Get the `VirtualFile` object at stdout
@@ -687,7 +740,7 @@ impl WasiFs {
     /// Opens a user-supplied file in the directory specified with the
     /// name and flags given
     // dead code because this is an API for external use
-    // TODO: is this used anywhere?
+    // TODO: is this used anywhere? Is it even sound?
     #[allow(dead_code, clippy::too_many_arguments)]
     pub fn open_file_at(
         &mut self,

lib/wasix/src/syscalls/wasi/path_open.rs

@@ -244,6 +244,8 @@ pub(crate) fn path_open_internal(
                 if minimum_rights.truncate {
                     open_flags |= Fd::TRUNCATE;
                 }
+                // TODO: I strongly suspect that assigning the handle unconditionally
+                // breaks opening the same file multiple times.
                 *handle = Some(Arc::new(std::sync::RwLock::new(wasi_try_ok_ok!(
                     open_options.open(&path).map_err(fs_error_into_wasi_err)
                 ))));

tests/wasix/shared-fd/main.c

@@ -0,0 +1,102 @@
+/// This test makes sure writing to the same file, when referred to
+/// by multiple FDs (some of which are shared), works correctly. The
+/// trace logs generated by Wasmer are also inspected to make sure
+/// the file is closed properly.
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/wait.h>
+
+void error_exit(const char *message)
+{
+    perror(message);
+    exit(-1);
+}
+
+int main(int argc, char *argv[])
+{
+    int fd1 = open("./output", O_CREAT | O_EXCL | O_WRONLY);
+    if (fd1 < 0)
+    {
+        error_exit("open");
+    }
+
+    int fd2 = dup(fd1);
+    if (fd2 < 0 || fd2 == fd1)
+    {
+        error_exit("parent dup");
+    }
+
+    if (!write(fd2, "parent 2\n", 9))
+    {
+        error_exit("parent write 2");
+    }
+
+    pid_t pid = fork();
+    if (pid == -1)
+    {
+        error_exit("fork");
+    }
+    else if (pid == 0)
+    {
+        if (close(fd2))
+        {
+            error_exit("child close parent fd2");
+        }
+
+        fd2 = dup(fd1);
+        if (fd2 < 0 || fd2 == fd1)
+        {
+            error_exit("child dup");
+        }
+
+        if (!write(fd1, "child 1\n", 8))
+        {
+            error_exit("child write 1");
+        }
+        if (close(fd1))
+        {
+            error_exit("child close 1");
+        }
+
+        if (!write(fd2, "child 2\n", 8))
+        {
+            error_exit("child write 2");
+        }
+        if (close(fd2))
+        {
+            error_exit("child close 2");
+        }
+        exit(0);
+    }
+    else
+    {
+        if (close(fd2))
+        {
+            error_exit("parent close 2");
+        }
+
+        int status;
+        waitpid(pid, &status, 0);
+        if (status)
+        {
+            error_exit("child exit code");
+        }
+
+        if (!write(fd1, "parent 1\n", 9))
+        {
+            error_exit("parent write 1");
+        }
+
+        // These prints can be inspected to make sure the file was
+        // closed as a result of the last close() call, see run.sh
+        printf("closing last fd\n");
+        if (close(fd1))
+        {
+            error_exit("parent close 1");
+        }
+        printf("last fd closed\n");
+    }
+}

tests/wasix/shared-fd/run.sh

@@ -0,0 +1,13 @@
+#!/bin/bash
+
+set -eu
+
+rm -f output*
+
+RUST_LOG=virtual_fs=trace $WASMER run main.wasm --dir . &> output-wasmer-log
+
+cat output | grep "parent 1" >/dev/null && \
+  cat output | grep "parent 2" >/dev/null && \
+  cat output | grep "child 1" >/dev/null && \
+  cat output | grep "child 2" >/dev/null && \
+  awk '/closing last fd/{m1=1} /Closing host file.*shared-fd\/output/{if(m1) m2=1} /last fd closed/{if(m2) print "found all lines"}' output-wasmer-log | grep "found all lines" >/dev/null