fs::copy() set file mode early

A convenience method like fs::copy() should try to prevent pitfalls a
normal user doesn't think about.

In case of an empty umask, setting the file mode early prevents
temporarily world readable or even writeable files,
because the default mode is 0o666.

In case the target is a named pipe or special device node, setting the
file mode can lead to unwanted side effects, like setting permissons on
`/dev/stdout` or for root setting permissions on `/dev/null`.

copy_file_range() returns EINVAL, if the destination is a FIFO/pipe or
a device like "/dev/null", so fallback to io::copy, too.

Use `fcopyfile` on MacOS instead of `copyfile`.

Fixes: rust-lang#26933
Fixed: rust-lang#37885

Author: haraldh

src/libstd/sys/unix/fs.rs

@@ -827,30 +827,54 @@ pub fn canonicalize(p: &Path) -> io::Result<PathBuf> {
     Ok(PathBuf::from(OsString::from_vec(buf)))
 }
 
+fn open_and_set_permissions(
+    from: &Path,
+    to: &Path,
+) -> io::Result<(crate::fs::File, crate::fs::File, u64, crate::fs::Metadata)> {
+    use crate::fs::{File, OpenOptions};
+    use crate::os::unix::fs::{OpenOptionsExt, PermissionsExt};
+
+    let reader = File::open(from)?;
+    let (perm, len) = {
+        let metadata = reader.metadata()?;
+        if !metadata.is_file() {
+            return Err(Error::new(
+                ErrorKind::InvalidInput,
+                "the source path is not an existing regular file",
+            ));
+        }
+        (metadata.permissions(), metadata.len())
+    };
+    let writer = OpenOptions::new()
+        // create the file with the correct mode right away
+        .mode(perm.mode())
+        .write(true)
+        .create(true)
+        .truncate(true)
+        .open(to)?;
+    let writer_metadata = writer.metadata()?;
+    if writer_metadata.is_file() {
+        // Set the correct file permissions, in case the file already existed.
+        // Don't set the permissions on already existing non-files like
+        // pipes/FIFOs or device nodes.
+        writer.set_permissions(perm)?;
+    }
+    Ok((reader, writer, len, writer_metadata))
+}
+
 #[cfg(not(any(target_os = "linux",
               target_os = "android",
               target_os = "macos",
               target_os = "ios")))]
 pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
-    use crate::fs::File;
-    if !from.is_file() {
-        return Err(Error::new(ErrorKind::InvalidInput,
-                              "the source path is not an existing regular file"))
-    }
+    let (mut reader, mut writer, _, _) = open_and_set_permissions(from, to)?;
 
-    let mut reader = File::open(from)?;
-    let mut writer = File::create(to)?;
-    let perm = reader.metadata()?.permissions();
-
-    let ret = io::copy(&mut reader, &mut writer)?;
-    writer.set_permissions(perm)?;
-    Ok(ret)
+    io::copy(&mut reader, &mut writer)
 }
 
 #[cfg(any(target_os = "linux", target_os = "android"))]
 pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
     use crate::cmp;
-    use crate::fs::File;
     use crate::sync::atomic::{AtomicBool, Ordering};
 
     // Kernel prior to 4.5 don't have copy_file_range
@@ -876,17 +900,7 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
         )
     }
 
-    if !from.is_file() {
-        return Err(Error::new(ErrorKind::InvalidInput,
-                              "the source path is not an existing regular file"))
-    }
-
-    let mut reader = File::open(from)?;
-    let mut writer = File::create(to)?;
-    let (perm, len) = {
-        let metadata = reader.metadata()?;
-        (metadata.permissions(), metadata.size())
-    };
+    let (mut reader, mut writer, len, _) = open_and_set_permissions(from, to)?;
 
     let has_copy_file_range = HAS_COPY_FILE_RANGE.load(Ordering::Relaxed);
     let mut written = 0u64;
@@ -896,13 +910,14 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
             let copy_result = unsafe {
                 // We actually don't have to adjust the offsets,
                 // because copy_file_range adjusts the file offset automatically
-                cvt(copy_file_range(reader.as_raw_fd(),
-                                    ptr::null_mut(),
-                                    writer.as_raw_fd(),
-                                    ptr::null_mut(),
-                                    bytes_to_copy,
-                                    0)
-                    )
+                cvt(copy_file_range(
+                    reader.as_raw_fd(),
+                    ptr::null_mut(),
+                    writer.as_raw_fd(),
+                    ptr::null_mut(),
+                    bytes_to_copy,
+                    0,
+                ))
             };
             if let Err(ref copy_err) = copy_result {
                 match copy_err.raw_os_error() {
@@ -920,24 +935,25 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
             Ok(ret) => written += ret as u64,
             Err(err) => {
                 match err.raw_os_error() {
-                    Some(os_err) if os_err == libc::ENOSYS
-                                 || os_err == libc::EXDEV
-                                 || os_err == libc::EPERM => {
-                        // Try fallback io::copy if either:
-                        // - Kernel version is < 4.5 (ENOSYS)
-                        // - Files are mounted on different fs (EXDEV)
-                        // - copy_file_range is disallowed, for example by seccomp (EPERM)
-                        assert_eq!(written, 0);
-                        let ret = io::copy(&mut reader, &mut writer)?;
-                        writer.set_permissions(perm)?;
-                        return Ok(ret)
-                    },
+                    Some(os_err)
+                    if os_err == libc::ENOSYS
+                        || os_err == libc::EXDEV
+                        || os_err == libc::EINVAL
+                        || os_err == libc::EPERM =>
+                        {
+                            // Try fallback io::copy if either:
+                            // - Kernel version is < 4.5 (ENOSYS)
+                            // - Files are mounted on different fs (EXDEV)
+                            // - copy_file_range is disallowed, for example by seccomp (EPERM)
+                            // - copy_file_range cannot be used with pipes or device nodes (EINVAL)
+                            assert_eq!(written, 0);
+                            return io::copy(&mut reader, &mut writer);
+                        }
                     _ => return Err(err),
                 }
             }
         }
     }
-    writer.set_permissions(perm)?;
     Ok(written)
 }
 
@@ -960,9 +976,9 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
     type copyfile_flags_t = u32;
 
     extern "C" {
-        fn copyfile(
-            from: *const libc::c_char,
-            to: *const libc::c_char,
+        fn fcopyfile(
+            from: libc::c_int,
+            to: libc::c_int,
             state: copyfile_state_t,
             flags: copyfile_flags_t,
         ) -> libc::c_int;
@@ -988,10 +1004,7 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
         }
     }
 
-    if !from.is_file() {
-        return Err(Error::new(ErrorKind::InvalidInput,
-                              "the source path is not an existing regular file"))
-    }
+    let (reader, writer, _, writer_metadata) = open_and_set_permissions(from, to)?;
 
     // We ensure that `FreeOnDrop` never contains a null pointer so it is
     // always safe to call `copyfile_state_free`
@@ -1003,12 +1016,18 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
         FreeOnDrop(state)
     };
 
+    let flags = if writer_metadata.is_file() {
+        COPYFILE_ALL
+    } else {
+        COPYFILE_DATA
+    };
+
     cvt(unsafe {
-        copyfile(
-            cstr(from)?.as_ptr(),
-            cstr(to)?.as_ptr(),
+        fcopyfile(
+            reader.as_raw_fd(),
+            writer.as_raw_fd(),
             state.0,
-            COPYFILE_ALL,
+            flags,
         )
     })?;
 

src/test/run-pass/paths-containing-nul.rs

@@ -1,6 +1,7 @@
 #![allow(deprecated)]
 // ignore-cloudabi no files or I/O
 // ignore-wasm32-bare no files or I/O
+// ignore-emscripten no files
 
 use std::fs;
 use std::io;
@@ -22,14 +23,18 @@ fn main() {
     assert_invalid_input("remove_file", fs::remove_file("\0"));
     assert_invalid_input("metadata", fs::metadata("\0"));
     assert_invalid_input("symlink_metadata", fs::symlink_metadata("\0"));
+
+    // If `dummy_file` does not exist, then we might get another unrelated error
+    let dummy_file = std::env::current_exe().unwrap();
+
     assert_invalid_input("rename1", fs::rename("\0", "a"));
-    assert_invalid_input("rename2", fs::rename("a", "\0"));
+    assert_invalid_input("rename2", fs::rename(&dummy_file, "\0"));
     assert_invalid_input("copy1", fs::copy("\0", "a"));
-    assert_invalid_input("copy2", fs::copy("a", "\0"));
+    assert_invalid_input("copy2", fs::copy(&dummy_file, "\0"));
     assert_invalid_input("hard_link1", fs::hard_link("\0", "a"));
-    assert_invalid_input("hard_link2", fs::hard_link("a", "\0"));
+    assert_invalid_input("hard_link2", fs::hard_link(&dummy_file, "\0"));
     assert_invalid_input("soft_link1", fs::soft_link("\0", "a"));
-    assert_invalid_input("soft_link2", fs::soft_link("a", "\0"));
+    assert_invalid_input("soft_link2", fs::soft_link(&dummy_file, "\0"));
     assert_invalid_input("read_link", fs::read_link("\0"));
     assert_invalid_input("canonicalize", fs::canonicalize("\0"));
     assert_invalid_input("create_dir", fs::create_dir("\0"));