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

Fix unsound Clone and undefined behavior in self-referential Buf variants #28

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

sebschrader
Copy link
Contributor

@sebschrader sebschrader commented Dec 12, 2024

TemplateBuf and ByteTemplate are implemented as self-referential structs, but their current implementation is unsound and contains undefined behavior.

According to my analysis the following issues exists:

  • The derived Clone implementations by the compiler are unsound, as they copy the static reference by value, causing all kinds of memory issues (panics due to memory issues caused my investigation.)
  • As field order of structs determines the drop order, the underlying String or Vec<u8> will be dropped before the reference, this UB according to my understanding, as there will be dangling reference.
  • String and Vec<u8> are implemented using core::ptr::Unique. As such all references to such a type, are invalidated, if such a type is moved. See Self-referential types for fun and profit by @hniksic for details

This PR tries to address these issues.

  • I've included a small test to reproduce the first issue.
  • The fixes for the first two points are rather self-explanatory.
  • For the last issue I've implemented the aliasable::String and aliasable::Bytes types instead of pulling in an extra dependency like the aliasable crate.
  • I've checked my changes, by running the tests with miri. It passes but requires tree-borrows (MIRIFLAGS="-Zmiri-tree-borrows").

References &T to a type T are invalidated when T is moved, the same
applies to core::ptr::Unique, which is used internally by Vec. This
makes construction of a self-referential types such as TemplateBuf or
ByteTemplateBuf a safety hazard.

Introduce two helper types, aliasable::Bytes and aliasable::String,
that store the raw parts of the original vectors, such that shared
references can be obtained from the raw (non-unique) pointers.

The implementation is inspired by the aliasable crate, which is was
created for that very purpose.

See the following blog post by Hrvoje Nikšić for more details:
https://morestina.net/blog/1868/self-referential-types-for-fun-and-profit
@de-vri-es
Copy link
Contributor

Eep, thank you for reporting this and opening a PR! I can't believe I added that Clone implementation 😱

Maybe we can can use Pin<String> / Pin<Vec<u8>> instead of more unsafe code with aliasable. However, I think we should push the fix for the Clone impl and the destructor order asap.

The last part of the fix is probably less likely to cause actual runtime problems. I still think they are super important to fix, but I would like to see if we can do it while reducing unsafe code instead of increasing it.

I will yank the unsound versions once fixes are released!

@de-vri-es
Copy link
Contributor

I merged the first three commits and released them as 0.3.6. I think that gives a bit of breathing room to look at the remaining issue of references to heap allocations being invalidated when String and Vec<u8> are moved.

@de-vri-es

This comment was marked as resolved.

@de-vri-es

This comment was marked as outdated.

@de-vri-es
Copy link
Contributor

I've opened #30 with a different approach to solving the aliasing problem. Rather than swapping Vec with a custom implementation, that approach wraps the Template that holds the references in MaybeUninit. More details available at that PR.

The main motivation for the alternative approach is that it's less extra unsafe code compared to aliasable::Byte/String. It also doesn't need extra miri flags to pass the tests.

@de-vri-es
Copy link
Contributor

de-vri-es commented Dec 13, 2024

Merging without the last commit.

Thanks again! This was a really important fix 😬

Released v0.3.7 with all the fixes and yanked all the unsound version.

@de-vri-es de-vri-es merged commit 200c432 into fizyr:main Dec 13, 2024
1 check passed
@sebschrader
Copy link
Contributor Author

I've opened #30 with a different approach to solving the aliasing problem. Rather than swapping Vec with a custom implementation, that approach wraps the Template that holds the references in MaybeUninit. More details available at that PR.

The main motivation for the alternative approach is that it's less extra unsafe code compared to aliasable::Byte/String. It also doesn't need extra miri flags to pass the tests.

I'm not familiar enough with aliasing guarantees/details around MaybeUnit to judge this approach.

Thank you for your quick action and your work in general too!

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