Fix some memory leaks on panicking destructors #27
Conversation
Use catch_unwind instead of should_panic to be more explicit about what shoud panic. More descriptive names. Use nontrivial owner type.
|
Having both owner and dependent impl panic, not sure how to test that without running into terminate. |
|
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. |
| dealloc(self.joined_void_ptr.as_ptr(), layout); | ||
| // Dropping owner | ||
| // and deallocating | ||
| // due to _guard at end of scope. |
There was a problem hiding this comment.
This function is suddenly much nicer, and less leaky. Awesome to see OwnerAndCellDropGuard get more mileage.
| @@ -149,16 +156,31 @@ impl<Owner, Dependent> OwnerAndCellDropGuard<Owner, Dependent> { | |||
|
|
|||
| impl<Owner, Dependent> Drop for OwnerAndCellDropGuard<Owner, Dependent> { | |||
| fn drop(&mut self) { | |||
| struct DeallocGuard { | |||
There was a problem hiding this comment.
I heard you like drop guards so I put a drop guard inside your drop guard :D
|
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. |
|
@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. |
|
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. |
|
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. |
Closes #26