Skip to content

Comments

Fix unaligned access to FILE_NOTIFY_INFORMATION#647

Merged
dfaust merged 1 commit intonotify-rs:mainfrom
ruihe774:misalign
Nov 24, 2024
Merged

Fix unaligned access to FILE_NOTIFY_INFORMATION#647
dfaust merged 1 commit intonotify-rs:mainfrom
ruihe774:misalign

Conversation

@ruihe774
Copy link
Contributor

@ruihe774 ruihe774 commented Oct 21, 2024

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.

@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
Contributor 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.

@dfaust dfaust merged commit 0524cb3 into notify-rs:main Nov 24, 2024
@samuelcolvin samuelcolvin mentioned this pull request Jan 10, 2025
Merged
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