hannobraun / inotify-rs

Idiomatic inotify wrapper for the Rust programming language
ISC License
254 stars 64 forks source link

Consider addressing buffer alignment in a more robust way #174

Closed hannobraun closed 2 years ago

hannobraun commented 3 years ago

See #155 and and #171 (specifically https://github.com/hannobraun/inotify-rs/pull/171#pullrequestreview-655632788) for context.

talklittle commented 2 years ago

Would it be acceptable to NOT align the byte buffer? That would save a lot of potential new code (for example, custom structs wrapping the buffer and enforcing alignment) and maybe not break as easily.

With this proposed approach, the key would be std::ptr::read_unaligned which lets us read the inotify_event data from the unaligned buffer.

Contrast with the * pointer dereference:

https://github.com/hannobraun/inotify-rs/blob/b68d62fd56ecedcefce1827e6cf2d62a08e0aa91/src/events.rs#L172

Which is not allowed (UB) according to the pointer docs:

Raw pointers can be unaligned or null. However, when a raw pointer is dereferenced (using the * operator), it must be non-null and aligned.

Since that * dereference seems to be the problematic part, maybe we can just fix that line instead of enforcing buffer alignment throughout the library? I'm assuming the performance cost reading a struct from unaligned memory, and adding one copy operation, would be negligible.

Here is what this approach looks like: https://github.com/hannobraun/inotify-rs/compare/master...talklittle:inotify-rs:allow-unaligned-buffers?expand=1
Note that it largely reverts #171. Also verified that it stops Miri from reporting the UB alignment error.

I can create a pull request if it makes sense.

hannobraun commented 2 years ago

Thank you for looking into this, @talklittle! Based on the documentation you linked and the Miri result, this looks like a viable way to do it. A pull request would be appreciated!