Fix rename's handling of trailing slashes on filenames. (#289)

* Fix `rename`'s handling of trailing slashes on filenames.

When the source path ends in a slash, but names a regular file,
`rename` should still fail, even though it ignores the slash when
the source names a directory.

* Only fix trailing slashes on Unix.

And fix trailing slashes by re-appending a slash.

Author: sunfishcode

cap-primitives/src/fs/via_parent/rename.rs

@@ -1,4 +1,6 @@
 use super::open_parent;
+#[cfg(unix)]
+use crate::fs::{append_dir_suffix, path_has_trailing_slash};
 use crate::fs::{rename_unchecked, strip_dir_suffix, MaybeOwnedFile};
 use std::path::Path;
 use std::{fs, io};
@@ -15,13 +17,30 @@ pub(crate) fn rename(
     let new_start = MaybeOwnedFile::borrowed(new_start);
 
     // As a special case, `rename` ignores a trailing slash rather than treating
-    // it as equivalent to a trailing slash-dot, so strip any trailing slashes.
+    // it as equivalent to a trailing slash-dot, so strip any trailing slashes
+    // for the purposes of `open_parent`.
+    //
+    // And on Unix, remember whether the source started with a slash so that we
+    // can still fail if it is and the source is a regular file.
+    #[cfg(unix)]
+    let old_starts_with_slash = path_has_trailing_slash(old_path);
     let old_path = strip_dir_suffix(old_path);
     let new_path = strip_dir_suffix(new_path);
 
     let (old_dir, old_basename) = open_parent(old_start, &*old_path)?;
     let (new_dir, new_basename) = open_parent(new_start, &*new_path)?;
 
+    // On Unix, re-append a slash if needed.
+    #[cfg(unix)]
+    let concat;
+    #[cfg(unix)]
+    let old_basename = if old_starts_with_slash {
+        concat = append_dir_suffix(old_basename.to_owned().into());
+        concat.as_os_str()
+    } else {
+        old_basename
+    };
+
     rename_unchecked(
         &old_dir,
         old_basename.as_ref(),

cap-primitives/src/rustix/fs/dir_utils.rs

@@ -1,15 +1,19 @@
 use crate::fs::OpenOptions;
 use ambient_authority::AmbientAuthority;
 use rustix::fs::OFlags;
-use std::ffi::OsStr;
+use std::ffi::{OsStr, OsString};
 use std::ops::Deref;
 #[cfg(unix)]
-use std::os::unix::{ffi::OsStrExt, fs::OpenOptionsExt};
+use std::os::unix::{
+    ffi::{OsStrExt, OsStringExt},
+    fs::OpenOptionsExt,
+};
 #[cfg(target_os = "wasi")]
-use std::os::wasi::{ffi::OsStrExt, fs::OpenOptionsExt};
-use std::path::Path;
-#[cfg(racy_asserts)]
-use std::{ffi::OsString, os::unix::ffi::OsStringExt, path::PathBuf};
+use std::os::wasi::{
+    ffi::{OsStrExt, OsStringExt},
+    fs::OpenOptionsExt,
+};
+use std::path::{Path, PathBuf};
 use std::{fs, io};
 
 /// Rust's `Path` implicitly strips redundant slashes, however they aren't
@@ -53,7 +57,6 @@ pub(crate) fn path_has_trailing_slash(path: &Path) -> bool {
 
 /// Append a trailing `/`. This can be used to require that the given `path`
 /// names a directory.
-#[cfg(racy_asserts)]
 pub(crate) fn append_dir_suffix(path: PathBuf) -> PathBuf {
     let mut bytes = path.into_os_string().into_vec();
     bytes.push(b'/');
@@ -158,6 +161,10 @@ fn strip_dir_suffix_tests() {
     assert_eq!(&*strip_dir_suffix(Path::new("foo")), Path::new("foo"));
     assert_eq!(&*strip_dir_suffix(Path::new("/")), Path::new("/"));
     assert_eq!(&*strip_dir_suffix(Path::new("//")), Path::new("/"));
+    assert_eq!(&*strip_dir_suffix(Path::new("/.")), Path::new("/."));
+    assert_eq!(&*strip_dir_suffix(Path::new("//.")), Path::new("/."));
+    assert_eq!(&*strip_dir_suffix(Path::new(".")), Path::new("."));
+    assert_eq!(&*strip_dir_suffix(Path::new("foo/.")), Path::new("foo/."));
 }
 
 #[test]

tests/fs_additional.rs

@@ -178,12 +178,47 @@ fn rename_slashdots() {
     check!(tmpdir.rename("dir", &tmpdir, "dir"));
     check!(tmpdir.rename("dir", &tmpdir, "dir/"));
     check!(tmpdir.rename("dir/", &tmpdir, "dir"));
+    check!(tmpdir.rename("dir/", &tmpdir, "dir/"));
 
     // TODO: Platform-specific error code.
     error_contains!(tmpdir.rename("dir", &tmpdir, "dir/."), "");
     error_contains!(tmpdir.rename("dir/.", &tmpdir, "dir"), "");
 }
 
+#[test]
+#[cfg_attr(windows, ignore)] // TODO investigate why this one is failing
+fn rename_slashdots_ambient() {
+    let dir = tempfile::tempdir().unwrap();
+
+    check!(std::fs::create_dir_all(dir.path().join("dir")));
+    check!(std::fs::rename(
+        dir.path().join("dir"),
+        dir.path().join("dir")
+    ));
+    check!(std::fs::rename(
+        dir.path().join("dir"),
+        dir.path().join("dir/")
+    ));
+    check!(std::fs::rename(
+        dir.path().join("dir/"),
+        dir.path().join("dir")
+    ));
+    check!(std::fs::rename(
+        dir.path().join("dir/"),
+        dir.path().join("dir/")
+    ));
+
+    // TODO: Platform-specific error code.
+    error_contains!(
+        std::fs::rename(dir.path().join("dir"), dir.path().join("dir/.")),
+        ""
+    );
+    error_contains!(
+        std::fs::rename(dir.path().join("dir/."), dir.path().join("dir")),
+        ""
+    );
+}
+
 #[test]
 fn optionally_recursive_mkdir() {
     let tmpdir = tmpdir();

tests/rename.rs

@@ -137,6 +137,10 @@ fn rename_basics() {
         tmpdir.rename("/..", &tmpdir, "nope.txt"),
         "a path led outside of the filesystem"
     );
+    error_contains!(
+        tmpdir.rename("file.txt/", &tmpdir, "nope.txt"),
+        "Not a directory"
+    );
 
     /* // TODO: Platform-specific error code.
     error!(