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

Pinning bumpalo::boxed::Box is unsound #186

Open
Lej77 opened this issue Nov 8, 2022 · 3 comments · May be fixed by #228
Open

Pinning bumpalo::boxed::Box is unsound #186

Lej77 opened this issue Nov 8, 2022 · 3 comments · May be fixed by #228

Comments

@Lej77
Copy link

Lej77 commented Nov 8, 2022

A pinned bumpalo::boxed::Box might not uphold Pin's Drop guarantee. Specifically if a pinned bumpalo::boxed::Box is forgotten then the memory backing it might still be freed. (This differs from how std::boxed::Box is implemented.)

To soundly give out pinned references from a Bump one could keep a counter of the number of pinned allocations that still need to be freed and if that counter isn't zero when Bump is dropped then it could leak all of its memory. However decrementing this counter when a Box is dropped would require bumpalo::boxed::Box to store a reference to Bump which would double the Box's size (two pointers).

@fitzgen
Copy link
Owner

fitzgen commented Nov 8, 2022

Probably should just remove the Pin support in that case.

@Lej77
Copy link
Author

Lej77 commented Nov 8, 2022

That would be a reasonable solution!

The best option for keeping Pin support would probably be to add a new box type that can be pinned (bumpalo::boxed::PinnedBox?) which would store an extra reference to Bump like I suggested before. Then only use cases that actually need Pin support would need to pay the extra cost. Well, except Bump would still need to keep an extra counter (Cell<usize>) for pinned allocations which would need to be checked before freeing any memory. I don't think that would impact performance much so it could be a reasonable compromise. The extra box type could maybe be generated with a macro to avoid duplicating too much code.

@khoover
Copy link

khoover commented May 21, 2023

This combined with #204 makes me think we want two box types; a ThinBox that's still the single word, but doesn't hold on to the Bump pointer, and a FatBox that does and can be Pinned or Cloned.

@cynecx cynecx linked a pull request Feb 4, 2024 that will close this issue
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 a pull request may close this issue.

3 participants