change: Get mode before +x using open file descriptor, not path

This uses `fstat` rather than a method that delegates to `lstat`,
to get the current mode bits of the still-open file using the open
file descriptor (obtained from the `File` object) rather than
opening the path, which could be a different file in the case of
concurrent modification by code outside gitoxide.

This is half of the change to using the file descriptor rather than
the path for the operations that adjust the mode by adding
executable bitsi. (As before, they are added for whoever already
had read bits, and we remove setuid, setgid, and sticky bits, so
that +x does not cause a security problem or other potentially
unexpected effects.)

The other half of this change was already done, appearing in a
recent `change:` commit. It consisted of setting the mode using
`fchmod` instead of a method that delegated to `chmod`. With only
that change, the overall effect would be worse, since possible
race conditions could introduce even greater inconsistency. With
both changes, the effect is consistent. Race conditions can still
occur, but their effect is probably less severe than before either
of the changes were made.

One of the ways this may be less subject to race conditions is
that, because neither obtaining the old mode nor setting the new
one uses a path, but only the open file descriptor, an unexpected
typechange will not occur: having opened a regular file, the open
file will not turn into a directory, symlink, or anything else,
*as seen by* the open file descriptor. The file may be moved or
replaced, but the open file descriptor "follows" the move and does
not alias the separate file that now has the original path.

Because we only intend to add executable bits to regular files, it
is no longer necessary to handle this as a race condition and keep
going. Attempting to add +x to any other type of thing should now
only be possible if there is a bug in the caller. So the check for
that, and the associated Option<...> logic used to keep things
testable, is not needed.

This still does the check, but panics if the file descriptor does
not represent a regular file. The tests of this are mostly removed,
but two tests that it does panic for the kinds of non-regular files
we are most likely to encounter are added. (It may eventually make
sense check this and panic only in debug builds, or not at all.)

There are now two `fstat` calls: the one that is added to check the
`st_mode` to figure out what to pass to `fchmod`, and the
preexisting higher level invocation (that delegates to `fstat`)
done just before closing the file, to update `entry.stat`. Using
the same call for both and accounting for any effect of `fchmod`
might be more complicated, but more importantly, it does not seem
like it would be correct. If attempting to set the permissions
reports success but has an unexpected effect, perhaps due to
restrictions of the underlying filesystem or how it is mounted
(which we may not be guarnateed to have detected in a probe to
decide if +x is supported), we would want the real stat.

Because this no longer uses `Permissions` and `PermissionsExt`
facilities to check the mode before adding executable bits or to
add the executable bits, it also includes some refactoring that is
possible now that they are not being used.

Author: EliahKagan

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

@@ -285,16 +285,9 @@ pub(crate) fn finalize_entry(
     // 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) {
-            // TODO: If we keep `fchmod`, maybe `set_mode_executable` shouldn't use `std::fs::Permissions`.
-            use std::os::unix::fs::PermissionsExt;
-            #[allow(clippy::useless_conversion)] // mode_t is u32 on many but not all OSes. It's u16 on macOS.
-            let raw_mode = new_perm.mode().try_into().expect("mode fits in `st_mode`");
-            let mode = rustix::fs::Mode::from_bits(raw_mode)
-                .expect("`set_mode_executable` shouldn't preserve or add unknown bits");
-            rustix::fs::fchmod(&file, mode).map_err(std::io::Error::from)?;
-        }
+        let old_raw_mode = rustix::fs::fstat(&file).map_err(std::io::Error::from)?.st_mode;
+        let new_mode = let_readers_execute(old_raw_mode);
+        rustix::fs::fchmod(&file, new_mode).map_err(std::io::Error::from)?;
     }
     // 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.
@@ -303,102 +296,93 @@ pub(crate) fn finalize_entry(
     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 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 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);
+    }
 }