Fix review

- comment improvements
- Fix FileAttributes using else when it shouldn't

Author: CraftSpider

src/shims/unix/fd.rs

@@ -122,7 +122,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         };
 
         let result = fd.as_unix(this).flock(this.machine.communicate(), parsed_op)?;
-        drop(fd);
         // return `0` if flock is successful
         let result = result.map(|()| 0i32);
         interp_ok(Scalar::from_i32(this.try_unwrap_io_result(result)?))

src/shims/windows/fs.rs

@@ -124,10 +124,12 @@ impl FileAttributes {
         if value & file_flag_backup_semantics != 0 {
             value &= !file_flag_backup_semantics;
             out |= FileAttributes::BACKUP_SEMANTICS;
-        } else if value & file_flag_open_reparse_point != 0 {
+        }
+        if value & file_flag_open_reparse_point != 0 {
             value &= !file_flag_open_reparse_point;
             out |= FileAttributes::OPEN_REPARSE;
-        } else if value & file_attribute_normal != 0 {
+        }
+        if value & file_attribute_normal != 0 {
             value &= !file_attribute_normal;
             out |= FileAttributes::NORMAL;
         }
@@ -137,6 +139,7 @@ impl FileAttributes {
         }
 
         if out == FileAttributes::ZERO {
+            // NORMAL is equivalent to 0. Avoid needing to check both cases by unifying the two.
             out = FileAttributes::NORMAL;
         }
         interp_ok(out)
@@ -162,6 +165,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let this = self.eval_context_mut();
         this.assert_target_os("windows", "CreateFileW");
         this.check_no_isolation("`CreateFileW`")?;
+
+        // This function appears to always set the error to 0. This is important for some flag
+        // combinations, which may set error code on success.
         this.set_last_error(IoError::Raw(Scalar::from_i32(0)))?;
 
         let file_name = this.read_path_from_wide_str(this.read_pointer(file_name)?)?;
@@ -200,6 +206,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         let is_dir = file_name.is_dir();
 
+        // BACKUP_SEMANTICS is how Windows calls the act of opening a directory handle.
         if !attributes.contains(FileAttributes::BACKUP_SEMANTICS) && is_dir {
             this.set_last_error(IoError::WindowsError("ERROR_ACCESS_DENIED"))?;
             return interp_ok(Handle::Invalid);
@@ -226,20 +233,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
         match creation_disposition {
             CreateAlways | OpenAlways => {
-                // This is racy, but there doesn't appear to be an std API that both succeeds if a
-                // file exists but tells us it isn't new. Either we accept racing one way or another,
-                // or we use an iffy heuristic like file creation time. This implementation prefers
-                // to fail in the direction of erroring more often.
                 // Per the documentation:
                 // If the specified file exists and is writable, the function truncates the file,
                 // the function succeeds, and last-error code is set to ERROR_ALREADY_EXISTS.
                 // If the specified file does not exist and is a valid path, a new file is created,
                 // the function succeeds, and the last-error code is set to zero.
                 // https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew
+                //
+                // This is racy, but there doesn't appear to be an std API that both succeeds if a
+                // file exists but tells us it isn't new. Either we accept racing one way or another,
+                // or we use an iffy heuristic like file creation time. This implementation prefers
+                // to fail in the direction of erroring more often.
                 if file_name.exists() {
                     this.set_last_error(IoError::WindowsError("ERROR_ALREADY_EXISTS"))?;
-                } else {
-                    this.set_last_error(IoError::Raw(Scalar::from_u32(0)))?;
                 }
                 options.create(true);
                 if creation_disposition == CreateAlways {

src/shims/windows/handle.rs

@@ -87,7 +87,7 @@ impl Handle {
         // INVALID_HANDLE_VALUE.
         let variant_count = variant_count::<Self>();
 
-        // However, std's ilog2 is floor(log2(x))
+        // However, std's ilog2 is floor(log2(x)).
         let floor_log2 = variant_count.ilog2();
 
         // We need to add one for non powers of two to compensate for the difference.
@@ -165,7 +165,7 @@ impl Handle {
     /// Structurally invalid handles return [`HandleError::InvalidHandle`].
     /// If the handle is structurally valid but semantically invalid, e.g. a for non-existent thread
     /// ID, returns [`HandleError::ThreadNotFound`].
-    pub fn try_from_scalar<'tcx>(
+    fn try_from_scalar<'tcx>(
         handle: Scalar,
         cx: &MiriInterpCx<'tcx>,
     ) -> InterpResult<'tcx, Result<Self, HandleError>> {
@@ -197,6 +197,8 @@ impl<'tcx> EvalContextExt<'tcx> for crate::MiriInterpCx<'tcx> {}
 
 #[allow(non_snake_case)]
 pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
+    /// Convert a scalar into a structured `Handle`.
+    /// If the handle is invalid, or references a non-existent item, this returns a machine abort.
     #[track_caller]
     fn read_handle(&self, handle: &OpTy<'tcx>, function_name: &str) -> InterpResult<'tcx, Handle> {
         let this = self.eval_context_ref();

tests/pass-dep/shims/windows-fs.rs

@@ -30,6 +30,7 @@ fn main() {
 unsafe fn test_create_dir_file() {
     let temp = utils::tmp();
     let raw_path = to_wide_cstr(&temp);
+    // Open the `temp` directory.
     let handle = CreateFileW(
         raw_path.as_ptr(),
         GENERIC_READ,

tests/pass/shims/fs.rs

@@ -145,10 +145,10 @@ fn test_metadata() {
     let path = utils::prepare_with_content("miri_test_fs_metadata.txt", bytes);
 
     // Test that metadata of an absolute path is correct.
-    check_metadata(bytes, &path).expect("Absolute path metadata");
+    check_metadata(bytes, &path).expect("absolute path metadata");
     // Test that metadata of a relative path is correct.
     std::env::set_current_dir(path.parent().unwrap()).unwrap();
-    check_metadata(bytes, Path::new(path.file_name().unwrap())).expect("Relative path metadata");
+    check_metadata(bytes, Path::new(path.file_name().unwrap())).expect("relative path metadata");
 
     // Removing file should succeed.
     remove_file(&path).unwrap();