Fix use-after-free with closures in JS bindings #1385
Merged
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
This commit fixes an erroneous use-after-free which can happen in erroneous situations in JS. It's intended that if you invoke a closure after its environment has been destroyed that you'll immediately get an error from Rust saying so. The JS binding generation for mutable closures, however, accidentally did not protect against this. Each closure has an internal reference count which is incremented while being invoked and decremented when the invocation finishes and also when the `Closure` in Rust is dropped. That means there's two branches where the reference count reaches zero and the internal pointer stored in JS needs to be set to zero. Only one, however, actually set the pointer to zero! This means that if a closure was destroyed while it was being invoked it would not correctly set its internal pointer to zero. A further invocation of the closure would then pass as seemingly valid pointer into Rust, causing a use-after-free. A test isn't included here specifically for this because our CI has started failing left-and-right over this test, so this commit will hopefully just make our CI green!
Co-Authored-By: alexcrichton <alex@alexcrichton.com>
Co-Authored-By: alexcrichton <alex@alexcrichton.com>
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.
This commit fixes an erroneous use-after-free which can happen in
erroneous situations in JS. It's intended that if you invoke a closure
after its environment has been destroyed that you'll immediately get an
error from Rust saying so. The JS binding generation for mutable
closures, however, accidentally did not protect against this.
Each closure has an internal reference count which is incremented while
being invoked and decremented when the invocation finishes and also when
the
Closurein Rust is dropped. That means there's two branches wherethe reference count reaches zero and the internal pointer stored in JS
needs to be set to zero. Only one, however, actually set the pointer to
zero!
This means that if a closure was destroyed while it was being invoked it
would not correctly set its internal pointer to zero. A further
invocation of the closure would then pass as seemingly valid pointer
into Rust, causing a use-after-free.
A test isn't included here specifically for this because our CI has
started failing left-and-right over this test, so this commit will
hopefully just make our CI green!