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.

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

Align input buffers before reading or writing inotify events #171

Merged
merged 2 commits into from
May 10, 2021

Conversation

FenrirWolf
Copy link
Contributor

Attempts to solve #155

This passes the current tests and eliminates the miri warning about unaligned references that currently triggers here, but I'm not certain if it's fully robust. The alignment code works by splitting away any unaligned portion of the input buffer, and so I'm not sure if the byte-tracking code in Stream and other places needs to be updated to reflect that.

@hannobraun
Copy link
Owner

Thank you for the contribution, @FenrirWolf!

This looks good. My only concern is whether other parts of the library need to be updated, as you mention. I will take a closer look next week.

Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

I've checked the rest of the code, and I think everything's fine. The stream code uses the same core primitives that this PR updates (Event::from_buffer, read_into_buffer), so I think it's fully covered by this fix.

What I don't like about this fix is how subtle it is. The alignment needs to be considered in multiple places, and while I think this PR gets it right, future changes might mess it up again. Still, I think this PR fixes the unsoundness, so I'm going to merge it.

I think it would be better, if we could somehow use a custom buffer type to enforce the alignment at the type level. I'm not sure how practical that is, and I don't have the bandwidth to look into that. If anyone wants to look into that approach, that would be very welcome.

Thanks again, @FenrirWolf!

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