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 rare UB in once crate when panicking #125

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JoeyBF
Copy link
Collaborator

@JoeyBF JoeyBF commented Mar 28, 2023

We're hitting some UB, in the form of segfaults and illegal instructions, and I tracked it down to that unsafe block. Some invariants aren't satisfied if we are panicking, so we just leak all the memory in that case

@dalcde
Copy link
Contributor

dalcde commented Apr 3, 2023 via email

@JoeyBF
Copy link
Collaborator Author

JoeyBF commented Apr 3, 2023

By this code path you mean when the lock is poisoned? The thing is that I agree with the justifications for the unsafe block, I'm not sure what kind of UB is even possible except if we are somehow already in a partially deallocated state (which shouldn't be possible either!)

@dalcde
Copy link
Contributor

dalcde commented Apr 3, 2023 via email

@hoodmane
Copy link
Contributor

@JoeyBF maybe we should just merge this for now? Since nobody seems to have the energy to look into this more carefully. Also, do we have any automation that runs miri? Presumably if we have tests for OnceVec that inject panics the runtime checks can tell us whether were are doing okay...

@JoeyBF
Copy link
Collaborator Author

JoeyBF commented Aug 14, 2024

Strangely, adding a test that just pushes out of order then panics doesn't set off miri without this PR, and only reports a memory leak with it (as expected). Maybe merge this and open an issue to look into it more closely?

@hoodmane
Copy link
Contributor

Ideally it would be good to make a test that miri flags as undefined behavior without this change as part of this pr. But has the problem recurred? If you can't reproduce it and it doesn't happen anymore then maybe the change is not needed.

@JoeyBF
Copy link
Collaborator Author

JoeyBF commented Aug 14, 2024

I believe it's still there. I'm not sure what I was working on at the time but when I find it again I'll make a minimal example and add it as a test. In the meantime I guess we can just leave this PR as-is

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.

3 participants