Skip to content
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

Jump to bottom

Fix unaligned access to FILE_NOTIFY_INFORMATION #647

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix unaligned access to FILE_NOTIFY_INFORMATION #647

wants to merge 1 commit into from

Conversation

ruihe774
Copy link

In some environments (e.g. wine), FILE_NOTIFY_INFORMATION is packed placed and is unaligned. Reading from unaligned FILE_NOTIFY_INFORMATION is a UB and causes panics in debug builds. This PR workarounds this issue by using ptr::read_unaligned.

dfaust
dfaust requested changes Oct 23, 2024
View reviewed changes
Copy link
Member

@dfaust dfaust left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good to me, but please add some more comments. This is an edge case that probably nobody working on this code will have on their radar.

notify/src/windows.rs Outdated Show resolved Hide resolved
@ruihe774 ruihe774 requested a review from dfaust October 25, 2024 03:06
@dfaust
Copy link
Member

dfaust commented Oct 25, 2024

Thanks. Unfortunately offset_of has only been stabilized in Rust 1.77 and our MSRV is 1.72.
My suggestion: I'm going to branch off and release a new version today and then merge this PR into main afterwards.

CC: @0xpr03 @JohnTitor

@ruihe774
Copy link
Author

ruihe774 commented Oct 25, 2024 •
edited
Loading

Thanks. Unfortunately offset_of has only been stabilized in Rust 1.77 and our MSRV is 1.72. My suggestion: I'm going to branch off and release a new version today and then merge this PR into main afterwards.

CC: @0xpr03 @JohnTitor

We can hard code the offset giving FILE_NOTIFY_INFORMATION has a stable ABI; somewhat dirty, though.

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

@dfaust dfaust Awaiting requested review from dfaust

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ruihe774 @dfaust