Ensure an Rc isn't freed while running its own destructor. #12047
Conversation
|
cc @alexcrichton: I think this weak solution works, and is certainly nicer than using a sentinel value for the strong count. |
|
this looks like a nice, contained, fix for this issue. |
|
That unit test doesn't look like its guaranteed to fail in the event of a double free. Certain events can cause other memory to get pushed in to position (e.g. in my tests the crashes happened after removing a println), and Valgrind will not necessarily kill a program for double freeing (though it will annotate this fact sometimes.) |
|
That test crashed reliably for me, but yeah, it is system dependent what happens on a double free. Do you happen to have a better/more reliable test? |
| @@ -80,7 +80,27 @@ impl<T> Drop for Rc<T> { | |||
| if self.ptr != 0 as *mut RcBox<T> { | |||
| (*self.ptr).strong -= 1; | |||
| if (*self.ptr).strong == 0 { | |||
There was a problem hiding this comment.
This could be a check against 1, with the decrement moved after the branch.
There was a problem hiding this comment.
I'd be worried about moving the decrements because you want to be sure that during destruction you can't upgrade a weak pointer (you're destroying the contents at that time).
There was a problem hiding this comment.
True, I guess that could be a problem. The libstdc++ implementation considers all of the strong pointers to have an initial weak count then then decreases it when the last strong pointer is freed.
|
The libstdc++ way is to set the initial weak count to 1 and consider it the weak count for all of the strong pointers. It then reduces the weak count after destroying the inner object. This actually removes a branch from the |
|
Removing my r+, @thestinger has some more suggestions on how to fix this. |
A weak pointer inside itself will have its destructor run when the last strong pointer to that data disappears, so we need to make sure that the Weak and Rc destructors don't duplicate work (i.e. freeing). By making the Rcs effectively take a weak pointer, we ensure that no Weak destructor will free the pointer while still ensuring that Weak pointers can't be upgraded to strong ones as the destructors run. This approach of starting weak at 1 is what libstdc++ does. Fixes rust-lang#12046.
|
@huonw A global counter for RC creations/frees which exists only under #cfg(test) would probably do it. Alternatively a low level memory allocator which is active at the test configuration which tracks for double-frees (that would be more widely helpful in general.) While the latter would be more widely helpful, this is somewhat of a show-stopping bug for a lot of designs right now. |
|
Building the tests with address sanitizer will catch a double-free. |
A weak pointer inside itself will have its destructor run when the last strong pointer to that data disappears, so we need to make sure that the Weak and Rc destructors don't duplicate work (i.e. freeing). By making the Rcs effectively take a weak pointer, we ensure that no Weak destructor will free the pointer while still ensuring that Weak pointers can't be upgraded to strong ones as the destructors run. This approach of starting weak at 1 is what libstdc++ does. Fixes #12046.
…s-with-brackets, r=Jarcho New Lint: empty_enum_variants_with_brackets This PR: - adds a new early pass lint that checks for enum variants with no fields that were defined using brackets. **Category: Restriction** - adds relevant UI tests for the new lint. Closes rust-lang#12007 ``` changelog: New lint: [`empty_enum_variants_with_brackets`] ```
A weak pointer inside itself will have its destructor run when the last
strong pointer to that data disappears, so we need to make sure that the
Weak and Rc destructors don't duplicate work (i.e. freeing).
By making the Rcs effectively take a weak pointer, we ensure that no
Weak destructor will free the pointer while still ensuring that Weak
pointers can't be upgraded to strong ones as the destructors run.
This approach of starting weak at 1 is what libstdc++ does.
Fixes #12046.