Unrolled build for rust-lang#129800

Rollup merge of rust-lang#129800 - ChrisDenton:remove-dir-all2, r=Amanieu

Move the Windows remove_dir_all impl into a module and make it more race resistant

This attempts to make the Windows implementation of `remove_dir_all` easier to understand and work with by separating out different concerns into their own functions. The code is mostly the same as before just moved around. There are some changes to make it more robust against races (e.g. two calls to `remove_dir_all` running concurrently). The module level comment explains the issue.

try-job: x86_64-msvc
try-job: i686-msvc

Author: rust-timer

library/std/src/sys/pal/windows/api.rs

@@ -254,7 +254,7 @@ pub struct WinError {
     pub code: u32,
 }
 impl WinError {
-    const fn new(code: u32) -> Self {
+    pub const fn new(code: u32) -> Self {
         Self { code }
     }
 }
@@ -272,8 +272,11 @@ impl WinError {
     // tidy-alphabetical-start
     pub const ACCESS_DENIED: Self = Self::new(c::ERROR_ACCESS_DENIED);
     pub const ALREADY_EXISTS: Self = Self::new(c::ERROR_ALREADY_EXISTS);
+    pub const BAD_NET_NAME: Self = Self::new(c::ERROR_BAD_NET_NAME);
+    pub const BAD_NETPATH: Self = Self::new(c::ERROR_BAD_NETPATH);
     pub const CANT_ACCESS_FILE: Self = Self::new(c::ERROR_CANT_ACCESS_FILE);
     pub const DELETE_PENDING: Self = Self::new(c::ERROR_DELETE_PENDING);
+    pub const DIR_NOT_EMPTY: Self = Self::new(c::ERROR_DIR_NOT_EMPTY);
     pub const DIRECTORY: Self = Self::new(c::ERROR_DIRECTORY);
     pub const FILE_NOT_FOUND: Self = Self::new(c::ERROR_FILE_NOT_FOUND);
     pub const INSUFFICIENT_BUFFER: Self = Self::new(c::ERROR_INSUFFICIENT_BUFFER);

library/std/src/sys/pal/windows/c/bindings.txt

@@ -34,6 +34,7 @@ Windows.Wdk.Storage.FileSystem.FILE_WRITE_THROUGH
 Windows.Wdk.Storage.FileSystem.NtCreateFile
 Windows.Wdk.Storage.FileSystem.NTCREATEFILE_CREATE_DISPOSITION
 Windows.Wdk.Storage.FileSystem.NTCREATEFILE_CREATE_OPTIONS
+Windows.Wdk.Storage.FileSystem.NtOpenFile
 Windows.Wdk.Storage.FileSystem.NtReadFile
 Windows.Wdk.Storage.FileSystem.NtWriteFile
 Windows.Wdk.Storage.FileSystem.SYMLINK_FLAG_RELATIVE
@@ -1931,10 +1932,14 @@ Windows.Win32.Foundation.RtlNtStatusToDosError
 Windows.Win32.Foundation.SetHandleInformation
 Windows.Win32.Foundation.SetLastError
 Windows.Win32.Foundation.STATUS_DELETE_PENDING
+Windows.Win32.Foundation.STATUS_DIRECTORY_NOT_EMPTY
 Windows.Win32.Foundation.STATUS_END_OF_FILE
+Windows.Win32.Foundation.STATUS_FILE_DELETED
+Windows.Win32.Foundation.STATUS_INVALID_HANDLE
 Windows.Win32.Foundation.STATUS_INVALID_PARAMETER
 Windows.Win32.Foundation.STATUS_NOT_IMPLEMENTED
 Windows.Win32.Foundation.STATUS_PENDING
+Windows.Win32.Foundation.STATUS_SHARING_VIOLATION
 Windows.Win32.Foundation.STATUS_SUCCESS
 Windows.Win32.Foundation.TRUE
 Windows.Win32.Foundation.UNICODE_STRING

library/std/src/sys/pal/windows/c/windows_sys.rs

@@ -105,6 +105,7 @@ windows_targets::link!("kernel32.dll" "system" fn WideCharToMultiByte(codepage :
 windows_targets::link!("kernel32.dll" "system" fn WriteConsoleW(hconsoleoutput : HANDLE, lpbuffer : PCWSTR, nnumberofcharstowrite : u32, lpnumberofcharswritten : *mut u32, lpreserved : *const core::ffi::c_void) -> BOOL);
 windows_targets::link!("kernel32.dll" "system" fn WriteFileEx(hfile : HANDLE, lpbuffer : *const u8, nnumberofbytestowrite : u32, lpoverlapped : *mut OVERLAPPED, lpcompletionroutine : LPOVERLAPPED_COMPLETION_ROUTINE) -> BOOL);
 windows_targets::link!("ntdll.dll" "system" fn NtCreateFile(filehandle : *mut HANDLE, desiredaccess : FILE_ACCESS_RIGHTS, objectattributes : *const OBJECT_ATTRIBUTES, iostatusblock : *mut IO_STATUS_BLOCK, allocationsize : *const i64, fileattributes : FILE_FLAGS_AND_ATTRIBUTES, shareaccess : FILE_SHARE_MODE, createdisposition : NTCREATEFILE_CREATE_DISPOSITION, createoptions : NTCREATEFILE_CREATE_OPTIONS, eabuffer : *const core::ffi::c_void, ealength : u32) -> NTSTATUS);
+windows_targets::link!("ntdll.dll" "system" fn NtOpenFile(filehandle : *mut HANDLE, desiredaccess : u32, objectattributes : *const OBJECT_ATTRIBUTES, iostatusblock : *mut IO_STATUS_BLOCK, shareaccess : u32, openoptions : u32) -> NTSTATUS);
 windows_targets::link!("ntdll.dll" "system" fn NtReadFile(filehandle : HANDLE, event : HANDLE, apcroutine : PIO_APC_ROUTINE, apccontext : *const core::ffi::c_void, iostatusblock : *mut IO_STATUS_BLOCK, buffer : *mut core::ffi::c_void, length : u32, byteoffset : *const i64, key : *const u32) -> NTSTATUS);
 windows_targets::link!("ntdll.dll" "system" fn NtWriteFile(filehandle : HANDLE, event : HANDLE, apcroutine : PIO_APC_ROUTINE, apccontext : *const core::ffi::c_void, iostatusblock : *mut IO_STATUS_BLOCK, buffer : *const core::ffi::c_void, length : u32, byteoffset : *const i64, key : *const u32) -> NTSTATUS);
 windows_targets::link!("ntdll.dll" "system" fn RtlNtStatusToDosError(status : NTSTATUS) -> u32);
@@ -2982,10 +2983,14 @@ pub struct STARTUPINFOW {
 }
 pub type STARTUPINFOW_FLAGS = u32;
 pub const STATUS_DELETE_PENDING: NTSTATUS = 0xC0000056_u32 as _;
+pub const STATUS_DIRECTORY_NOT_EMPTY: NTSTATUS = 0xC0000101_u32 as _;
 pub const STATUS_END_OF_FILE: NTSTATUS = 0xC0000011_u32 as _;
+pub const STATUS_FILE_DELETED: NTSTATUS = 0xC0000123_u32 as _;
+pub const STATUS_INVALID_HANDLE: NTSTATUS = 0xC0000008_u32 as _;
 pub const STATUS_INVALID_PARAMETER: NTSTATUS = 0xC000000D_u32 as _;
 pub const STATUS_NOT_IMPLEMENTED: NTSTATUS = 0xC0000002_u32 as _;
 pub const STATUS_PENDING: NTSTATUS = 0x103_u32 as _;
+pub const STATUS_SHARING_VIOLATION: NTSTATUS = 0xC0000043_u32 as _;
 pub const STATUS_SUCCESS: NTSTATUS = 0x0_u32 as _;
 pub const STD_ERROR_HANDLE: STD_HANDLE = 4294967284u32;
 pub type STD_HANDLE = u32;

library/std/src/sys/pal/windows/fs.rs

@@ -14,8 +14,11 @@ use crate::sys::handle::Handle;
 use crate::sys::path::maybe_verbatim;
 use crate::sys::time::SystemTime;
 use crate::sys::{c, cvt, Align8};
-use crate::sys_common::{ignore_notfound, AsInner, FromInner, IntoInner};
-use crate::{fmt, ptr, slice, thread};
+use crate::sys_common::{AsInner, FromInner, IntoInner};
+use crate::{fmt, ptr, slice};
+
+mod remove_dir_all;
+use remove_dir_all::remove_dir_all_iterative;
 
 pub struct File {
     handle: Handle,
@@ -646,6 +649,22 @@ impl File {
             Ok(info)
         }
     }
+
+    /// Deletes the file, consuming the file handle to ensure the delete occurs
+    /// as immediately as possible.
+    /// This attempts to use `posix_delete` but falls back to `win32_delete`
+    /// if that is not supported by the filesystem.
+    #[allow(unused)]
+    fn delete(self) -> Result<(), WinError> {
+        // If POSIX delete is not supported for this filesystem then fallback to win32 delete.
+        match self.posix_delete() {
+            Err(WinError::INVALID_PARAMETER)
+            | Err(WinError::NOT_SUPPORTED)
+            | Err(WinError::INVALID_FUNCTION) => self.win32_delete(),
+            result => result,
+        }
+    }
+
     /// Delete using POSIX semantics.
     ///
     /// Files will be deleted as soon as the handle is closed. This is supported
@@ -654,21 +673,23 @@ impl File {
     ///
     /// If the operation is not supported for this filesystem or OS version
     /// then errors will be `ERROR_NOT_SUPPORTED` or `ERROR_INVALID_PARAMETER`.
-    fn posix_delete(&self) -> io::Result<()> {
+    #[allow(unused)]
+    fn posix_delete(&self) -> Result<(), WinError> {
         let info = c::FILE_DISPOSITION_INFO_EX {
             Flags: c::FILE_DISPOSITION_FLAG_DELETE
                 | c::FILE_DISPOSITION_FLAG_POSIX_SEMANTICS
                 | c::FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE,
         };
-        api::set_file_information_by_handle(self.handle.as_raw_handle(), &info).io_result()
+        api::set_file_information_by_handle(self.handle.as_raw_handle(), &info)
     }
 
     /// Delete a file using win32 semantics. The file won't actually be deleted
     /// until all file handles are closed. However, marking a file for deletion
     /// will prevent anyone from opening a new handle to the file.
-    fn win32_delete(&self) -> io::Result<()> {
+    #[allow(unused)]
+    fn win32_delete(&self) -> Result<(), WinError> {
         let info = c::FILE_DISPOSITION_INFO { DeleteFile: c::TRUE as _ };
-        api::set_file_information_by_handle(self.handle.as_raw_handle(), &info).io_result()
+        api::set_file_information_by_handle(self.handle.as_raw_handle(), &info)
     }
 
     /// Fill the given buffer with as many directory entries as will fit.
@@ -684,21 +705,23 @@ impl File {
     /// A symlink directory is simply an empty directory with some "reparse" metadata attached.
     /// So if you open a link (not its target) and iterate the directory,
     /// you will always iterate an empty directory regardless of the target.
-    fn fill_dir_buff(&self, buffer: &mut DirBuff, restart: bool) -> io::Result<bool> {
+    #[allow(unused)]
+    fn fill_dir_buff(&self, buffer: &mut DirBuff, restart: bool) -> Result<bool, WinError> {
         let class =
             if restart { c::FileIdBothDirectoryRestartInfo } else { c::FileIdBothDirectoryInfo };
 
         unsafe {
-            let result = cvt(c::GetFileInformationByHandleEx(
-                self.handle.as_raw_handle(),
+            let result = c::GetFileInformationByHandleEx(
+                self.as_raw_handle(),
                 class,
                 buffer.as_mut_ptr().cast(),
                 buffer.capacity() as _,
-            ));
-            match result {
-                Ok(_) => Ok(true),
-                Err(e) if e.raw_os_error() == Some(c::ERROR_NO_MORE_FILES as _) => Ok(false),
-                Err(e) => Err(e),
+            );
+            if result == 0 {
+                let err = api::get_last_error();
+                if err.code == c::ERROR_NO_MORE_FILES { Ok(false) } else { Err(err) }
+            } else {
+                Ok(true)
             }
         }
     }
@@ -804,62 +827,6 @@ unsafe fn from_maybe_unaligned<'a>(p: *const u16, len: usize) -> Cow<'a, [u16]>
     }
 }
 
-/// Open a link relative to the parent directory, ensure no symlinks are followed.
-fn open_link_no_reparse(parent: &File, name: &[u16], access: u32) -> io::Result<File> {
-    // This is implemented using the lower level `NtCreateFile` function as
-    // unfortunately opening a file relative to a parent is not supported by
-    // win32 functions. It is however a fundamental feature of the NT kernel.
-    //
-    // See https://docs.microsoft.com/en-us/windows/win32/api/winternl/nf-winternl-ntcreatefile
-    unsafe {
-        let mut handle = ptr::null_mut();
-        let mut io_status = c::IO_STATUS_BLOCK::PENDING;
-        let mut name_str = c::UNICODE_STRING::from_ref(name);
-        use crate::sync::atomic::{AtomicU32, Ordering};
-        // The `OBJ_DONT_REPARSE` attribute ensures that we haven't been
-        // tricked into following a symlink. However, it may not be available in
-        // earlier versions of Windows.
-        static ATTRIBUTES: AtomicU32 = AtomicU32::new(c::OBJ_DONT_REPARSE);
-        let object = c::OBJECT_ATTRIBUTES {
-            ObjectName: &mut name_str,
-            RootDirectory: parent.as_raw_handle(),
-            Attributes: ATTRIBUTES.load(Ordering::Relaxed),
-            ..c::OBJECT_ATTRIBUTES::default()
-        };
-        let status = c::NtCreateFile(
-            &mut handle,
-            access,
-            &object,
-            &mut io_status,
-            crate::ptr::null_mut(),
-            0,
-            c::FILE_SHARE_DELETE | c::FILE_SHARE_READ | c::FILE_SHARE_WRITE,
-            c::FILE_OPEN,
-            // If `name` is a symlink then open the link rather than the target.
-            c::FILE_OPEN_REPARSE_POINT,
-            crate::ptr::null_mut(),
-            0,
-        );
-        // Convert an NTSTATUS to the more familiar Win32 error codes (aka "DosError")
-        if c::nt_success(status) {
-            Ok(File::from_raw_handle(handle))
-        } else if status == c::STATUS_DELETE_PENDING {
-            // We make a special exception for `STATUS_DELETE_PENDING` because
-            // otherwise this will be mapped to `ERROR_ACCESS_DENIED` which is
-            // very unhelpful.
-            Err(io::Error::from_raw_os_error(c::ERROR_DELETE_PENDING as i32))
-        } else if status == c::STATUS_INVALID_PARAMETER
-            && ATTRIBUTES.load(Ordering::Relaxed) == c::OBJ_DONT_REPARSE
-        {
-            // Try without `OBJ_DONT_REPARSE`. See above.
-            ATTRIBUTES.store(0, Ordering::Relaxed);
-            open_link_no_reparse(parent, name, access)
-        } else {
-            Err(io::Error::from_raw_os_error(c::RtlNtStatusToDosError(status) as _))
-        }
-    }
-}
-
 impl AsInner<Handle> for File {
     #[inline]
     fn as_inner(&self) -> &Handle {
@@ -1142,114 +1109,22 @@ pub fn rmdir(p: &Path) -> io::Result<()> {
     Ok(())
 }
 
-/// Open a file or directory without following symlinks.
-fn open_link(path: &Path, access_mode: u32) -> io::Result<File> {
+pub fn remove_dir_all(path: &Path) -> io::Result<()> {
+    // Open a file or directory without following symlinks.
     let mut opts = OpenOptions::new();
-    opts.access_mode(access_mode);
+    opts.access_mode(c::FILE_LIST_DIRECTORY);
     // `FILE_FLAG_BACKUP_SEMANTICS` allows opening directories.
     // `FILE_FLAG_OPEN_REPARSE_POINT` opens a link instead of its target.
     opts.custom_flags(c::FILE_FLAG_BACKUP_SEMANTICS | c::FILE_FLAG_OPEN_REPARSE_POINT);
-    File::open(path, &opts)
-}
-
-pub fn remove_dir_all(path: &Path) -> io::Result<()> {
-    let file = open_link(path, c::DELETE | c::FILE_LIST_DIRECTORY)?;
+    let file = File::open(path, &opts)?;
 
     // Test if the file is not a directory or a symlink to a directory.
     if (file.basic_info()?.FileAttributes & c::FILE_ATTRIBUTE_DIRECTORY) == 0 {
         return Err(io::Error::from_raw_os_error(c::ERROR_DIRECTORY as _));
     }
 
-    match ignore_notfound(remove_dir_all_iterative(&file, File::posix_delete)) {
-        Err(e) => {
-            if let Some(code) = e.raw_os_error() {
-                match code as u32 {
-                    // If POSIX delete is not supported for this filesystem then fallback to win32 delete.
-                    c::ERROR_NOT_SUPPORTED
-                    | c::ERROR_INVALID_FUNCTION
-                    | c::ERROR_INVALID_PARAMETER => {
-                        remove_dir_all_iterative(&file, File::win32_delete)
-                    }
-                    _ => Err(e),
-                }
-            } else {
-                Err(e)
-            }
-        }
-        ok => ok,
-    }
-}
-
-fn remove_dir_all_iterative(f: &File, delete: fn(&File) -> io::Result<()>) -> io::Result<()> {
-    // When deleting files we may loop this many times when certain error conditions occur.
-    // This allows remove_dir_all to succeed when the error is temporary.
-    const MAX_RETRIES: u32 = 10;
-
-    let mut buffer = DirBuff::new();
-    let mut dirlist = vec![f.duplicate()?];
-
-    // FIXME: This is a hack so we can push to the dirlist vec after borrowing from it.
-    fn copy_handle(f: &File) -> mem::ManuallyDrop<File> {
-        unsafe { mem::ManuallyDrop::new(File::from_raw_handle(f.as_raw_handle())) }
-    }
-
-    let mut restart = true;
-    while let Some(dir) = dirlist.last() {
-        let dir = copy_handle(dir);
-
-        // Fill the buffer and iterate the entries.
-        let more_data = dir.fill_dir_buff(&mut buffer, restart)?;
-        restart = false;
-        for (name, is_directory) in buffer.iter() {
-            if is_directory {
-                let child_dir = open_link_no_reparse(
-                    &dir,
-                    &name,
-                    c::SYNCHRONIZE | c::DELETE | c::FILE_LIST_DIRECTORY,
-                );
-                // On success, add the handle to the queue.
-                // If opening the directory fails we treat it the same as a file
-                if let Ok(child_dir) = child_dir {
-                    dirlist.push(child_dir);
-                    continue;
-                }
-            }
-            for i in 1..=MAX_RETRIES {
-                let result = open_link_no_reparse(&dir, &name, c::SYNCHRONIZE | c::DELETE);
-                match result {
-                    Ok(f) => delete(&f)?,
-                    // Already deleted, so skip.
-                    Err(e) if e.kind() == io::ErrorKind::NotFound => break,
-                    // Retry a few times if the file is locked or a delete is already in progress.
-                    Err(e)
-                        if i < MAX_RETRIES
-                            && (e.raw_os_error() == Some(c::ERROR_DELETE_PENDING as _)
-                                || e.raw_os_error() == Some(c::ERROR_SHARING_VIOLATION as _)) => {}
-                    // Otherwise return the error.
-                    Err(e) => return Err(e),
-                }
-                thread::yield_now();
-            }
-        }
-        // If there were no more files then delete the directory.
-        if !more_data {
-            if let Some(dir) = dirlist.pop() {
-                // Retry deleting a few times in case we need to wait for a file to be deleted.
-                for i in 1..=MAX_RETRIES {
-                    let result = delete(&dir);
-                    if let Err(e) = result {
-                        if i == MAX_RETRIES || e.kind() != io::ErrorKind::DirectoryNotEmpty {
-                            return Err(e);
-                        }
-                        thread::yield_now();
-                    } else {
-                        break;
-                    }
-                }
-            }
-        }
-    }
-    Ok(())
+    // Remove the directory and all its contents.
+    remove_dir_all_iterative(file).io_result()
 }
 
 pub fn readlink(path: &Path) -> io::Result<PathBuf> {