SpectralSequences / sseq

The root repository for the SpectralSequences project.
Apache License 2.0
25 stars 10 forks source link

Fix rare UB in `once` crate when panicking #125

Open JoeyBF opened 1 year ago

JoeyBF commented 1 year ago

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 commented 1 year ago

I'm not convinced it is an issue with panicking per se; I think this code path is rarely (never?) hit outside of panicking. I think it would be better to look into the underlying issue more deeply.

JoeyBF commented 1 year ago

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 commented 1 year ago

I mean dropping when ooo is non-empty

hoodmane commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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