From a6152b06c405fabbeba19c4f13ce9ef163362c54 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez?=
 <7822554+AlexTMjugador@users.noreply.github.com>
Date: Thu, 16 Nov 2023 00:15:36 +0100
Subject: [PATCH] Fix and simplify output file permission copying code (#578)

A detailed read to the [Rust documentation for the `fs::Permissions`
struct](https://doc.rust-lang.org/stable/std/fs/struct.Permissions.html)
and a little digging into its implementation in the standard library
have shown that this code didn't work as expected in any platform, and
was a bit weird to begin with:

- It first read the permissions from the input file metadata.
- Then it fetched the output file metadata.
- After that, it changed the permissions for that output file metadata.
- It then performed a sanity check that the output file had the expected
permissions.

Barring the fact that the sanity check in step 4 is not needed, the
overall approach is wrong because setting the permissions in a file
metadata struct does not actually persist those changes anywere; it's
just an in-memory change only, so these operations were useless. The
Rust documentation explicitly mentions that the `set_readonly` method
"does not modify the files attributes" [sic], but it's easy to miss that
warning and not realize that it also applies to the methods offered by
the `PermissionsExt` trait. The code only appeared to work because in
most cases the default permissions for new files happen to match the
input file permissions, so the sanity check passed.

To fix this, use the `set_permissions` method on `File` to actually set
the output file permissions to be the same as the input file
permissions, which is both much simpler and robust.

These changes were tested in the context of issue #576, and fix #576.
---
 src/lib.rs | 68 +++---------------------------------------------------
 1 file changed, 3 insertions(+), 65 deletions(-)

diff --git a/src/lib.rs b/src/lib.rs
index 2530c310..d4046583 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -21,6 +21,7 @@
 
 #[cfg(feature = "parallel")]
 extern crate rayon;
+
 #[cfg(not(feature = "parallel"))]
 mod rayon;
 
@@ -980,78 +981,15 @@ fn is_fully_optimized(original_size: usize, optimized_size: usize, opts: &Option
     original_size <= optimized_size && !opts.force
 }
 
-#[cfg(not(unix))]
 fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> {
-    let readonly_input = metadata_input.permissions().readonly();
-
     out_file
-        .metadata()
+        .set_permissions(metadata_input.permissions())
         .map_err(|err_io| {
             PngError::new(&format!(
-                "unable to read filesystem metadata of output file: {}",
+                "unable to set permissions for output file: {}",
                 err_io
             ))
         })
-        .and_then(|out_meta| {
-            out_meta.permissions().set_readonly(readonly_input);
-            out_file
-                .metadata()
-                .map_err(|err_io| {
-                    PngError::new(&format!(
-                        "unable to re-read filesystem metadata of output file: {}",
-                        err_io
-                    ))
-                })
-                .and_then(|out_meta_reread| {
-                    if out_meta_reread.permissions().readonly() != readonly_input {
-                        Err(PngError::new(&format!(
-                            "failed to set readonly, expected: {}, found: {}",
-                            readonly_input,
-                            out_meta_reread.permissions().readonly()
-                        )))
-                    } else {
-                        Ok(())
-                    }
-                })
-        })
-}
-
-#[cfg(unix)]
-fn copy_permissions(metadata_input: &Metadata, out_file: &File) -> PngResult<()> {
-    use std::os::unix::fs::PermissionsExt;
-
-    let permissions = metadata_input.permissions().mode();
-
-    out_file
-        .metadata()
-        .map_err(|err_io| {
-            PngError::new(&format!(
-                "unable to read filesystem metadata of output file: {}",
-                err_io
-            ))
-        })
-        .and_then(|out_meta| {
-            out_meta.permissions().set_mode(permissions);
-            out_file
-                .metadata()
-                .map_err(|err_io| {
-                    PngError::new(&format!(
-                        "unable to re-read filesystem metadata of output file: {}",
-                        err_io
-                    ))
-                })
-                .and_then(|out_meta_reread| {
-                    if out_meta_reread.permissions().mode() != permissions {
-                        Err(PngError::new(&format!(
-                            "failed to set permissions, expected: {:04o}, found: {:04o}",
-                            permissions,
-                            out_meta_reread.permissions().mode()
-                        )))
-                    } else {
-                        Ok(())
-                    }
-                })
-        })
 }
 
 #[cfg(not(feature = "filetime"))]