Merge pull request #6922 from uutils/revert-6595-install_copy_permission

Revert "install: create destination file with safer modes before copy"

Author: sylvestre

src/uu/install/src/install.rs

@@ -13,12 +13,9 @@ use filetime::{set_file_times, FileTime};
 use std::error::Error;
 use std::fmt::{Debug, Display};
 use std::fs;
-#[cfg(not(unix))]
 use std::fs::File;
 use std::os::unix::fs::MetadataExt;
 #[cfg(unix)]
-use std::os::unix::fs::OpenOptionsExt;
-#[cfg(unix)]
 use std::os::unix::prelude::OsStrExt;
 use std::path::{Path, PathBuf, MAIN_SEPARATOR};
 use std::process;
@@ -753,52 +750,27 @@ fn perform_backup(to: &Path, b: &Behavior) -> UResult<Option<PathBuf>> {
 fn copy_file(from: &Path, to: &Path) -> UResult<()> {
     // fs::copy fails if destination is a invalid symlink.
     // so lets just remove all existing files at destination before copy.
-    let remove_destination = || {
-        if let Err(e) = fs::remove_file(to) {
-            if e.kind() != std::io::ErrorKind::NotFound {
-                show_error!(
-                    "Failed to remove existing file {}. Error: {:?}",
-                    to.display(),
-                    e
-                );
-            }
+    if let Err(e) = fs::remove_file(to) {
+        if e.kind() != std::io::ErrorKind::NotFound {
+            show_error!(
+                "Failed to remove existing file {}. Error: {:?}",
+                to.display(),
+                e
+            );
         }
-    };
-    remove_destination();
-
-    // create the destination file first. Using safer mode on unix to avoid
-    // potential unsafe mode between time-of-creation and time-of-chmod.
-    #[cfg(unix)]
-    let creation = fs::OpenOptions::new()
-        .write(true)
-        .create_new(true)
-        .mode(0o600)
-        .open(to);
-    #[cfg(not(unix))]
-    let creation = File::create(to);
-
-    if let Err(e) = creation {
-        show_error!(
-            "Failed to create destination file {}. Error: {:?}",
-            to.display(),
-            e
-        );
-        return Err(InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), e).into());
     }
 
-    // drop the file to close the fd of creation
-    drop(creation);
-
-    /* workaround a limitation of fs::copy: skip copy if source is /dev/null
-     * https://github.com/rust-lang/rust/issues/79390
-     */
-    if from.as_os_str() != "/dev/null" {
-        if let Err(err) = fs::copy(from, to) {
-            remove_destination();
+    if from.as_os_str() == "/dev/null" {
+        /* workaround a limitation of fs::copy
+         * https://github.com/rust-lang/rust/issues/79390
+         */
+        if let Err(err) = File::create(to) {
             return Err(
                 InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), err).into(),
             );
         }
+    } else if let Err(err) = fs::copy(from, to) {
+        return Err(InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), err).into());
     }
     Ok(())
 }

tests/by-util/test_install.rs

@@ -1717,41 +1717,3 @@ fn test_install_root_combined() {
     run_and_check(&["-Cv", "c", "d"], "d", 0, 0);
     run_and_check(&["-Cv", "c", "d"], "d", 0, 0);
 }
-
-#[cfg(all(unix, feature = "chmod"))]
-#[test]
-fn test_install_copy_failures() {
-    let scene = TestScenario::new(util_name!());
-
-    let at = &scene.fixtures;
-
-    let file1 = "source_file";
-    let file2 = "target_file";
-
-    at.touch(file1);
-    scene.ccmd("chmod").arg("000").arg(file1).succeeds();
-
-    // if source file is not readable, it will raise a permission error.
-    // since we create the file with mode 0600 before `fs::copy`, if the
-    // copy failed, the file should be removed.
-    scene
-        .ucmd()
-        .arg(file1)
-        .arg(file2)
-        .arg("--mode=400")
-        .fails()
-        .stderr_contains("permission denied");
-    assert!(!at.file_exists(file2));
-
-    // if source file is good to copy, it should succeed and set the
-    // destination file permissions accordingly
-    scene.ccmd("chmod").arg("644").arg(file1).succeeds();
-    scene
-        .ucmd()
-        .arg(file1)
-        .arg(file2)
-        .arg("--mode=400")
-        .succeeds();
-    assert!(at.file_exists(file2));
-    assert_eq!(0o100_400_u32, at.metadata(file2).permissions().mode());
-}