Fix UB from misalignment and provenance widening in `std::sys::windows` #101171

thomcc · 2022-08-29T22:49:04Z

This fixes two types of UB:

Reading past the end of a reference in types like &c::REPARSE_DATA_BUFFER (see Storing an object as &Header, but reading the data past the end of the header unsafe-code-guidelines#256). This is fixed by using addr_of!. I think there are probably a couple more cases where we do this for other structures, and will look into it in a bit.
Failing to ensure that a [u8; N] on the stack is sufficiently aligned to convert to a REPARSE_DATA_BUFFER. This was done by introducing a new AlignedAs struct that allows aligning one type to the alignment of another type. I expect there are other places where we have this issue too, or I wouldn't introduce this type, but will get to them after this lands.

Worth noting, it is implemented in a way that can cause problems depending on how we fix repr(C) on MSVC targets does not always match MSVC type layout when ZST are involved #81996, but this would be caught by the test I added (and presumably if we decide to fix that in a way that would break this code, we'd also introduce a #[repr(simple)] or #[repr(linear)] as a replacement for this usage of #[repr(C)]).

Edit: None of that is still in the code, I just went with a Align8 since that's all we'll need for almost everything we want to call.

These are more or less "potential UB" since it's likely at the moment everything works fine, although the alignment not causing issues might just be down to luck (and x86 being forgiving).

~~NB: I've only ensured this check builds, but will run tests soon.~~ All tests pass, including stage2 compiler tests.

r? @ChrisDenton

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

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

ChrisDenton

Ok, I think this is basically fine. Only one thing that I think needs to be addressed, the other comments are just me being resigned to Rust not having better support for this atm. 🤷

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

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

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

ChrisDenton · 2022-08-30T06:58:28Z

Oh, just noticed:

rust/library/std/src/sys/windows/fs.rs

Line 1348 in 1b8025a

let mut data = [0u8; c::MAXIMUM_REPARSE_DATA_BUFFER_SIZE];

EDIT: Wait no that's a different buffer... for some reason.

thomcc · 2022-08-30T07:07:57Z

EDIT: Wait no that's a different buffer... for some reason.

It's still wrong the way it's written, I think?

That said there are a couple places in this file that are like this that I didn't touch (for example, it's not 100% clear to me what the best fix for the DirBuff stuff would be, but I was just going to tackle it later).

ChrisDenton · 2022-08-30T07:16:37Z

It's still wrong the way it's written, I think?

Yeah, but I'm ok with this PR being more focused. That said, I'm also more than happy for you to fix that too!

thomcc · 2022-08-30T07:20:07Z

I fixed it. It actually was wrong in another way, since it was turning a pointer derived from data.as_ptr() into a mutable pointer, which is wrong because it's mutating a pointer derived from &self, which can't give out mutable pointers.

(Tragically as_mut_ptr() would also be wrong -- that goes through &mut self which would invalidate the other pointers derived from it, so I just added a new local for data.value.as_mut_ptr())

ChrisDenton · 2022-08-30T07:32:47Z

lgtm

@bors r+

bors · 2022-08-30T07:32:49Z

📌 Commit d9c760d has been approved by ChrisDenton

It is now in the queue for this repository.

thomcc · 2022-08-30T08:05:14Z

@bors r-

After a quick discussion with @ChrisDenton I'm actually going to simplify this a bit.

thomcc · 2022-08-30T08:55:17Z

I'll remove the donotland CI changes after it passes (don't have a convenient windows box right now)

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

thomcc · 2022-08-30T13:14:24Z

Windows passes (https://github.com/rust-lang/rust/runs/8091513764?check_suite_focus=true, https://github.com/rust-lang/rust/runs/8091513602?check_suite_focus=true), so I've undone the CI commit.

ChrisDenton · 2022-08-30T18:00:50Z

@bors r+

bors · 2022-08-30T18:00:52Z

📌 Commit c41f21b has been approved by ChrisDenton

It is now in the queue for this repository.

Fix UB from misalignment and provenance widening in `std::sys::windows` This fixes two types of UB: 1. Reading past the end of a reference in types like `&c::REPARSE_DATA_BUFFER` (see rust-lang/unsafe-code-guidelines#256). This is fixed by using `addr_of!`. I think there are probably a couple more cases where we do this for other structures, and will look into it in a bit. 2. Failing to ensure that a `[u8; N]` on the stack is sufficiently aligned to convert to a `REPARSE_DATA_BUFFER`. ~~This was done by introducing a new `AlignedAs` struct that allows aligning one type to the alignment of another type. I expect there are other places where we have this issue too, or I wouldn't introduce this type, but will get to them after this lands.~~ ~~Worth noting, it *is* implemented in a way that can cause problems depending on how we fix rust-lang#81996, but this would be caught by the test I added (and presumably if we decide to fix that in a way that would break this code, we'd also introduce a `#[repr(simple)]` or `#[repr(linear)]` as a replacement for this usage of `#[repr(C)]`).~~ Edit: None of that is still in the code, I just went with a `Align8` since that's all we'll need for almost everything we want to call. These are more or less "potential UB" since it's likely at the moment everything works fine, although the alignment not causing issues might just be down to luck (and x86 being forgiving). ~~NB: I've only ensured this check builds, but will run tests soon.~~ All tests pass, including stage2 compiler tests. r? `@ChrisDenton`

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#100970 (Allow deriving multipart suggestions) - rust-lang#100984 (Reinstate preloading of some dll imports) - rust-lang#101011 (Use getentropy when possible on all Apple platforms) - rust-lang#101025 (Add tier-3 support for powerpc64 and riscv64 openbsd) - rust-lang#101049 (Remove span fatal from ast lowering) - rust-lang#101100 (Make call suggestions more general and more accurate) - rust-lang#101171 (Fix UB from misalignment and provenance widening in `std::sys::windows`) - rust-lang#101185 (Tweak `WellFormedLoc`s a bit) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Avoid needless buffer zeroing in `std::sys::windows::fs` Followup to rust-lang#101171 and rust-lang#101193. This finishes up avoiding buffer zeroing pointed out in rust-lang#100729 (comment) (thanks!) r? `@ChrisDenton`

Avoid UB in the Windows filesystem code in... bootstrap? This basically a subset of the changes from rust-lang#101171. I didn't think to look in src/bootstrap for more windows filesystem API usage, which was apparently a mistake on my part. It's kinda goofy that stuff like this is in here, but what are you gonna do, computers are awful. I also added `winbase` to the `winapi` dep -- I tested this in a tmp crate but needed to add this to your Cargo.toml -- you `use winapi::stuff::winbase` in this function, but are relying on something else turning on that feature.

Avoid needless buffer zeroing in `std::sys::windows::fs` Followup to rust-lang/rust#101171 and rust-lang/rust#101193. This finishes up avoiding buffer zeroing pointed out in rust-lang/rust#100729 (comment) (thanks!) r? `@ChrisDenton`

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 29, 2022

rust-highfive assigned ChrisDenton Aug 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 29, 2022

thomcc commented Aug 29, 2022

View reviewed changes

library/std/src/sys/windows/fs.rs Outdated Show resolved Hide resolved

thomcc added the O-windows Operating system: Windows label Aug 29, 2022

Fix some possible UB in std::sys::windows

1b8025a

thomcc force-pushed the fix-winfs-ub branch from 9962b0d to 1b8025a Compare August 29, 2022 23:00

ChrisDenton reviewed Aug 30, 2022

View reviewed changes

library/std/src/sys/windows/c.rs Show resolved Hide resolved

ChrisDenton reviewed Aug 30, 2022

View reviewed changes

library/std/src/sys/windows/c.rs Show resolved Hide resolved

library/std/src/sys/windows/mod.rs Outdated Show resolved Hide resolved

library/std/src/sys/windows/fs.rs Outdated Show resolved Hide resolved

Fix UWP and use AlignedReparseBuf in symlink_junction_inner

d9c760d

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 Aug 30, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 30, 2022

Replace AlignedAs with a more specific Align8 type

5c3490c

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 30, 2022

ChrisDenton reviewed Aug 30, 2022

View reviewed changes

library/std/src/sys/windows/fs.rs Outdated Show resolved Hide resolved

thomcc force-pushed the fix-winfs-ub branch from 2bf542b to 52ebbec Compare August 30, 2022 09:43

This comment has been minimized.

Sign in to view

thomcc force-pushed the fix-winfs-ub branch from 9d4e79b to 293d97b Compare August 30, 2022 10:46

Fix UB in Windows DirBuffIter (provenance and alignment)

c41f21b

thomcc force-pushed the fix-winfs-ub branch from 293d97b to 9199c31 Compare August 30, 2022 12:04

thomcc mentioned this pull request Aug 30, 2022

Add IsTerminal trait to determine if a descriptor or handle is a terminal #98033

Merged

thomcc force-pushed the fix-winfs-ub branch from 9199c31 to c41f21b Compare August 30, 2022 13:14

thomcc removed the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 30, 2022

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 30, 2022

This was referenced Aug 31, 2022

Rollup of 9 pull requests #101223

Closed

Rollup of 8 pull requests #101225

Merged

bors merged commit 0ed046f into rust-lang:master Aug 31, 2022

rustbot added this to the 1.65.0 milestone Aug 31, 2022

thomcc mentioned this pull request Aug 31, 2022

Avoid needless buffer zeroing in std::sys::windows::fs #101236

Merged

thomcc deleted the fix-winfs-ub branch August 31, 2022 15:26

This was referenced Sep 4, 2022

Add Rust std support for x86_64-unknown-uefi #100316

Closed

Avoid UB in the Windows filesystem code in... bootstrap? #101455

Merged

badboy mentioned this pull request Nov 17, 2022

std::fs::remove_dir_all occasionally panics on Windows with assertion failed: info.is_aligned() #104530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix UB from misalignment and provenance widening in `std::sys::windows` #101171

Fix UB from misalignment and provenance widening in `std::sys::windows` #101171

thomcc commented Aug 29, 2022 •

edited

Loading

ChrisDenton left a comment

ChrisDenton commented Aug 30, 2022 •

edited

Loading

thomcc commented Aug 30, 2022

ChrisDenton commented Aug 30, 2022

thomcc commented Aug 30, 2022

ChrisDenton commented Aug 30, 2022

bors commented Aug 30, 2022

thomcc commented Aug 30, 2022

thomcc commented Aug 30, 2022

This comment has been minimized.

This comment has been minimized.

thomcc commented Aug 30, 2022

ChrisDenton commented Aug 30, 2022

bors commented Aug 30, 2022

Fix UB from misalignment and provenance widening in std::sys::windows #101171

Fix UB from misalignment and provenance widening in std::sys::windows #101171

Conversation

thomcc commented Aug 29, 2022 • edited Loading

ChrisDenton left a comment

Choose a reason for hiding this comment

ChrisDenton commented Aug 30, 2022 • edited Loading

thomcc commented Aug 30, 2022

ChrisDenton commented Aug 30, 2022

thomcc commented Aug 30, 2022

ChrisDenton commented Aug 30, 2022

bors commented Aug 30, 2022

thomcc commented Aug 30, 2022

thomcc commented Aug 30, 2022

This comment has been minimized.

This comment has been minimized.

thomcc commented Aug 30, 2022

ChrisDenton commented Aug 30, 2022

bors commented Aug 30, 2022

Fix UB from misalignment and provenance widening in `std::sys::windows` #101171

Fix UB from misalignment and provenance widening in `std::sys::windows` #101171

thomcc commented Aug 29, 2022 •

edited

Loading

ChrisDenton commented Aug 30, 2022 •

edited

Loading