Skip to content

GetUserProfileDirectoryW is now documented to always store the size #141405

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
May 24, 2025
+3 −5
Merged

GetUserProfileDirectoryW is now documented to always store the size #141405

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
2 changes: 0 additions & 2 deletions library/std/src/sys/pal/windows/os.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,6 @@ fn home_dir_crt() -> Option<PathBuf> {
|buf, mut sz| {
// GetUserProfileDirectoryW does not quite use the usual protocol for
// negotiating the buffer size, so we have to translate.
// FIXME(#141254): We rely on the *undocumented* property that this function will
// always set the size, not just on failure.
match c::GetUserProfileDirectoryW(
ptr::without_provenance_mut(CURRENT_PROCESS_TOKEN),
buf,
Expand Down
6 changes: 3 additions & 3 deletions src/tools/miri/src/shims/windows/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,16 +230,16 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
interp_ok(match directories::UserDirs::new() {
Some(dirs) => {
let home = dirs.home_dir();
let size_avail = if this.ptr_is_null(size.ptr())? {
let size_avail = if this.ptr_is_null(buf)? {
Copy link
Member Author

Choose a reason for hiding this comment

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

The comment already talked about checking buf, not size... and we unconditionally write size so it being null already triggered UB before.

Copy link
Contributor

@cgettys-microsoft cgettys-microsoft May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense to me - docs agree that lpcchSize is required:
https://learn.microsoft.com/en-us/windows/win32/api/userenv/nf-userenv-getuserprofiledirectoryw

As does SAL
https://github.com/microsoft/win32metadata/blob/c7ffce1c97bb724580580f3856b94c6d4ad677ed/generation/WinSDK/RecompiledIdlHeaders/um/UserEnv.h#L375

    _In_                            HANDLE  hToken,
    _Out_writes_opt_(*lpcchSize)    LPWSTR lpProfileDir,
    _Inout_                         LPDWORD lpcchSize

(There's no Opt suffix to denote that nullptr is an allowed value for lpcchSize)

Great work as always :)

0 // if the buf pointer is null, we can't write to it; `size` will be updated to the required length
} else {
this.read_scalar(&size)?.to_u32()?
};
// Of course we cannot use `windows_check_buffer_size` here since this uses
// a different method for dealing with a too-small buffer than the other functions...
let (success, len) = this.write_path_to_wide_str(home, buf, size_avail.into())?;
// The Windows docs just say that this is written on failure, but std relies on it
// always being written. Also see <https://github.com/rust-lang/rust/issues/141254>.
// As per <https://github.com/MicrosoftDocs/sdk-api/pull/1810>, the size is always
// written, not just on failure.
this.write_scalar(Scalar::from_u32(len.try_into().unwrap()), &size)?;
if success {
Scalar::from_i32(1) // return TRUE
Expand Down
Loading