Create `windows/api.rs` for safer FFI #116816

ChrisDenton · 2023-10-16T18:58:05Z

FFI is inherently unsafe. For memory safety we need to assert that some contract is being upheld on both sides of the FFI, though of course we can only ever check our side. In Rust, unsafe blocks are used to assert safety and // SAFETY comments describing why it is safe. Currently in sys/windows we have a lot of this unsafety spread all over the place, with variations on the same unsafe patterns repeated. And because of the repitition and frequency, we're a bit lax with the safety comments.

This PR aims to fix this and to make FFI safety more auditable by creating an api module with the goal of centralising and consolidating this unsafety. It contains thin wrappers around the Windows API that make most functions safe to call or, if that's not possible, then at least safer. Note that its goal is only to address safety. It does not stray far from the Windows API and intentionally does not attempt to make higher lever wrappers around, for example, file handles. This is better left to the existing modules. The windows/api.rs file has a top level comment to help future contributors understand the intent of the module and the design decisions made.

I chose two functions as a first tentative step towards the above goal:

GetLastError is trivially safe. There's no reason to wrap it in an unsafe block every time. So I simply created a safe get_last_error wrapper.
SetFileInformationByHandle is more complex. It essentially takes a generic type but over a C API which necessitates some amount of ceremony. Rather than implementing similar unsafe patterns in multiple places, I provide a safe set_file_information_by_handle that takes a Rusty generic type and handles converting that to the form required by the C FFI.

r? libs

workingjubilee · 2023-10-16T20:55:15Z

r? @workingjubilee

If someone else familiiar with Windows wants to get to this before me, be my guest, but queueing this for now.

ChrisDenton · 2023-10-16T21:10:22Z

Thanks @workingjubilee! I'd be interested in a general vibe check. Like do you think this is a good idea in principle (the specific implementation aside).

workingjubilee · 2023-10-18T05:17:58Z

Yes, I do. I think std::sys rather direly needs organization and this looks like a decent step towards it.

workingjubilee

There were a few "hrm, really?" moments but then I did the doublecheck on the types/etc. and it seems exactly what we want. Perfect, no notes.

workingjubilee · 2023-10-28T07:39:38Z

@bors r+

bors · 2023-10-28T07:39:40Z

📌 Commit 3733316 has been approved by workingjubilee

It is now in the queue for this repository.

…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#116534 (Remove -Zdep-tasks.) - rust-lang#116739 (Make `E0277` use short paths) - rust-lang#116816 (Create `windows/api.rs` for safer FFI) - rust-lang#116945 (When encountering sealed traits, point types that implement it) - rust-lang#117025 (Cleanup and improve `--check-cfg` implementation) - rust-lang#117256 (Parse rustc version at compile time) - rust-lang#117268 (`rustc_interface` cleanups) - rust-lang#117277 (fix failure to detect a too-big-type after adding padding) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#116816 - ChrisDenton:api.rs, r=workingjubilee Create `windows/api.rs` for safer FFI FFI is inherently unsafe. For memory safety we need to assert that some contract is being upheld on both sides of the FFI, though of course we can only ever check our side. In Rust, `unsafe` blocks are used to assert safety and `// SAFETY` comments describing why it is safe. Currently in sys/windows we have a lot of this unsafety spread all over the place, with variations on the same unsafe patterns repeated. And because of the repitition and frequency, we're a bit lax with the safety comments. This PR aims to fix this and to make FFI safety more auditable by creating an `api` module with the goal of centralising and consolidating this unsafety. It contains thin wrappers around the Windows API that make most functions safe to call or, if that's not possible, then at least safer. Note that its goal is *only* to address safety. It does not stray far from the Windows API and intentionally does not attempt to make higher lever wrappers around, for example, file handles. This is better left to the existing modules. The windows/api.rs file has a top level comment to help future contributors understand the intent of the module and the design decisions made. I chose two functions as a first tentative step towards the above goal: - [`GetLastError`](https://learn.microsoft.com/en-us/windows/win32/api/errhandlingapi/nf-errhandlingapi-getlasterror) is trivially safe. There's no reason to wrap it in an `unsafe` block every time. So I simply created a safe `get_last_error` wrapper. - [`SetFileInformationByHandle`](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-setfileinformationbyhandle) is more complex. It essentially takes a generic type but over a C API which necessitates some amount of ceremony. Rather than implementing similar unsafe patterns in multiple places, I provide a safe `set_file_information_by_handle` that takes a Rusty generic type and handles converting that to the form required by the C FFI. r? libs

klensy · 2023-12-05T16:01:30Z

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

+    // Const assert that the size is less than u32::MAX.
+    // Uses a trait to workaround restriction on using generic types in inner items.
+    trait Win32SizeOf: Sized {
+        const WIN32_SIZE_OF: u32 = {
+            let size = core::mem::size_of::<Self>();
+            assert!(size <= u32::MAX as usize);


Comment says about less than u32::MAX, but actual code checks assert!(size <= u32::MAX as usize);

I assume this is because the size should not exceed u32::MAX, right @ChrisDenton? Happy to r+ a patch changing the comment to that effect.

Yeah, I probably meant to write "less than or equal" but "not exceed" reads better.

library: fix comment about const assert in win api Resolves [comment ](rust-lang#116816 (comment)) r? `@workingjubilee`

Rollup merge of rust-lang#118669 - klensy:comment-fix, r=workingjubilee library: fix comment about const assert in win api Resolves [comment ](rust-lang#116816 (comment)) r? `@workingjubilee`

rustbot assigned joshtriplett Oct 16, 2023

rustbot added O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 16, 2023

Create windows/api.rs for safer FFI

3733316

ChrisDenton force-pushed the api.rs branch from f30309f to 3733316 Compare October 16, 2023 19:05

rustbot assigned workingjubilee and unassigned joshtriplett Oct 16, 2023

joboet mentioned this pull request Oct 22, 2023

Tracking Issue for restructuring sys_common #84187

Closed

workingjubilee approved these changes Oct 28, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 28, 2023

workingjubilee mentioned this pull request Oct 28, 2023

Rollup of 8 pull requests #117309

Merged

bors merged commit d87b5e4 into rust-lang:master Oct 28, 2023
11 checks passed

rustbot added this to the 1.75.0 milestone Oct 28, 2023

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

ChrisDenton deleted the api.rs branch November 21, 2023 11:22

klensy reviewed Dec 5, 2023

View reviewed changes

klensy mentioned this pull request Dec 6, 2023

library: fix comment about const assert in win api #118669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `windows/api.rs` for safer FFI #116816

Create `windows/api.rs` for safer FFI #116816

ChrisDenton commented Oct 16, 2023

workingjubilee commented Oct 16, 2023

ChrisDenton commented Oct 16, 2023

workingjubilee commented Oct 18, 2023

workingjubilee left a comment

workingjubilee commented Oct 28, 2023

bors commented Oct 28, 2023

klensy Dec 5, 2023

workingjubilee Dec 5, 2023

ChrisDenton Dec 5, 2023

Create windows/api.rs for safer FFI #116816

Create windows/api.rs for safer FFI #116816

Conversation

ChrisDenton commented Oct 16, 2023

workingjubilee commented Oct 16, 2023

ChrisDenton commented Oct 16, 2023

workingjubilee commented Oct 18, 2023

workingjubilee left a comment

Choose a reason for hiding this comment

workingjubilee commented Oct 28, 2023

bors commented Oct 28, 2023

klensy Dec 5, 2023

Choose a reason for hiding this comment

workingjubilee Dec 5, 2023

Choose a reason for hiding this comment

ChrisDenton Dec 5, 2023

Choose a reason for hiding this comment

Create `windows/api.rs` for safer FFI #116816

Create `windows/api.rs` for safer FFI #116816