search

Voultapher / self_cell

Safe-to-use proc-macro-free self-referential structs in stable Rust.
Apache License 2.0
242 stars 15 forks source link

Fix some memory leaks on panicking destructors #27

Closed steffahn closed 3 years ago

steffahn commented 3 years ago

Closes #26

Voultapher commented 3 years ago

Having both owner and dependent impl panic, not sure how to test that without running into terminate.

steffahn commented 3 years ago

If there's two panics then an abort is expected.

This could be tested by having the owner and the dependent implement drop, the dependent panicking, the owner's panic with some side-effect. Then catch the panic, and test for the side-effect.

I don't really think such a test is strictly necessary, in particular I don't think it would need to be part of this PR.

Then you'll know that both destructors (owner and dependent) were run. Since owner was run during unwinding, if it were to panic an abort would've automatically resulted.

steffahn commented 3 years ago

Actually, I'm only now reading your modified tests. By making the owner a string, you're already testing whether the owner is dropped if dependent panics. If it wasn't, there would be a leak. Since it is dropped, we can be sure that if it's destructor panicked as well, an abort would be triggered.

Voultapher commented 3 years ago

@steffahn are we good for v0.9.3 or do you want to do some more research? IMO a day or two are not critical here, but notifying users of an important update, I'd prefer doing once instead of twice or more.

steffahn commented 3 years ago

I'm not planning on doing anything else today, and as of now I'm not aware of any remaining unsoundness. I don't know yet if I'll take another look around the source tomorrow or not.

Voultapher commented 3 years ago

Ok, thanks for taking a look. I want this project to be easy and safe-to-use, without caveats. So I'm always happy to see soundness issues found and addressed. Better now than later. I'm quite busy right now myself. So I'll release a new version hopefully Friday and open a PR for bracket.