Allow WASI preopen file descriptors to be closed. (#5828)

Early on in WASI, we weren't sure whether we should allow preopens to be
closed, so conservatively, we disallowed them. Among other things, this
protected assumptions in wasi-libc that it can hold onto preopen file
descriptors and rely on them always being open.

However now, I think it makes sense to relax this restriction. wasi-libc
itself doesn't expose the preopen file descriptors, so users shouldn't
ever be closing them naively, unless they have wild closes. And
toolchains other than wasi-libc may want to close preopens as a way to
drop priveleges once the main file handles are opened.

crates/test-programs/wasi-tests/src/bin/close_preopen.rs

@@ -7,17 +7,7 @@ unsafe fn test_close_preopen(dir_fd: wasi::Fd) {
     assert!(dir_fd > pre_fd, "dir_fd number");
 
     // Try to close a preopened directory handle.
-    assert_errno!(
+    wasi::fd_close(pre_fd).expect("closing a preopened file descriptor");
-        wasi::fd_close(pre_fd).expect_err("closing a preopened file descriptor"),
-        wasi::ERRNO_NOTSUP
-    );
-
-    // Try to renumber over a preopened directory handle.
-    assert_errno!(
-        wasi::fd_renumber(dir_fd, pre_fd)
-            .expect_err("renumbering over a preopened file descriptor"),
-        wasi::ERRNO_NOTSUP
-    );
 
     // Ensure that dir_fd is still open.
     let dir_fdstat = wasi::fd_fdstat_get(dir_fd).expect("failed fd_fdstat_get");
@@ -27,19 +17,10 @@ unsafe fn test_close_preopen(dir_fd: wasi::Fd) {
         "expected the scratch directory to be a directory",
     );
 
-    // Try to renumber a preopened directory handle.
+    // Ensure that pre_fd is closed.
     assert_errno!(
-        wasi::fd_renumber(pre_fd, dir_fd)
+        wasi::fd_fdstat_get(pre_fd).expect_err("failed fd_fdstat_get"),
-            .expect_err("renumbering over a preopened file descriptor"),
+        wasi::ERRNO_BADF
-        wasi::ERRNO_NOTSUP
-    );
-
-    // Ensure that dir_fd is still open.
-    let dir_fdstat = wasi::fd_fdstat_get(dir_fd).expect("failed fd_fdstat_get");
-    assert_eq!(
-        dir_fdstat.fs_filetype,
-        wasi::FILETYPE_DIRECTORY,
-        "expected the scratch directory to be a directory",
     );
 }
 

crates/test-programs/wasi-tests/src/bin/overwrite_preopen.rs

@@ -0,0 +1,50 @@
+use std::{env, process};
+use wasi_tests::{assert_errno, open_scratch_directory};
+
+unsafe fn test_overwrite_preopen(dir_fd: wasi::Fd) {
+    let pre_fd: wasi::Fd = (libc::STDERR_FILENO + 1) as wasi::Fd;
+
+    assert!(dir_fd > pre_fd, "dir_fd number");
+
+    let old_dir_filestat = wasi::fd_filestat_get(dir_fd).expect("failed fd_filestat_get");
+
+    // Try to renumber over a preopened directory handle.
+    wasi::fd_renumber(dir_fd, pre_fd)
+            .expect("renumbering over a preopened file descriptor");
+
+    // Ensure that pre_fd is still open.
+    let new_dir_filestat = wasi::fd_filestat_get(pre_fd).expect("failed fd_filestat_get");
+
+    // Ensure that we renumbered.
+    assert_eq!(old_dir_filestat.dev, new_dir_filestat.dev);
+    assert_eq!(old_dir_filestat.ino, new_dir_filestat.ino);
+
+    // Ensure that dir_fd is closed.
+    assert_errno!(
+        wasi::fd_fdstat_get(dir_fd).expect_err("failed fd_fdstat_get"),
+        wasi::ERRNO_BADF
+    );
+}
+
+fn main() {
+    let mut args = env::args();
+    let prog = args.next().unwrap();
+    let arg = if let Some(arg) = args.next() {
+        arg
+    } else {
+        eprintln!("usage: {} <scratch directory>", prog);
+        process::exit(1);
+    };
+
+    // Open scratch directory
+    let dir_fd = match open_scratch_directory(&arg) {
+        Ok(dir_fd) => dir_fd,
+        Err(err) => {
+            eprintln!("{}", err);
+            process::exit(1)
+        }
+    };
+
+    // Run the tests.
+    unsafe { test_overwrite_preopen(dir_fd) }
+}

crates/wasi-common/src/dir.rs

@@ -218,21 +218,12 @@ impl DirFdStat {
 
 pub(crate) trait TableDirExt {
     fn get_dir(&self, fd: u32) -> Result<Arc<DirEntry>, Error>;
-    fn is_preopen(&self, fd: u32) -> bool;
 }
 
 impl TableDirExt for crate::table::Table {
     fn get_dir(&self, fd: u32) -> Result<Arc<DirEntry>, Error> {
         self.get(fd)
     }
-    fn is_preopen(&self, fd: u32) -> bool {
-        if self.is::<DirEntry>(fd) {
-            let dir_entry: Arc<DirEntry> = self.get(fd).unwrap();
-            dir_entry.preopen_path.is_some()
-        } else {
-            false
-        }
-    }
 }
 
 #[derive(Debug, Clone)]

crates/wasi-common/src/snapshots/preview_1.rs

@@ -145,12 +145,6 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
         if table.is::<FileEntry>(fd) {
             let _ = table.delete::<FileEntry>(fd);
         } else if table.is::<DirEntry>(fd) {
-            // We cannot close preopened directories
-            let dir_entry: Arc<DirEntry> = table.get(fd).unwrap();
-            if dir_entry.preopen_path().is_some() {
-                return Err(Error::not_supported().context("cannot close propened directory"));
-            }
-            drop(dir_entry);
             let _ = table.delete::<DirEntry>(fd);
         } else {
             return Err(Error::badf().context("key does not refer to file or directory"));
@@ -533,9 +527,6 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx {
         if !table.contains_key(from) {
             return Err(Error::badf());
         }
-        if table.is_preopen(from) || table.is_preopen(to) {
-            return Err(Error::not_supported().context("cannot renumber a preopen"));
-        }
         table.renumber(from, to)
     }