Title: [215368] trunk/Source/_javascript_Core
- Revision
- 215368
- Author
- sbar...@apple.com
- Date
- 2017-04-14 11:09:35 -0700 (Fri, 14 Apr 2017)
Log Message
WebAssembly: There is a short window of time where a CodeBlock could be destroyed before all of its async compilation callbacks are called
https://bugs.webkit.org/show_bug.cgi?id=170641
Reviewed by Keith Miller.
There is an unlikely race when a CodeBlock compilation fails,
the module compiles a new CodeBlock for that memory mode, all while
the CodeBlock is notifying its callbacks that it has finished.
There is a chance that the Module could deref its failed CodeBlock
at that point, destroying it, before the callbacks were able to
grab a Ref to the CodeBlock. This patch fixes the race by having the
callbacks ref the CodeBlock.
This patch also has the Plan clear out all of its callbacks
once it gets completed. This adds an extra defense to anybody
that grabs refs to the Plan in the callback.
* wasm/WasmCodeBlock.cpp:
(JSC::Wasm::CodeBlock::CodeBlock):
(JSC::Wasm::CodeBlock::compileAsync):
* wasm/WasmPlan.cpp:
(JSC::Wasm::Plan::complete):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (215367 => 215368)
--- trunk/Source/_javascript_Core/ChangeLog 2017-04-14 17:57:40 UTC (rev 215367)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-04-14 18:09:35 UTC (rev 215368)
@@ -1,3 +1,28 @@
+2017-04-14 Saam Barati <sbar...@apple.com>
+
+ WebAssembly: There is a short window of time where a CodeBlock could be destroyed before all of its async compilation callbacks are called
+ https://bugs.webkit.org/show_bug.cgi?id=170641
+
+ Reviewed by Keith Miller.
+
+ There is an unlikely race when a CodeBlock compilation fails,
+ the module compiles a new CodeBlock for that memory mode, all while
+ the CodeBlock is notifying its callbacks that it has finished.
+ There is a chance that the Module could deref its failed CodeBlock
+ at that point, destroying it, before the callbacks were able to
+ grab a Ref to the CodeBlock. This patch fixes the race by having the
+ callbacks ref the CodeBlock.
+
+ This patch also has the Plan clear out all of its callbacks
+ once it gets completed. This adds an extra defense to anybody
+ that grabs refs to the Plan in the callback.
+
+ * wasm/WasmCodeBlock.cpp:
+ (JSC::Wasm::CodeBlock::CodeBlock):
+ (JSC::Wasm::CodeBlock::compileAsync):
+ * wasm/WasmPlan.cpp:
+ (JSC::Wasm::Plan::complete):
+
2017-04-13 Filip Pizlo <fpi...@apple.com>
Air::RegLiveness should be constraint-based
Modified: trunk/Source/_javascript_Core/wasm/WasmCodeBlock.cpp (215367 => 215368)
--- trunk/Source/_javascript_Core/wasm/WasmCodeBlock.cpp 2017-04-14 17:57:40 UTC (rev 215367)
+++ trunk/Source/_javascript_Core/wasm/WasmCodeBlock.cpp 2017-04-14 18:09:35 UTC (rev 215368)
@@ -39,11 +39,8 @@
: m_calleeCount(moduleInformation.internalFunctionCount())
, m_mode(mode)
{
- m_plan = adoptRef(*new Plan(vm, makeRef(moduleInformation), Plan::FullCompile, createSharedTask<Plan::CallbackType>([this] (VM&, Plan&) {
- // It's safe to use |this| here because our Module owns us, and a Module
- // can't be destroyed until it has finished compiling its various Wasm bits.
- // The JS API ensures this invariant by keeping alive the module as it has
- // a pending promise.
+ RefPtr<CodeBlock> protectedThis = this;
+ m_plan = adoptRef(*new Plan(vm, makeRef(moduleInformation), Plan::FullCompile, createSharedTask<Plan::CallbackType>([this, protectedThis = WTFMove(protectedThis)] (VM&, Plan&) {
auto locker = holdLock(m_lock);
if (m_plan->failed()) {
m_errorMessage = m_plan->errorMessage();
@@ -94,9 +91,10 @@
}
if (plan) {
- // Our lifetime is guaranteed by the JS code by having it
- // keep alive the JS objects that ensure our lifetime.
- plan->addCompletionTask(vm, createSharedTask<Plan::CallbackType>([this, task = WTFMove(task)] (VM& vm, Plan&) {
+ // We don't need to keep a RefPtr on the Plan because the worklist will keep
+ // a RefPtr on the Plan until the plan finishes notifying all of its callbacks.
+ RefPtr<CodeBlock> protectedThis = this;
+ plan->addCompletionTask(vm, createSharedTask<Plan::CallbackType>([this, task = WTFMove(task), protectedThis = WTFMove(protectedThis)] (VM& vm, Plan&) {
task->run(vm, makeRef(*this));
}));
} else
Modified: trunk/Source/_javascript_Core/wasm/WasmPlan.cpp (215367 => 215368)
--- trunk/Source/_javascript_Core/wasm/WasmPlan.cpp 2017-04-14 17:57:40 UTC (rev 215367)
+++ trunk/Source/_javascript_Core/wasm/WasmPlan.cpp 2017-04-14 18:09:35 UTC (rev 215368)
@@ -325,6 +325,7 @@
moveToState(State::Completed);
for (auto& task : m_completionTasks)
task.second->run(*task.first, *this);
+ m_completionTasks.clear();
m_completed.notifyAll();
}
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
