Skip to content

Use with_native_path for Windows #139683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 13, 2025
Merged

Use with_native_path for Windows #139683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions library/std/src/sys/fs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ cfg_if::cfg_if! {
mod windows;
use windows as imp;
pub use windows::{symlink_inner, junction_point};
use crate::sys::path::with_native_path;
} else if #[cfg(target_os = "hermit")] {
mod hermit;
use hermit as imp;
Expand All @@ -39,7 +40,7 @@ cfg_if::cfg_if! {
}

// FIXME: Replace this with platform-specific path conversion functions.
#[cfg(not(target_family = "unix"))]
#[cfg(not(any(target_family = "unix", target_os = "windows")))]
#[inline]
pub fn with_native_path<T>(path: &Path, f: &dyn Fn(&Path) -> io::Result<T>) -> io::Result<T> {
f(path)
Expand All @@ -51,7 +52,7 @@ pub use imp::{
};

pub fn read_dir(path: &Path) -> io::Result<ReadDir> {
// FIXME: use with_native_path
// FIXME: use with_native_path on all platforms
imp::readdir(path)
}

Expand All @@ -68,15 +69,22 @@ pub fn remove_dir(path: &Path) -> io::Result<()> {
}

pub fn remove_dir_all(path: &Path) -> io::Result<()> {
// FIXME: use with_native_path
imp::remove_dir_all(path)
// FIXME: use with_native_path on all platforms
#[cfg(not(windows))]
return imp::remove_dir_all(path);
#[cfg(windows)]
with_native_path(path, &imp::remove_dir_all)
}

pub fn read_link(path: &Path) -> io::Result<PathBuf> {
with_native_path(path, &imp::readlink)
}

pub fn symlink(original: &Path, link: &Path) -> io::Result<()> {
// FIXME: use with_native_path on all platforms
#[cfg(windows)]
return imp::symlink(original, link);
#[cfg(not(windows))]
Comment on lines +85 to +87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to double check, this has the oposite logic to the other cfg's

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's intentional. Because this is the one case where we can't (yet) simply pass through a native path to the Windows function but we can for Unix. And fortunately if I get the logic wrong here it'll be a loud error due to the signatures not matching.

with_native_path(original, &|original| {
with_native_path(link, &|link| imp::symlink(original, link))
})
Expand Down Expand Up @@ -105,11 +113,17 @@ pub fn canonicalize(path: &Path) -> io::Result<PathBuf> {
}

pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
// FIXME: use with_native_path
imp::copy(from, to)
// FIXME: use with_native_path on all platforms
#[cfg(not(windows))]
return imp::copy(from, to);
#[cfg(windows)]
with_native_path(from, &|from| with_native_path(to, &|to| imp::copy(from, to)))
}

pub fn exists(path: &Path) -> io::Result<bool> {
// FIXME: use with_native_path
imp::exists(path)
// FIXME: use with_native_path on all platforms
#[cfg(not(windows))]
return imp::exists(path);
#[cfg(windows)]
with_native_path(path, &imp::exists)
}
69 changes: 30 additions & 39 deletions library/std/src/sys/fs/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::sync::Arc;
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::path::maybe_verbatim;
use crate::sys::path::{WCStr, maybe_verbatim};
use crate::sys::time::SystemTime;
use crate::sys::{Align8, c, cvt};
use crate::sys_common::{AsInner, FromInner, IntoInner};
Expand Down Expand Up @@ -298,10 +298,12 @@ impl OpenOptions {
impl File {
pub fn open(path: &Path, opts: &OpenOptions) -> io::Result<File> {
let path = maybe_verbatim(path)?;
// SAFETY: maybe_verbatim returns null-terminated strings
let path = unsafe { WCStr::from_wchars_with_null_unchecked(&path) };
Self::open_native(&path, opts)
}

fn open_native(path: &[u16], opts: &OpenOptions) -> io::Result<File> {
fn open_native(path: &WCStr, opts: &OpenOptions) -> io::Result<File> {
let creation = opts.get_creation_mode()?;
let handle = unsafe {
c::CreateFileW(
Expand Down Expand Up @@ -1212,9 +1214,8 @@ pub fn readdir(p: &Path) -> io::Result<ReadDir> {
}
}

pub fn unlink(p: &Path) -> io::Result<()> {
let p_u16s = maybe_verbatim(p)?;
if unsafe { c::DeleteFileW(p_u16s.as_ptr()) } == 0 {
pub fn unlink(path: &WCStr) -> io::Result<()> {
if unsafe { c::DeleteFileW(path.as_ptr()) } == 0 {
let err = api::get_last_error();
// if `DeleteFileW` fails with ERROR_ACCESS_DENIED then try to remove
// the file while ignoring the readonly attribute.
Expand All @@ -1223,7 +1224,7 @@ pub fn unlink(p: &Path) -> io::Result<()> {
let mut opts = OpenOptions::new();
opts.access_mode(c::DELETE);
opts.custom_flags(c::FILE_FLAG_OPEN_REPARSE_POINT);
if let Ok(f) = File::open_native(&p_u16s, &opts) {
if let Ok(f) = File::open_native(&path, &opts) {
if f.posix_delete().is_ok() {
return Ok(());
}
Expand All @@ -1236,10 +1237,7 @@ pub fn unlink(p: &Path) -> io::Result<()> {
}
}

pub fn rename(old: &Path, new: &Path) -> io::Result<()> {
let old = maybe_verbatim(old)?;
let new = maybe_verbatim(new)?;

pub fn rename(old: &WCStr, new: &WCStr) -> io::Result<()> {
if unsafe { c::MoveFileExW(old.as_ptr(), new.as_ptr(), c::MOVEFILE_REPLACE_EXISTING) } == 0 {
let err = api::get_last_error();
// if `MoveFileExW` fails with ERROR_ACCESS_DENIED then try to move
Expand All @@ -1253,7 +1251,8 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> {

// Calculate the layout of the `FILE_RENAME_INFO` we pass to `SetFileInformation`
// This is a dynamically sized struct so we need to get the position of the last field to calculate the actual size.
let Ok(new_len_without_nul_in_bytes): Result<u32, _> = ((new.len() - 1) * 2).try_into()
let Ok(new_len_without_nul_in_bytes): Result<u32, _> =
((new.count_bytes() - 1) * 2).try_into()
else {
return Err(err).io_result();
};
Expand Down Expand Up @@ -1282,7 +1281,7 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> {

new.as_ptr().copy_to_nonoverlapping(
(&raw mut (*file_rename_info).FileName).cast::<u16>(),
new.len(),
new.count_bytes(),
);
}

Expand All @@ -1309,20 +1308,19 @@ pub fn rename(old: &Path, new: &Path) -> io::Result<()> {
Ok(())
}

pub fn rmdir(p: &Path) -> io::Result<()> {
let p = maybe_verbatim(p)?;
pub fn rmdir(p: &WCStr) -> io::Result<()> {
cvt(unsafe { c::RemoveDirectoryW(p.as_ptr()) })?;
Ok(())
}

pub fn remove_dir_all(path: &Path) -> io::Result<()> {
pub fn remove_dir_all(path: &WCStr) -> io::Result<()> {
// 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.
// `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);
let file = File::open(path, &opts)?;
let file = File::open_native(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 {
Expand All @@ -1333,14 +1331,14 @@ pub fn remove_dir_all(path: &Path) -> io::Result<()> {
remove_dir_all_iterative(file).io_result()
}

pub fn readlink(path: &Path) -> io::Result<PathBuf> {
pub fn readlink(path: &WCStr) -> io::Result<PathBuf> {
// Open the link with no access mode, instead of generic read.
// By default FILE_LIST_DIRECTORY is denied for the junction "C:\Documents and Settings", so
// this is needed for a common case.
let mut opts = OpenOptions::new();
opts.access_mode(0);
opts.custom_flags(c::FILE_FLAG_OPEN_REPARSE_POINT | c::FILE_FLAG_BACKUP_SEMANTICS);
let file = File::open(path, &opts)?;
let file = File::open_native(&path, &opts)?;
file.readlink()
}

Expand Down Expand Up @@ -1378,19 +1376,17 @@ pub fn symlink_inner(original: &Path, link: &Path, dir: bool) -> io::Result<()>
}

#[cfg(not(target_vendor = "uwp"))]
pub fn link(original: &Path, link: &Path) -> io::Result<()> {
let original = maybe_verbatim(original)?;
let link = maybe_verbatim(link)?;
pub fn link(original: &WCStr, link: &WCStr) -> io::Result<()> {
cvt(unsafe { c::CreateHardLinkW(link.as_ptr(), original.as_ptr(), ptr::null_mut()) })?;
Ok(())
}

#[cfg(target_vendor = "uwp")]
pub fn link(_original: &Path, _link: &Path) -> io::Result<()> {
pub fn link(_original: &WCStr, _link: &WCStr) -> io::Result<()> {
return Err(io::const_error!(io::ErrorKind::Unsupported, "hard link are not supported on UWP"));
}

pub fn stat(path: &Path) -> io::Result<FileAttr> {
pub fn stat(path: &WCStr) -> io::Result<FileAttr> {
match metadata(path, ReparsePoint::Follow) {
Err(err) if err.raw_os_error() == Some(c::ERROR_CANT_ACCESS_FILE as i32) => {
if let Ok(attrs) = lstat(path) {
Expand All @@ -1404,7 +1400,7 @@ pub fn stat(path: &Path) -> io::Result<FileAttr> {
}
}

pub fn lstat(path: &Path) -> io::Result<FileAttr> {
pub fn lstat(path: &WCStr) -> io::Result<FileAttr> {
metadata(path, ReparsePoint::Open)
}

Expand All @@ -1420,7 +1416,7 @@ impl ReparsePoint {
}
}

fn metadata(path: &Path, reparse: ReparsePoint) -> io::Result<FileAttr> {
fn metadata(path: &WCStr, reparse: ReparsePoint) -> io::Result<FileAttr> {
let mut opts = OpenOptions::new();
// No read or write permissions are necessary
opts.access_mode(0);
Expand All @@ -1429,7 +1425,7 @@ fn metadata(path: &Path, reparse: ReparsePoint) -> io::Result<FileAttr> {
// Attempt to open the file normally.
// If that fails with `ERROR_SHARING_VIOLATION` then retry using `FindFirstFileExW`.
// If the fallback fails for any reason we return the original error.
match File::open(path, &opts) {
match File::open_native(&path, &opts) {
Ok(file) => file.file_attr(),
Err(e)
if [Some(c::ERROR_SHARING_VIOLATION as _), Some(c::ERROR_ACCESS_DENIED as _)]
Expand All @@ -1442,8 +1438,6 @@ fn metadata(path: &Path, reparse: ReparsePoint) -> io::Result<FileAttr> {
// However, there are special system files, such as
// `C:\hiberfil.sys`, that are locked in a way that denies even that.
unsafe {
let path = maybe_verbatim(path)?;

// `FindFirstFileExW` accepts wildcard file names.
// Fortunately wildcards are not valid file names and
// `ERROR_SHARING_VIOLATION` means the file exists (but is locked)
Expand Down Expand Up @@ -1482,8 +1476,7 @@ fn metadata(path: &Path, reparse: ReparsePoint) -> io::Result<FileAttr> {
}
}

pub fn set_perm(p: &Path, perm: FilePermissions) -> io::Result<()> {
let p = maybe_verbatim(p)?;
pub fn set_perm(p: &WCStr, perm: FilePermissions) -> io::Result<()> {
unsafe {
cvt(c::SetFileAttributesW(p.as_ptr(), perm.attrs))?;
Ok(())
Expand All @@ -1499,17 +1492,17 @@ fn get_path(f: &File) -> io::Result<PathBuf> {
)
}

pub fn canonicalize(p: &Path) -> io::Result<PathBuf> {
pub fn canonicalize(p: &WCStr) -> io::Result<PathBuf> {
let mut opts = OpenOptions::new();
// No read or write permissions are necessary
opts.access_mode(0);
// This flag is so we can open directories too
opts.custom_flags(c::FILE_FLAG_BACKUP_SEMANTICS);
let f = File::open(p, &opts)?;
let f = File::open_native(p, &opts)?;
get_path(&f)
}

pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
pub fn copy(from: &WCStr, to: &WCStr) -> io::Result<u64> {
unsafe extern "system" fn callback(
_TotalFileSize: i64,
_TotalBytesTransferred: i64,
Expand All @@ -1528,13 +1521,11 @@ pub fn copy(from: &Path, to: &Path) -> io::Result<u64> {
c::PROGRESS_CONTINUE
}
}
let pfrom = maybe_verbatim(from)?;
let pto = maybe_verbatim(to)?;
let mut size = 0i64;
cvt(unsafe {
c::CopyFileExW(
pfrom.as_ptr(),
pto.as_ptr(),
from.as_ptr(),
to.as_ptr(),
Some(callback),
(&raw mut size) as *mut _,
ptr::null_mut(),
Expand Down Expand Up @@ -1624,14 +1615,14 @@ pub fn junction_point(original: &Path, link: &Path) -> io::Result<()> {
}

// Try to see if a file exists but, unlike `exists`, report I/O errors.
pub fn exists(path: &Path) -> io::Result<bool> {
pub fn exists(path: &WCStr) -> io::Result<bool> {
// Open the file to ensure any symlinks are followed to their target.
let mut opts = OpenOptions::new();
// No read, write, etc access rights are needed.
opts.access_mode(0);
// Backup semantics enables opening directories as well as files.
opts.custom_flags(c::FILE_FLAG_BACKUP_SEMANTICS);
match File::open(path, &opts) {
match File::open_native(path, &opts) {
Err(e) => match e.kind() {
// The file definitely does not exist
io::ErrorKind::NotFound => Ok(false),
Expand Down
34 changes: 34 additions & 0 deletions library/std/src/sys/path/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,40 @@ mod tests;
pub const MAIN_SEP_STR: &str = "\\";
pub const MAIN_SEP: char = '\\';

/// A null terminated wide string.
#[repr(transparent)]
pub struct WCStr([u16]);

impl WCStr {
/// Convert a slice to a WCStr without checks.
///
/// Though it is memory safe, the slice should also not contain interior nulls
/// as this may lead to unwanted truncation.
///
/// # Safety
///
/// The slice must end in a null.
pub unsafe fn from_wchars_with_null_unchecked(s: &[u16]) -> &Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, the convention in CStr is to write nul to refer to the ASCII byte and null to refer to a pointer.

It’s not worth bumping this out of the queue, but something to possibly keep in mind for your next PR in the series.

unsafe { &*(s as *const [u16] as *const Self) }
}

pub fn as_ptr(&self) -> *const u16 {
self.0.as_ptr()
}

pub fn count_bytes(&self) -> usize {
self.0.len()
}
}

#[inline]
pub fn with_native_path<T>(path: &Path, f: &dyn Fn(&WCStr) -> io::Result<T>) -> io::Result<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Curiosity) why do these use &dyn rather than impl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the optimisation made in #121101. I've not personally looked into the trade-offs though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Binary size, see #121101.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks, I didn't realize this existed before #138832

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the optimisation made in #121101. I've not personally looked into the trade-offs though.

Jinxed 😄. It might be possible to improve things by splitting the string validation and copying to a separate, non-generic function but keep the stack allocation and inner function call in the generic part. Maybe I'll pursue that once this PR is merged...

let path = maybe_verbatim(path)?;
// SAFETY: maybe_verbatim returns null-terminated strings
let path = unsafe { WCStr::from_wchars_with_null_unchecked(&path) };
f(path)
}

#[inline]
pub fn is_sep_byte(b: u8) -> bool {
b == b'/' || b == b'\\'
Expand Down
Loading