diff --git a/library/std/src/fs.rs b/library/std/src/fs.rs index bf61ab79964c4..7671ebe5816a2 100644 --- a/library/std/src/fs.rs +++ b/library/std/src/fs.rs @@ -2802,7 +2802,6 @@ impl ReadDir { /// handle used by `ReadDir`. However, this may fall back to opening a new handle based on the /// path, which is subject to race conditions if the file system has changed since the `ReadDir` /// was created. - // FIXME: before stabilization, ensure that we have a race-free implementation on Windows! #[unstable(feature = "dirfd", issue = "120426")] pub fn dir(&self) -> io::Result { self.0.dir().map(|d| Dir { inner: d }) diff --git a/library/std/src/fs/tests.rs b/library/std/src/fs/tests.rs index d611bac98f2e3..55d5d78b103d6 100644 --- a/library/std/src/fs/tests.rs +++ b/library/std/src/fs/tests.rs @@ -52,11 +52,13 @@ pub(crate) fn got_symlink_permission(tmpdir: &TempDir) -> bool { } let link = tmpdir.join("some_hopefully_unique_link_name"); - match symlink_file(r"nonexisting_target", link) { + let res = match symlink_file(r"nonexisting_target", &link) { // ERROR_PRIVILEGE_NOT_HELD = 1314 Err(ref err) if err.raw_os_error() == Some(1314) => false, Ok(_) | Err(_) => true, - } + }; + fs::remove_file(link).ok(); // do not leave behind junk + res } #[test] @@ -2186,12 +2188,23 @@ fn realpath_works_tricky() { #[test] fn dir_entry_methods() { + #[cfg(windows)] + use crate::os::windows::fs::FileTypeExt; + let tmpdir = tmpdir(); + let mut num_files = 0; fs::create_dir_all(&tmpdir.join("a")).unwrap(); File::create(&tmpdir.join("b")).unwrap(); + num_files += 2; + if got_symlink_permission(&tmpdir) { + symlink_dir("a", &tmpdir.join("c")).unwrap(); + symlink_file("b", &tmpdir.join("d")).unwrap(); + num_files += 2; + } for file in tmpdir.path().read_dir().unwrap().map(|f| f.unwrap()) { + num_files -= 1; let fname = file.file_name(); match fname.to_str() { Some("a") => { @@ -2202,9 +2215,28 @@ fn dir_entry_methods() { assert!(file.file_type().unwrap().is_file()); assert!(file.metadata().unwrap().is_file()); } + Some("c") => { + assert!(file.file_type().unwrap().is_symlink()); + assert!(file.metadata().unwrap().is_symlink()); + #[cfg(windows)] + { + assert!(file.file_type().unwrap().is_symlink_dir()); + assert!(file.metadata().unwrap().file_type().is_symlink_dir()); + } + } + Some("d") => { + assert!(file.file_type().unwrap().is_symlink()); + assert!(file.metadata().unwrap().is_symlink()); + #[cfg(windows)] + { + assert!(file.file_type().unwrap().is_symlink_file()); + assert!(file.metadata().unwrap().file_type().is_symlink_file()); + } + } f => panic!("unknown file name: {f:?}"), } } + assert_eq!(num_files, 0); } #[test] @@ -2224,6 +2256,15 @@ fn read_dir_not_found() { assert_eq!(res.err().unwrap().kind(), ErrorKind::NotFound); } +#[test] +fn read_dir_not_directory() { + let tmpdir = tmpdir(); + let filename = tmpdir.join("file"); + File::create(&filename).unwrap(); + let res = fs::read_dir(&filename); + assert_eq!(res.err().unwrap().kind(), ErrorKind::NotADirectory); +} + #[test] fn file_open_not_found() { let res = File::open("/path/that/does/not/exist"); diff --git a/library/std/src/sys/fs/windows.rs b/library/std/src/sys/fs/windows.rs index 0585ea5348c78..180b8939a4a09 100644 --- a/library/std/src/sys/fs/windows.rs +++ b/library/std/src/sys/fs/windows.rs @@ -14,7 +14,7 @@ use crate::sync::Arc; pub use crate::sys::fs::common::{ExtraHomeDirs, ExtraMediaDirs}; use crate::sys::handle::Handle; use crate::sys::pal::api::{self, WinError, set_file_information_by_handle}; -use crate::sys::pal::{IoResult, fill_utf16_buf, to_u16s, truncate_utf16_at_nul}; +use crate::sys::pal::{IoResult, fill_utf16_buf, to_u16s}; use crate::sys::path::{WCStr, maybe_verbatim}; use crate::sys::time::SystemTime; use crate::sys::{Align8, AsInner, FromInner, IntoInner, c, cvt}; @@ -53,19 +53,20 @@ pub struct FileType { } pub struct ReadDir { - handle: Option, + handle: Handle, root: Arc, - first: Option, + /// If `None`, there are no more entries. + buf: Option, + /// Index of the next entry in `buf`. If `None`, all entries in `buf` have been yielded + /// already and we need to request the next set of entries. + next_entry: Option, } -struct FindNextFileHandle(c::HANDLE); - -unsafe impl Send for FindNextFileHandle {} -unsafe impl Sync for FindNextFileHandle {} - pub struct DirEntry { root: Arc, - data: c::WIN32_FIND_DATAW, + data: c::FILE_ID_BOTH_DIR_INFO, + // We need to store an owned copy of the filename as it has a variable size. + name: Box<[u16]>, } unsafe impl Send for OpenOptions {} @@ -115,10 +116,16 @@ pub struct DirBuilder; impl ReadDir { pub fn dir(&self) -> io::Result { + let handle = self.handle.as_raw_handle(); + // SAFETY: the handle is valid and we are not dropping it. + let dir_handle = mem::ManuallyDrop::new(dir::Dir::from_inner(unsafe { + Handle::from_raw_handle(handle) + })); + // We don't want to expose the handle or any duplicate of it as the directory stream has + // state attached to it. So we open a completely new handle based on this one. let mut options = OpenOptions::new(); options.read(true); - // FIXME: use race-free handle-based approach instead. - Dir::open(&self.root, &options) + dir_handle.open_dir(Path::new(""), &options) } } @@ -130,73 +137,17 @@ impl fmt::Debug for ReadDir { } } -impl Iterator for ReadDir { - type Item = io::Result; - fn next(&mut self) -> Option> { - let Some(handle) = self.handle.as_ref() else { - // This iterator was initialized with an `INVALID_HANDLE_VALUE` as its handle. - // Simply return `None` because this is only the case when `FindFirstFileExW` in - // the construction of this iterator returns `ERROR_FILE_NOT_FOUND` which means - // no matchhing files can be found. - return None; - }; - if let Some(first) = self.first.take() { - if let Some(e) = DirEntry::new(&self.root, &first) { - return Some(Ok(e)); - } - } - unsafe { - let mut wfd = mem::zeroed(); - loop { - if c::FindNextFileW(handle.0, &mut wfd) == 0 { - self.handle = None; - match api::get_last_error() { - WinError::NO_MORE_FILES => return None, - WinError { code } => { - return Some(Err(Error::from_raw_os_error(code as i32))); - } - } - } - if let Some(e) = DirEntry::new(&self.root, &wfd) { - return Some(Ok(e)); - } - } - } - } -} - -impl Drop for FindNextFileHandle { - fn drop(&mut self) { - let r = unsafe { c::FindClose(self.0) }; - debug_assert!(r != 0); - } -} - impl DirEntry { - fn new(root: &Arc, wfd: &c::WIN32_FIND_DATAW) -> Option { - match &wfd.cFileName[0..3] { - // check for '.' and '..' - &[46, 0, ..] | &[46, 46, 0, ..] => return None, - _ => {} - } - - Some(DirEntry { root: root.clone(), data: *wfd }) - } - pub fn path(&self) -> PathBuf { self.root.join(self.file_name()) } pub fn file_name(&self) -> OsString { - let filename = truncate_utf16_at_nul(&self.data.cFileName); - OsString::from_wide(filename) + OsString::from_wide(&self.name) } pub fn file_type(&self) -> io::Result { - Ok(FileType::new( - self.data.dwFileAttributes, - /* reparse_tag = */ self.data.dwReserved0, - )) + Ok(FileType::new(self.data.FileAttributes, /* reparse_tag = */ self.data.EaSize)) } pub fn metadata(&self) -> io::Result { @@ -865,13 +816,40 @@ impl File { let info = c::FILE_DISPOSITION_INFO { DeleteFile: true }; api::set_file_information_by_handle(self.handle.as_raw_handle(), &info) } +} + +/// A buffer for holding directory entries. +struct DirBuff { + buffer: Box; Self::BUFFER_SIZE]>>, +} +impl DirBuff { + const BUFFER_SIZE: usize = 1024; + fn new() -> Self { + Self { + // Safety: `Align8<[MaybeUninit; N]>` does not need + // initialization. + buffer: unsafe { Box::new_uninit().assume_init() }, + } + } + fn capacity(&self) -> usize { + self.buffer.0.len() + } + fn as_mut_ptr(&mut self) -> *mut u8 { + self.buffer.0.as_mut_ptr().cast() + } + /// Returns a `DirBuffIter`. + fn iter(&self) -> DirBuffIter<'_> { + DirBuffIter::new(self) + } /// Fill the given buffer with as many directory entries as will fit. /// This will remember its position and continue from the last call unless /// `restart` is set to `true`. /// /// The returned bool indicates if there are more entries or not. + /// If this returns `false`, no new entries have been written to the buffer. /// You must only iterate the buffer if this returned `true`! + /// /// It is an error if `self` is not a directory. /// /// # Symlinks and other reparse points @@ -881,16 +859,16 @@ impl File { /// So if you open a link (not its target) and iterate the directory, /// you will always iterate an empty directory regardless of the target. #[allow(unused)] - fn fill_dir_buff(&self, buffer: &mut DirBuff, restart: bool) -> Result { + fn fill(&mut self, handle: &Handle, restart: bool) -> Result { let class = if restart { c::FileIdBothDirectoryRestartInfo } else { c::FileIdBothDirectoryInfo }; unsafe { let result = c::GetFileInformationByHandleEx( - self.as_raw_handle(), + handle.as_raw_handle(), class, - buffer.as_mut_ptr().cast(), - buffer.capacity() as _, + self.as_mut_ptr().cast(), + self.capacity() as _, ); if result == 0 { // Apparently ERROR_NO_MORE_FILES also means that the buffer is empty / was left @@ -903,31 +881,6 @@ impl File { } } } - -/// A buffer for holding directory entries. -struct DirBuff { - buffer: Box; Self::BUFFER_SIZE]>>, -} -impl DirBuff { - const BUFFER_SIZE: usize = 1024; - fn new() -> Self { - Self { - // Safety: `Align8<[MaybeUninit; N]>` does not need - // initialization. - buffer: unsafe { Box::new_uninit().assume_init() }, - } - } - fn capacity(&self) -> usize { - self.buffer.0.len() - } - fn as_mut_ptr(&mut self) -> *mut u8 { - self.buffer.0.as_mut_ptr().cast() - } - /// Returns a `DirBuffIter`. - fn iter(&self) -> DirBuffIter<'_> { - DirBuffIter::new(self) - } -} impl AsRef<[MaybeUninit]> for DirBuff { fn as_ref(&self) -> &[MaybeUninit] { &self.buffer.0 @@ -1147,10 +1100,37 @@ impl From for FileAttr { } } } +impl From for FileAttr { + fn from(info: c::FILE_ID_BOTH_DIR_INFO) -> Self { + FileAttr { + attributes: info.FileAttributes, + creation_time: filetime_from_i64(info.CreationTime), + last_access_time: filetime_from_i64(info.LastAccessTime), + last_write_time: filetime_from_i64(info.LastWriteTime), + change_time: Some(filetime_from_i64(info.ChangeTime)), + // We use the EOF marker as file size, i.e., the actual number of bytes that + // can be read -- rather than the allocation size. + file_size: info.EndOfFile as u64, + reparse_tag: if info.FileAttributes & c::FILE_ATTRIBUTE_REPARSE_POINT != 0 { + // reparse tag is stored in `EaSize` + info.EaSize + } else { + 0 + }, + volume_serial_number: None, + number_of_links: None, + // There is a FileIndex field, but it is "undefined for file systems such as NTFS". + file_index: None, + } + } +} fn to_u64(ft: &c::FILETIME) -> u64 { (ft.dwLowDateTime as u64) | ((ft.dwHighDateTime as u64) << 32) } +fn filetime_from_i64(val: i64) -> c::FILETIME { + c::FILETIME { dwLowDateTime: val as u32, dwHighDateTime: (val >> 32) as u32 } +} impl FilePermissions { pub fn readonly(&self) -> bool { @@ -1230,65 +1210,90 @@ impl DirBuilder { } pub fn readdir(p: &Path) -> io::Result { - // We push a `*` to the end of the path which cause the empty path to be - // treated as the current directory. So, for consistency with other platforms, - // we explicitly error on the empty path. - if p.as_os_str().is_empty() { - // Return an error code consistent with other ways of opening files. - // E.g. fs::metadata or File::open. - return Err(io::Error::from_raw_os_error(c::ERROR_PATH_NOT_FOUND as i32)); - } - let root = p.to_path_buf(); - let star = p.join("*"); - let path = maybe_verbatim(&star)?; + // Open a file or directory without following symlinks. + let mut opts = OpenOptions::new(); + opts.access_mode(c::FILE_LIST_DIRECTORY); + // `FILE_FLAG_BACKUP_SEMANTICS` allows opening directories. + opts.custom_flags(c::FILE_FLAG_BACKUP_SEMANTICS); + let file = File::open(p, &opts)?; - unsafe { - let mut wfd: c::WIN32_FIND_DATAW = mem::zeroed(); - // this is like FindFirstFileW (see https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-findfirstfileexw), - // but with FindExInfoBasic it should skip filling WIN32_FIND_DATAW.cAlternateFileName - // (see https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-win32_find_dataw) - // (which will be always null string value and currently unused) and should be faster. - // - // We can pass FIND_FIRST_EX_LARGE_FETCH to dwAdditionalFlags to speed up things more, - // but as we don't know user's use profile of this function, lets be conservative. - let find_handle = c::FindFirstFileExW( - path.as_ptr(), - c::FindExInfoBasic, - &mut wfd as *mut _ as _, - c::FindExSearchNameMatch, - ptr::null(), - 0, - ); + // Test if the file is not a directory. + if (file.basic_info()?.FileAttributes & c::FILE_ATTRIBUTE_DIRECTORY) == 0 { + return Err(io::Error::from_raw_os_error(c::ERROR_DIRECTORY as _)); + } - if find_handle != c::INVALID_HANDLE_VALUE { - Ok(ReadDir { - handle: Some(FindNextFileHandle(find_handle)), - root: Arc::new(root), - first: Some(wfd), - }) - } else { - // The status `ERROR_FILE_NOT_FOUND` is returned by the `FindFirstFileExW` function - // if no matching files can be found, but not necessarily that the path to find the - // files in does not exist. - // - // Hence, a check for whether the path to search in exists is added when the last - // os error returned by Windows is `ERROR_FILE_NOT_FOUND` to handle this scenario. - // If that is the case, an empty `ReadDir` iterator is returned as it returns `None` - // in the initial `.next()` invocation because `ERROR_NO_MORE_FILES` would have been - // returned by the `FindNextFileW` function. - // - // See issue #120040: https://github.com/rust-lang/rust/issues/120040. - let last_error = api::get_last_error(); - if last_error == WinError::FILE_NOT_FOUND { - return Ok(ReadDir { handle: None, root: Arc::new(root), first: None }); + Ok(ReadDir { + handle: file.into_inner(), + root: Arc::new(p.to_path_buf()), + buf: Some(DirBuff::new()), + next_entry: None, // causes entries to be fetched on first `next` + }) +} + +impl Iterator for ReadDir { + type Item = io::Result; + fn next(&mut self) -> Option> { + let Some(buf) = &mut self.buf else { + return None; + }; + + let next_entry = match self.next_entry { + Some(next_entry) => next_entry, + None => { + // Fetch next batch of entries. + let more_entries = match buf.fill(&self.handle, /* restart */ false).io_result() { + Ok(more_entries) => more_entries, + Err(err) => return Some(Err(err)), + }; + + if !more_entries { + // Directory is done! + self.buf = None; + return None; + } + + // Next entry (to be yielded now) is at offset 0. + 0 } + }; - // Just return the error constructed from the raw OS error if the above is not the case. - // - // Note: `ERROR_PATH_NOT_FOUND` would have been returned by the `FindFirstFileExW` function - // when the path to search in does not exist in the first place. - Err(Error::from_raw_os_error(last_error.code as i32)) + // Get the data of the next entry from the buffer. + let info = buf.as_ref()[next_entry..].as_ptr().cast::(); + // SAFETY: + // - The buffer contains a `FILE_ID_BOTH_DIR_INFO` struct but the last + // field (the file name) is unsized. So an offset has to be used to + // get the file name slice. + // - The OS has guaranteed initialization of the fields of + // `FILE_ID_BOTH_DIR_INFO` and the trailing filename (for at least + // `FileNameLength` bytes) + // While this is guaranteed to be aligned in documentation for + // https://docs.microsoft.com/en-us/windows/win32/api/winbase/ns-winbase-file_id_both_dir_info + // it does not seem that reality is so kind, and assuming this + // caused crashes in some cases (https://github.com/rust-lang/rust/issues/104530) + // presumably, this can be blamed on buggy filesystem drivers, but who knows. + let entry = unsafe { info.read_unaligned() }; + // SAFETY: the OS promises to store a name of the given length at this offset. + let name = unsafe { + from_maybe_unaligned( + (&raw const (*info).FileName).cast::(), + (entry.FileNameLength as usize) / size_of::(), + ) + }; + + // Update offset for the next entry after this one. + if entry.NextEntryOffset == 0 { + self.next_entry = None; + } else { + self.next_entry = Some(next_entry + (entry.NextEntryOffset as usize)); } + + // Check if this is an entry we skip (`.` and `..`). + const DOT: u16 = b'.' as u16; + if matches!(*name, [DOT] | [DOT, DOT]) { + return self.next(); + } + + Some(Ok(DirEntry { data: entry, name: Box::from(name), root: self.root.clone() })) } } diff --git a/library/std/src/sys/fs/windows/dir.rs b/library/std/src/sys/fs/windows/dir.rs index 60f8c2522de26..7a265ad2b41e7 100644 --- a/library/std/src/sys/fs/windows/dir.rs +++ b/library/std/src/sys/fs/windows/dir.rs @@ -280,6 +280,25 @@ impl fmt::Debug for Dir { } } +impl AsInner for Dir { + #[inline] + fn as_inner(&self) -> &Handle { + &self.handle + } +} + +impl IntoInner for Dir { + fn into_inner(self) -> Handle { + self.handle + } +} + +impl FromInner for Dir { + fn from_inner(handle: Handle) -> Dir { + Dir { handle } + } +} + #[unstable(feature = "dirfd", issue = "120426")] impl AsRawHandle for fs::Dir { fn as_raw_handle(&self) -> RawHandle { diff --git a/library/std/src/sys/fs/windows/remove_dir_all.rs b/library/std/src/sys/fs/windows/remove_dir_all.rs index f701a22f7d6dd..94479045787ed 100644 --- a/library/std/src/sys/fs/windows/remove_dir_all.rs +++ b/library/std/src/sys/fs/windows/remove_dir_all.rs @@ -176,7 +176,7 @@ pub fn remove_dir_all_iterative(dir: File) -> Result<(), WinError> { while let Some(dir) = dirlist.pop() { let restart = false; // we never have to restart, we iterate every directory only once - let more_data = dir.fill_dir_buff(&mut buffer, restart)?; + let more_data = buffer.fill(&dir.handle, restart)?; if !more_data { // This directory is done! // Attempt to delete, retrying on not empty errors because we may diff --git a/library/std/src/sys/pal/windows/mod.rs b/library/std/src/sys/pal/windows/mod.rs index a9f3cdbdd9d3e..8a8b2b5889db5 100644 --- a/library/std/src/sys/pal/windows/mod.rs +++ b/library/std/src/sys/pal/windows/mod.rs @@ -198,14 +198,6 @@ pub fn os2path(s: &[u16]) -> PathBuf { PathBuf::from(OsString::from_wide(s)) } -pub fn truncate_utf16_at_nul(v: &[u16]) -> &[u16] { - match unrolled_find_u16s(0, v) { - // don't include the 0 - Some(i) => &v[..i], - None => v, - } -} - pub fn ensure_no_nuls>(s: T) -> io::Result { if s.as_ref().encode_wide().any(|b| b == 0) { Err(io::const_error!(io::ErrorKind::InvalidInput, "nul byte found in provided data"))