Skip to content

Commit

Permalink
Fix and simplify output file permission copying code (#578)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
AlexTMjugador committed Nov 15, 2023
1 parent e8f4ca8 commit a6152b0
Showing 1 changed file with 3 additions and 65 deletions.
68 changes: 3 additions & 65 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

#[cfg(feature = "parallel")]
extern crate rayon;

#[cfg(not(feature = "parallel"))]
mod rayon;

Expand Down Expand Up @@ -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"))]
Expand Down

0 comments on commit a6152b0

Please sign in to comment.