Skip to content

Commit

Permalink
Merge pull request GitoxideLabs#1803 from EliahKagan/run-ci/fchmod
Browse files Browse the repository at this point in the history 
When adding +x, get and set mode through the file descriptor
  • Loading branch information
@Byron
Byron authored Jan 25, 2025
2 parents 847eda2 + ee7b10c commit 810b5cf
Show file tree
Hide file tree
Showing 4 changed files with 98 additions and 97 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions gix-worktree-state/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,9 @@ gix-filter = { version = "^0.17.0", path = "../gix-filter" }
io-close = "0.3.7"
thiserror = "2.0.0"
bstr = { version = "1.3.0", default-features = false }

[target.'cfg(unix)'.dependencies]
rustix = { version = "0.38.20", default-features = false, features = [
"std",
"fs",
] }
6 changes: 1 addition & 5 deletions gix-worktree-state/src/checkout/chunk.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,6 @@ where
// We process each key and do as the filter process tells us, while collecting data about the overall progress.
let keys: BTreeSet<_> = delayed_filter_results.iter().map(|d| d.key.clone()).collect();
let mut unknown_paths = Vec::new();
let mut rela_path_as_path = Default::default();
for key in keys {
loop {
let rela_paths = ctx.filters.driver_state_mut().list_delayed_paths(&key)?;
Expand Down Expand Up @@ -229,10 +228,7 @@ where
entry::finalize_entry(
delayed.entry,
write.inner.into_inner().map_err(std::io::IntoInnerError::into_error)?,
set_executable_after_creation.then(|| {
rela_path_as_path = gix_path::from_bstr(delayed.entry_path);
rela_path_as_path.as_ref()
}),
set_executable_after_creation,
)?;
delayed_files += 1;
files.fetch_add(1, Ordering::Relaxed);
Expand Down
182 changes: 90 additions & 92 deletions gix-worktree-state/src/checkout/entry.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ where
};

// For possibly existing, overwritten files, we must change the file mode explicitly.
finalize_entry(entry, file, set_executable_after_creation.then_some(dest))?;
finalize_entry(entry, file, set_executable_after_creation)?;
num_bytes
}
gix_index::entry::Mode::SYMLINK => {
Expand Down Expand Up @@ -275,20 +275,16 @@ pub(crate) fn open_file(
try_op_or_unlink(path, overwrite_existing, |p| options.open(p)).map(|f| (f, set_executable_after_creation))
}

/// Close `file` and store its stats in `entry`, possibly setting `file` executable depending on `set_executable_after_creation`.
#[cfg_attr(windows, allow(unused_variables))]
/// Close `file` and store its stats in `entry`, possibly setting `file` executable.
pub(crate) fn finalize_entry(
entry: &mut gix_index::Entry,
file: std::fs::File,
set_executable_after_creation: Option<&Path>,
#[cfg_attr(windows, allow(unused_variables))] set_executable_after_creation: bool,
) -> Result<(), crate::checkout::Error> {
// For possibly existing, overwritten files, we must change the file mode explicitly.
#[cfg(unix)]
if let Some(path) = set_executable_after_creation {
let old_perm = std::fs::symlink_metadata(path)?.permissions();
if let Some(new_perm) = set_mode_executable(old_perm) {
std::fs::set_permissions(path, new_perm)?;
}
if set_executable_after_creation {
set_executable(&file)?;
}
// NOTE: we don't call `file.sync_all()` here knowing that some filesystems don't handle this well.
// revisit this once there is a bug to fix.
Expand All @@ -297,102 +293,104 @@ pub(crate) fn finalize_entry(
Ok(())
}

/// Use `fstat` and `fchmod` on a file descriptor to make a regular file executable.
///
/// See `let_readers_execute` for the exact details of how the mode is transformed.
#[cfg(unix)]
fn set_mode_executable(mut perm: std::fs::Permissions) -> Option<std::fs::Permissions> {
use std::os::unix::fs::PermissionsExt;
let mut mode = perm.mode();
if mode & 0o170000 != 0o100000 {
return None; // Stop if we don't have a regular file anymore.
}
mode &= 0o777; // Clear non-rwx bits (setuid, setgid, sticky).
mode |= (mode & 0o444) >> 2; // Let readers also execute.
perm.set_mode(mode);
Some(perm)
fn set_executable(file: &std::fs::File) -> Result<(), std::io::Error> {
let old_raw_mode = rustix::fs::fstat(file)?.st_mode;
let new_mode = let_readers_execute(old_raw_mode);
rustix::fs::fchmod(file, new_mode)?;
Ok(())
}

/// Given the st_mode of a regular file, compute the mode with executable bits safely added.
///
/// Currently this adds executable bits for whoever has read bits already. It doesn't use the umask.
/// Set-user-ID and set-group-ID bits are unset for safety. The sticky bit is also unset.
///
/// This returns only mode bits, not file type. The return value can be passed to chmod or fchmod.
#[cfg(unix)]
fn let_readers_execute(mut raw_mode: rustix::fs::RawMode) -> rustix::fs::Mode {
assert_eq!(
raw_mode & 0o170000,
0o100000,
"bug in caller if not from a regular file"
);
raw_mode &= 0o777; // Clear type, non-rwx mode bits (setuid, setgid, sticky).
raw_mode |= (raw_mode & 0o444) >> 2; // Let readers also execute.
rustix::fs::Mode::from_bits(raw_mode).expect("all bits recognized")
}

#[cfg(all(test, unix))]
mod tests {
fn pretty(maybe_mode: Option<u32>) -> String {
match maybe_mode {
Some(mode) => format!("Some({mode:04o})"),
None => "None".into(),
}
}

#[test]
fn set_mode_executable() {
fn let_readers_execute() {
let cases = [
// Common cases:
(0o100755, Some(0o755)),
(0o100644, Some(0o755)),
(0o100750, Some(0o750)),
(0o100640, Some(0o750)),
(0o100700, Some(0o700)),
(0o100600, Some(0o700)),
(0o100775, Some(0o775)),
(0o100664, Some(0o775)),
(0o100770, Some(0o770)),
(0o100660, Some(0o770)),
(0o100764, Some(0o775)),
(0o100760, Some(0o770)),
(0o100755, 0o755),
(0o100644, 0o755),
(0o100750, 0o750),
(0o100640, 0o750),
(0o100700, 0o700),
(0o100600, 0o700),
(0o100775, 0o775),
(0o100664, 0o775),
(0o100770, 0o770),
(0o100660, 0o770),
(0o100764, 0o775),
(0o100760, 0o770),
// Less common:
(0o100674, Some(0o775)),
(0o100670, Some(0o770)),
(0o100000, Some(0o000)),
(0o100400, Some(0o500)),
(0o100440, Some(0o550)),
(0o100444, Some(0o555)),
(0o100462, Some(0o572)),
(0o100242, Some(0o252)),
(0o100167, Some(0o177)),
(0o100674, 0o775),
(0o100670, 0o770),
(0o100000, 0o000),
(0o100400, 0o500),
(0o100440, 0o550),
(0o100444, 0o555),
(0o100462, 0o572),
(0o100242, 0o252),
(0o100167, 0o177),
// With set-user-ID, set-group-ID, and sticky bits:
(0o104755, Some(0o755)),
(0o104644, Some(0o755)),
(0o102755, Some(0o755)),
(0o102644, Some(0o755)),
(0o101755, Some(0o755)),
(0o101644, Some(0o755)),
(0o106755, Some(0o755)),
(0o106644, Some(0o755)),
(0o104750, Some(0o750)),
(0o104640, Some(0o750)),
(0o102750, Some(0o750)),
(0o102640, Some(0o750)),
(0o101750, Some(0o750)),
(0o101640, Some(0o750)),
(0o106750, Some(0o750)),
(0o106640, Some(0o750)),
(0o107644, Some(0o755)),
(0o107000, Some(0o000)),
(0o106400, Some(0o500)),
(0o102462, Some(0o572)),
// Where it was replaced with a directory due to a race:
(0o040755, None),
(0o040644, None),
(0o040600, None),
(0o041755, None),
(0o041644, None),
(0o046644, None),
// Where it was replaced with a symlink due to a race:
(0o120777, None),
(0o120644, None),
// Where it was replaced with some other non-regular file due to a race:
(0o140644, None),
(0o060644, None),
(0o020644, None),
(0o010644, None),
(0o104755, 0o755),
(0o104644, 0o755),
(0o102755, 0o755),
(0o102644, 0o755),
(0o101755, 0o755),
(0o101644, 0o755),
(0o106755, 0o755),
(0o106644, 0o755),
(0o104750, 0o750),
(0o104640, 0o750),
(0o102750, 0o750),
(0o102640, 0o750),
(0o101750, 0o750),
(0o101640, 0o750),
(0o106750, 0o750),
(0o106640, 0o750),
(0o107644, 0o755),
(0o107000, 0o000),
(0o106400, 0o500),
(0o102462, 0o572),
];
for (old_mode, expected) in cases {
use std::os::unix::fs::PermissionsExt;
let old_perm = std::fs::Permissions::from_mode(old_mode);
let actual = super::set_mode_executable(old_perm).map(|perm| perm.mode());
for (st_mode, raw_expected) in cases {
let expected = rustix::fs::Mode::from_bits(raw_expected).expect("expected mode is a mode");
let actual = super::let_readers_execute(st_mode);
assert_eq!(
actual,
expected,
"{old_mode:06o} should become {}, became {}",
pretty(expected),
pretty(actual)
actual, expected,
"{st_mode:06o} should become {expected:04o}, became {actual:04o}"
);
}
}

#[test]
#[should_panic]
fn let_readers_execute_panics_on_directory() {
super::let_readers_execute(0o040644);
}

#[test]
#[should_panic]
fn let_readers_execute_should_panic_on_symlink() {
super::let_readers_execute(0o120644);
}
}

0 comments on commit 810b5cf

Please sign in to comment.