Apply #![deny(unsafe_op_in_unsafe_fn)] to sys/windows
#76676
Conversation
|
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
carbotaniuman
force-pushed
the
unsafe_op_in_unsafe_fn
branch
from
bac8570
to
724c67b
Compare
jyn514
added
C-cleanup
| } | ||
|
|
||
| /// # Safety | ||
| /// | ||
| /// `ptr`, once aligned, must have space for a Header at `ptr.offset(-1)`. |
There was a problem hiding this comment.
This is not a sufficient condition. ptr must not only have space, but also be valid for writing. Consider borrowing from https://doc.rust-lang.org/nightly/std/ptr/fn.write.html#safety, for example.
It looks like regardless this function is wrong as it gets &mut access to uninitialized memory -- it would be worth fixing that before trying to document unsafety here, I think, likely in a separate PR. Or at least leaving a FIXME comment.
There was a problem hiding this comment.
Should I just inline the function and avoid the &mut?
There was a problem hiding this comment.
Change get_header to return a *mut Header and change align_ptr to use ptr::write to write the header.
This comment has been minimized.
This comment has been minimized.
carbotaniuman
force-pushed
the
unsafe_op_in_unsafe_fn
branch
from
67eccea
to
5e2d510
Compare
|
☔ The latest upstream changes (presumably #77436) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
LeSeulArtichaut
added
S-waiting-on-author
|
Ping from triage: @carbotaniuman could you resolve the merge conflicts and address the last review comment? Thanks! |
carbotaniuman
force-pushed
the
unsafe_op_in_unsafe_fn
branch
from
d92e4c3
to
ff27e01
Compare
|
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
rustbot
removed
the
S-waiting-on-author
rustbot
added
the
S-waiting-on-review
|
r? @Mark-Simulacrum as you left some reviews before. |
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { | ||
| allocate_with_flags(layout, c::HEAP_ZERO_MEMORY) | ||
| // SAFETY: the safety contract for `allocate_with_flags must be upheld by the caller. |
There was a problem hiding this comment.
This is likely not sufficient. See e.g. #74477 for necessary commentary here; these comments should show that the function being called has preconditions equivalent to the trait.
There are several more cases in this PR where you assert that the preconditions/safety contract of X must be upheld by the caller, please take a look at those and try to adjust them too.
|
#76676 (comment) is also not yet resolved. |
Mark-Simulacrum
added
S-waiting-on-author
|
@carbotaniuman Ping from triage, would you mind addressing the comments above? Thanks! |
|
@carbotaniuman Pinging again from triage, would you mind addressing the comments above? Thanks! |
|
@carbotaniuman thanks for taking the time to contribute. I have to close this due to inactivity. If you wish and you have the time you can open a new PR with these changes and we'll take it from there. Thanks |
Dylan-DPC-zz
added
S-inactive
Rework `std::sys::windows::alloc` I came across rust-lang#76676 (comment), which points out that there was unsound code in the Windows alloc code, creating a &mut to possibly uninitialized memory. I reworked the code so that that particular issue does not occur anymore, and started adding more documentation and safety comments. Full list of changes: - moved and documented the relevant Windows Heap API functions - refactor `allocate_with_flags` to `allocate` (and remove the other helper functions), which now takes just a `bool` if the memory should be zeroed - add checks for if `GetProcessHeap` returned null - add a test that checks if the size and alignment of a `Header` are indeed <= `MIN_ALIGN` - add `#![deny(unsafe_op_in_unsafe_fn)]` and the necessary unsafe blocks with safety comments I feel like I may have overdone the documenting, the unsoundness fix is the most important part; I could spit this PR up in separate parts.
Rework `std::sys::windows::alloc` I came across rust-lang#76676 (comment), which points out that there was unsound code in the Windows alloc code, creating a &mut to possibly uninitialized memory. I reworked the code so that that particular issue does not occur anymore, and started adding more documentation and safety comments. Full list of changes: - moved and documented the relevant Windows Heap API functions - refactor `allocate_with_flags` to `allocate` (and remove the other helper functions), which now takes just a `bool` if the memory should be zeroed - add checks for if `GetProcessHeap` returned null - add a test that checks if the size and alignment of a `Header` are indeed <= `MIN_ALIGN` - add `#![deny(unsafe_op_in_unsafe_fn)]` and the necessary unsafe blocks with safety comments I feel like I may have overdone the documenting, the unsoundness fix is the most important part; I could spit this PR up in separate parts.
Put unsafe operations in unsafe blocks for sys/windows. Partial fix for #73904.