Skip to content

Replace unsafe security_attributes function with safe inherit_handle alternative #145150

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
Aug 10, 2025
Merged

Replace unsafe security_attributes function with safe inherit_handle alternative #145150

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
15 changes: 10 additions & 5 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 @@ -80,7 +80,7 @@ pub struct OpenOptions {
attributes: u32,
share_mode: u32,
security_qos_flags: u32,
security_attributes: *mut c::SECURITY_ATTRIBUTES,
inherit_handle: bool,
Copy link
Member

Choose a reason for hiding this comment

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

I spent some time wandering through other reviews and... interesting observation: people really don't like bool flags around here - readability issues, they say, I've been switching to enums myself, even for simple true/false cases

Copy link
Member

Choose a reason for hiding this comment

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

I think this is much more relevant for function parameters, but this is mostly a struct field and so it's less relevant (I'd even argue marginally harmful).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd just note that OpenOptions uses bool for most parameters. If this were a public API then we'd likely want to be consistent with those existing methods anyway.

}

#[derive(Clone, PartialEq, Eq, Debug)]
Expand Down Expand Up @@ -203,7 +203,7 @@ impl OpenOptions {
share_mode: c::FILE_SHARE_READ | c::FILE_SHARE_WRITE | c::FILE_SHARE_DELETE,
attributes: 0,
security_qos_flags: 0,
security_attributes: ptr::null_mut(),
inherit_handle: false,
}
}

Expand Down Expand Up @@ -243,8 +243,8 @@ impl OpenOptions {
// receive is `SECURITY_ANONYMOUS = 0x0`, which we can't check for later on.
self.security_qos_flags = flags | c::SECURITY_SQOS_PRESENT;
}
pub fn security_attributes(&mut self, attrs: *mut c::SECURITY_ATTRIBUTES) {
self.security_attributes = attrs;
pub fn inherit_handle(&mut self, inherit: bool) {
self.inherit_handle = inherit;
}

fn get_access_mode(&self) -> io::Result<u32> {
Expand Down Expand Up @@ -307,12 +307,17 @@ impl File {

fn open_native(path: &WCStr, opts: &OpenOptions) -> io::Result<File> {
let creation = opts.get_creation_mode()?;
let sa = c::SECURITY_ATTRIBUTES {
nLength: size_of::<c::SECURITY_ATTRIBUTES>() as u32,
lpSecurityDescriptor: ptr::null_mut(),
bInheritHandle: opts.inherit_handle as c::BOOL,
};
let handle = unsafe {
c::CreateFileW(
path.as_ptr(),
opts.get_access_mode()?,
opts.share_mode,
opts.security_attributes,
if opts.inherit_handle { &sa } else { ptr::null() },
creation,
opts.get_flags_and_attributes(),
ptr::null_mut(),
Expand Down
8 changes: 1 addition & 7 deletions library/std/src/sys/process/windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,16 +623,10 @@ impl Stdio {
// permissions as well as the ability to be inherited to child
// processes (as this is about to be inherited).
Stdio::Null => {
let size = size_of::<c::SECURITY_ATTRIBUTES>();
let mut sa = c::SECURITY_ATTRIBUTES {
nLength: size as u32,
lpSecurityDescriptor: ptr::null_mut(),
bInheritHandle: 1,
};
let mut opts = OpenOptions::new();
opts.read(stdio_id == c::STD_INPUT_HANDLE);
opts.write(stdio_id != c::STD_INPUT_HANDLE);
opts.security_attributes(&mut sa);
opts.inherit_handle(true);
File::open(Path::new(r"\\.\NUL"), &opts).map(|file| file.into_inner())
}
}
Expand Down
Loading