Fix memory leak if C++ catches a Rust panic and discards it #67711
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
Amanieu
force-pushed
the
fix_unwind_leak
branch
from
4fd349c to
fdce28b
Compare
There was a problem hiding this comment.
As far as I can tell, this is correct and a good fix -- I'm not sure if I feel entirely comfortable approving this though, so I'd prefer if e.g. @alexcrichton takes a look as well. (Not sure who else has the requisite expertise).
(We'll also want to drop the pipelines change before r+ of course)
| @@ -87,10 +87,8 @@ pub fn payload() -> *mut u8 { | |||
| } | |||
|
|
|||
| pub unsafe fn cleanup(ptr: *mut u8) -> Box<dyn Any + Send> { | |||
| let my_ep = ptr as *mut Exception; | |||
| let cause = (*my_ep).cause.take(); | |||
| uw::_Unwind_DeleteException(ptr as *mut _); | |||
There was a problem hiding this comment.
Who calls DeleteException on the exception? There's not a drop impl on Exception or _Unwind_Exception AFAICT.
There was a problem hiding this comment.
All _Unwind_DeleteException does it invoke the call the exception_cleanup callback in _Unwind_Exception. Since the exception is our own, we can just free the Box directly.
There was a problem hiding this comment.
I'm flagging this as temporarily unresolved to leave a comment here too since this was a question I had. Could a comment be left indicating that we're explicitly not calling _Unwind_DeleteException? Also, do you have a link to the source that it only calls the internal callback?
There was a problem hiding this comment.
The documentation for _Unwind_DeleteException explicitly says that this function is for foreign exception that the runtime does not know how to free.
There was a problem hiding this comment.
Oh sure sorry I'm not questioning the validity of this deletion, just un-resolving it so my request for a comment in the code didn't show up as collapsed.
|
@bors rollup=never |
|
It seems that MSVC basically requires exception objects to be copyable, otherwise it may end up double-freeing an exception object if it is captured using |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Amanieu
force-pushed
the
fix_unwind_leak
branch
6 times, most recently
from
e5c3aae to
3f0c097
Compare
|
Should be good to merge now. |
alexcrichton
reviewed
Jan 6, 2020
| @@ -52,22 +52,32 @@ pub fn payload() -> *mut u8 { | |||
| ptr::null_mut() | |||
| } | |||
|
|
|||
| struct Exception { | |||
| data: Option<Box<dyn Any + Send>>, | |||
There was a problem hiding this comment.
Reading over this it seemed odd that the Option went away below but seemed to be added here, but this is for emcc.rs so I'm not reviewing it too too closely.
There was a problem hiding this comment.
I added an option here because the C++ runtime will invoke the destructor during __cxa_end_catch. We need to ensure that the exception object contains None at that point to avoid a double-free.
alexcrichton
reviewed
Jan 6, 2020
| let local_ptr = catchpad.bitcast(local_ptr, bx.type_ptr_to(i64_2)); | ||
| catchpad.store(payload, local_ptr, i64_align); | ||
|
|
||
| // Clear the first word of the exception so avoid double-dropping it. |
There was a problem hiding this comment.
To make sure I understand what's going on here, we catch the exception by reference now which means that the system unwinder will always invoke the destructor function listed. For us that destructor function will skip its actual work since the first slot will be set to zero (as configured here).
If that's true, how come we're catching by reference? Why not continue to catch by-value and just skip the destructor for Rust exceptions?
There was a problem hiding this comment.
If we catch by value then C++ will give the catch a copy (not a move) of the exception object. It will still call the exception destructor after the catch ends. Catching by reference avoids this copy (which won't work anyways since our copy constructor panics).
There was a problem hiding this comment.
Ah ok, makes sense. Does C++ never catch by value or in any way try to invoke the copy constructor?
Also, can you inline these words into the code comments as well?
alexcrichton
reviewed
Jan 6, 2020
|
Also to clarify, from the current state of this PR, this is just fixing MSVC/emscripten, right? It looks like the changes for Linux/etc mean that we already don't leak there? (trying to make sure I understand) |
That's correct, the |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Amanieu
force-pushed
the
fix_unwind_leak
branch
from
ca1bcc2 to
5b41d99
Compare
|
Fixed formatting |
|
@bors: r+ |
|
📌 Commit 5b41d99 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jan 8, 2020
Fix memory leak if C++ catches a Rust panic and discards it If C++ catches a Rust panic using `catch (...)` and then chooses not to rethrow it, the `Box<dyn Any>` in the exception may be leaked. This PR fixes this by adding the necessary destructors to the exception object. r? @Mark-Simulacrum
|
⌛ Testing commit 4f163af with merge b49bad00f406048613bc23565e59c7bc70f9a549... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
|
@bors r=alexcrichton |
|
📌 Commit 35d72fa376a628c729aac86e6457c41376c6d7ad has been approved by |
bors
added
S-waiting-on-bors
Amanieu
force-pushed
the
fix_unwind_leak
branch
from
35d72fa to
cfde360
Compare
|
@bors r=alexcrichton |
|
📌 Commit cfde360f05686ae496c146edc036c72bc8e4a826 has been approved by |
|
⌛ Testing commit cfde360f05686ae496c146edc036c72bc8e4a826 with merge cbb700b656772414ec2269a7de31c1cf80926b8f... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
S-waiting-on-review
Amanieu
force-pushed
the
fix_unwind_leak
branch
from
cfde360 to
25519e5
Compare
|
@bors r=alexcrichton |
|
📌 Commit 25519e5 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jan 14, 2020
Fix memory leak if C++ catches a Rust panic and discards it If C++ catches a Rust panic using `catch (...)` and then chooses not to rethrow it, the `Box<dyn Any>` in the exception may be leaked. This PR fixes this by adding the necessary destructors to the exception object. r? @Mark-Simulacrum
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If C++ catches a Rust panic using
catch (...)and then chooses not to rethrow it, theBox<dyn Any>in the exception may be leaked. This PR fixes this by adding the necessary destructors to the exception object.r? @Mark-Simulacrum