Merge pull request #1803 from EliahKagan/run-ci/fchmod

When adding +x, get and set mode through the file descriptor

Author: Byron

Cargo.lock

@@ -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",
+] }

gix-worktree-state/Cargo.toml

@@ -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)?;
@@ -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);

gix-worktree-state/src/checkout/chunk.rs

@@ -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 => {
@@ -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.
@@ -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);
+    }
 }