Skip to content

Comments

Fix UB in windows::start_read#602

Closed
encounter wants to merge 1 commit intonotify-rs:mainfrom
encounter:windows-ub
Closed

Fix UB in windows::start_read#602
encounter wants to merge 1 commit intonotify-rs:mainfrom
encounter:windows-ub

Conversation

@encounter
Copy link
Contributor

@encounter encounter commented Jun 4, 2024

The primary issue is that mem::transmute from isize to Box<_> (without first casting to *mut _) is undefined behavior.

Rust Playground example (check with Tools -> Miri)

On Rust v1.78.0+, this ends up crashing with STATUS_ILLEGAL_INSTRUCTION when ReadDirectoryChangesW fails and this branch is hit in release mode. One easy way to hit this branch is to attempt to create a notifier for \\wsl.localhost\... or similar.

While this could be fixed by simply adding as *mut ReadDirectoryRequest, this cleans up the overall unsafe logic to be more readable and idiomatic Rust.

The primary issue is that `mem::transmute`
from `isize` to `Box<_>` (without first
casting to `*mut _`) is undefined behavior.

On Rust v1.78.0+, this ends up crashing with
`STATUS_ILLEGAL_INSTRUCTION` when
`ReadDirectoryChangesW` fails and this
branch is hit in release mode.

While this could be fixed by simply adding
`as *mut ReadDirectoryRequest`, this cleans
up the overall unsafe logic to be more
readable and idiomatic Rust.
@0xpr03
Copy link
Member

0xpr03 commented Jun 24, 2024

Ugh looks like freebsd needs a CI fix. Let's see when I can actually fix this all up.

@0xpr03
Copy link
Member

0xpr03 commented Jun 26, 2024

Hm I would have liked #604 to be a separate commit. This way it is harder to diff. Whole thing is already hairy enough.

@0xpr03
Copy link
Member

0xpr03 commented Jun 26, 2024

I will merge the other one first, that will also credit the author and merges the part we know to be working. Please do me the favor and rebase on top of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants