From 392384e28a1ea23a297c4bc5ad85e2853cfa3c1a Mon Sep 17 00:00:00 2001 From: "fpizlo@apple.com" Date: Sun, 15 May 2016 23:08:21 +0000 Subject: [PATCH] DFG::Plan shouldn't read from its VM once it's been cancelled https://bugs.webkit.org/show_bug.cgi?id=157726 Reviewed by Saam Barati. Plan::vm was a reference, not a pointer, and so wasn't nulled by Plan::cancel(). So, a cancelled plan may have a dangling pointer to a VM: we could delete the VM after cancelling the plan. Prior to http://trac.webkit.org/changeset/200705, this was probably fine because nobody would read Plan::vm if the plan was cancelled. But r200705 changed that. It was a hard regression to spot because usually a cancelled plan will still refer to a valid VM. This change fixes the regression and makes it a lot easier to spot the regression in the future. Plan::vm is now a pointer and we null it in Plan::cancel(). Now if you make this mistake, you will get a crash anytime the Plan is cancelled, not just anytime the plan is cancelled and the VM gets deleted. Also, it's now very clear what to do when you want to use Plan::vm on the cancel path: you can null-check vm; if it's null, assume the worst. Because we null the VM of a cancelled plan, we cannot have Safepoint::vm() return the plan's VM anymore. That's because when we cancel a plan that is at a safepoint, we use the safepoint's VM to determine whether this is one of our safepoints *after* the plan is already cancelled. So, Safepoint now has its own copy of m_vm, and that copy gets nulled when the Safepoint is cancelled. The Safepoint's m_vm will be nulled moments after Plan's vm gets nulled (see Worklist::removeDeadPlans(), which has a cancel path for Plans in one loop and a cancel path for Safepoints in the loop after it). * dfg/DFGJITFinalizer.cpp: (JSC::DFG::JITFinalizer::finalizeCommon): * dfg/DFGPlan.cpp: (JSC::DFG::Plan::Plan): (JSC::DFG::Plan::computeCompileTimes): (JSC::DFG::Plan::reportCompileTimes): (JSC::DFG::Plan::compileInThreadImpl): (JSC::DFG::Plan::reallyAdd): (JSC::DFG::Plan::notifyCompiling): (JSC::DFG::Plan::finalizeWithoutNotifyingCallback): (JSC::DFG::Plan::cancel): * dfg/DFGPlan.h: (JSC::DFG::Plan::canTierUpAndOSREnter): * dfg/DFGSafepoint.cpp: (JSC::DFG::Safepoint::cancel): (JSC::DFG::Safepoint::vm): * dfg/DFGSafepoint.h: * dfg/DFGWorklist.cpp: (JSC::DFG::Worklist::isActiveForVM): (JSC::DFG::Worklist::waitUntilAllPlansForVMAreReady): (JSC::DFG::Worklist::removeAllReadyPlansForVM): (JSC::DFG::Worklist::rememberCodeBlocks): (JSC::DFG::Worklist::visitWeakReferences): (JSC::DFG::Worklist::removeDeadPlans): (JSC::DFG::Worklist::runThread): * ftl/FTLJITFinalizer.cpp: (JSC::FTL::JITFinalizer::finalizeFunction): git-svn-id: https://svn.webkit.org/repository/webkit/trunk@200933 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/JavaScriptCore/ChangeLog | 57 +++++++++++++++++++++++++++ Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp | 2 +- Source/JavaScriptCore/dfg/DFGPlan.cpp | 17 ++++---- Source/JavaScriptCore/dfg/DFGPlan.h | 7 +++- Source/JavaScriptCore/dfg/DFGSafepoint.cpp | 10 +++-- Source/JavaScriptCore/dfg/DFGSafepoint.h | 5 ++- Source/JavaScriptCore/dfg/DFGWorklist.cpp | 24 +++++------ Source/JavaScriptCore/ftl/FTLJITFinalizer.cpp | 2 +- 8 files changed, 94 insertions(+), 30 deletions(-) diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index 5beaab2..0079cb8 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,60 @@ +2016-05-15 Filip Pizlo + + DFG::Plan shouldn't read from its VM once it's been cancelled + https://bugs.webkit.org/show_bug.cgi?id=157726 + + Reviewed by Saam Barati. + + Plan::vm was a reference, not a pointer, and so wasn't nulled by Plan::cancel(). So, a + cancelled plan may have a dangling pointer to a VM: we could delete the VM after cancelling + the plan. + + Prior to http://trac.webkit.org/changeset/200705, this was probably fine because nobody + would read Plan::vm if the plan was cancelled. But r200705 changed that. It was a hard + regression to spot because usually a cancelled plan will still refer to a valid VM. + + This change fixes the regression and makes it a lot easier to spot the regression in the + future. Plan::vm is now a pointer and we null it in Plan::cancel(). Now if you make this + mistake, you will get a crash anytime the Plan is cancelled, not just anytime the plan is + cancelled and the VM gets deleted. Also, it's now very clear what to do when you want to + use Plan::vm on the cancel path: you can null-check vm; if it's null, assume the worst. + + Because we null the VM of a cancelled plan, we cannot have Safepoint::vm() return the + plan's VM anymore. That's because when we cancel a plan that is at a safepoint, we use the + safepoint's VM to determine whether this is one of our safepoints *after* the plan is + already cancelled. So, Safepoint now has its own copy of m_vm, and that copy gets nulled + when the Safepoint is cancelled. The Safepoint's m_vm will be nulled moments after Plan's + vm gets nulled (see Worklist::removeDeadPlans(), which has a cancel path for Plans in one + loop and a cancel path for Safepoints in the loop after it). + + * dfg/DFGJITFinalizer.cpp: + (JSC::DFG::JITFinalizer::finalizeCommon): + * dfg/DFGPlan.cpp: + (JSC::DFG::Plan::Plan): + (JSC::DFG::Plan::computeCompileTimes): + (JSC::DFG::Plan::reportCompileTimes): + (JSC::DFG::Plan::compileInThreadImpl): + (JSC::DFG::Plan::reallyAdd): + (JSC::DFG::Plan::notifyCompiling): + (JSC::DFG::Plan::finalizeWithoutNotifyingCallback): + (JSC::DFG::Plan::cancel): + * dfg/DFGPlan.h: + (JSC::DFG::Plan::canTierUpAndOSREnter): + * dfg/DFGSafepoint.cpp: + (JSC::DFG::Safepoint::cancel): + (JSC::DFG::Safepoint::vm): + * dfg/DFGSafepoint.h: + * dfg/DFGWorklist.cpp: + (JSC::DFG::Worklist::isActiveForVM): + (JSC::DFG::Worklist::waitUntilAllPlansForVMAreReady): + (JSC::DFG::Worklist::removeAllReadyPlansForVM): + (JSC::DFG::Worklist::rememberCodeBlocks): + (JSC::DFG::Worklist::visitWeakReferences): + (JSC::DFG::Worklist::removeDeadPlans): + (JSC::DFG::Worklist::runThread): + * ftl/FTLJITFinalizer.cpp: + (JSC::FTL::JITFinalizer::finalizeFunction): + 2016-05-15 Yusuke Suzuki Modernize Intl constructors; using InternalFunction::createSubclassStructure diff --git a/Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp b/Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp index d673c94..5391158 100644 --- a/Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp +++ b/Source/JavaScriptCore/dfg/DFGJITFinalizer.cpp @@ -91,7 +91,7 @@ void JITFinalizer::finalizeCommon() #endif // ENABLE(FTL_JIT) if (m_plan.compilation) - m_plan.vm.m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation); + m_plan.vm->m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation); if (!m_plan.willTryToTierUp) m_plan.codeBlock->baselineVersion()->m_didFailFTLCompilation = true; diff --git a/Source/JavaScriptCore/dfg/DFGPlan.cpp b/Source/JavaScriptCore/dfg/DFGPlan.cpp index d3e095e..355cb4e 100644 --- a/Source/JavaScriptCore/dfg/DFGPlan.cpp +++ b/Source/JavaScriptCore/dfg/DFGPlan.cpp @@ -138,13 +138,13 @@ Profiler::CompilationKind profilerCompilationKindForMode(CompilationMode mode) Plan::Plan(CodeBlock* passedCodeBlock, CodeBlock* profiledDFGCodeBlock, CompilationMode mode, unsigned osrEntryBytecodeIndex, const Operands& mustHandleValues) - : vm(*passedCodeBlock->vm()) + : vm(passedCodeBlock->vm()) , codeBlock(passedCodeBlock) , profiledDFGCodeBlock(profiledDFGCodeBlock) , mode(mode) , osrEntryBytecodeIndex(osrEntryBytecodeIndex) , mustHandleValues(mustHandleValues) - , compilation(codeBlock->vm()->m_perBytecodeProfiler ? adoptRef(new Profiler::Compilation(codeBlock->vm()->m_perBytecodeProfiler->ensureBytecodesFor(codeBlock), profilerCompilationKindForMode(mode))) : 0) + , compilation(vm->m_perBytecodeProfiler ? adoptRef(new Profiler::Compilation(vm->m_perBytecodeProfiler->ensureBytecodesFor(codeBlock), profilerCompilationKindForMode(mode))) : 0) , inlineCallFrames(adoptRef(new InlineCallFrameSet())) , identifiers(codeBlock) , weakReferences(codeBlock) @@ -160,7 +160,7 @@ bool Plan::computeCompileTimes() const { return reportCompileTimes() || Options::reportTotalCompileTimes() - || vm.m_perBytecodeProfiler; + || (vm && vm->m_perBytecodeProfiler); } bool Plan::reportCompileTimes() const @@ -244,7 +244,7 @@ Plan::CompilationPath Plan::compileInThreadImpl(LongLivedState& longLivedState) dataLog("\n"); } - Graph dfg(vm, *this, longLivedState); + Graph dfg(*vm, *this, longLivedState); if (!parse(dfg)) { finalizer = std::make_unique(*this); @@ -537,9 +537,9 @@ bool Plan::isStillValid() void Plan::reallyAdd(CommonData* commonData) { watchpoints.reallyAdd(codeBlock, *commonData); - identifiers.reallyAdd(vm, commonData); - weakReferences.reallyAdd(vm, commonData); - transitions.reallyAdd(vm, commonData); + identifiers.reallyAdd(*vm, commonData); + weakReferences.reallyAdd(*vm, commonData); + transitions.reallyAdd(*vm, commonData); } void Plan::notifyCompiling() @@ -561,7 +561,7 @@ void Plan::notifyReady() CompilationResult Plan::finalizeWithoutNotifyingCallback() { // We will establish new references from the code block to things. So, we need a barrier. - vm.heap.writeBarrier(codeBlock); + vm->heap.writeBarrier(codeBlock); if (!isStillValid()) { CODEBLOCK_LOG_EVENT(codeBlock, "dfgFinalize", ("invalidated")); @@ -660,6 +660,7 @@ bool Plan::isKnownToBeLiveDuringGC() void Plan::cancel() { + vm = nullptr; codeBlock = nullptr; profiledDFGCodeBlock = nullptr; mustHandleValues.clear(); diff --git a/Source/JavaScriptCore/dfg/DFGPlan.h b/Source/JavaScriptCore/dfg/DFGPlan.h index 0cbfa0a..1a9ef7e 100644 --- a/Source/JavaScriptCore/dfg/DFGPlan.h +++ b/Source/JavaScriptCore/dfg/DFGPlan.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2013-2015 Apple Inc. All rights reserved. + * Copyright (C) 2013-2016 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -78,7 +78,10 @@ struct Plan : public ThreadSafeRefCounted { bool canTierUpAndOSREnter() const { return !tierUpAndOSREnterBytecodes.isEmpty(); } - VM& vm; + // Warning: pretty much all of the pointer fields in this object get nulled by cancel(). So, if + // you're writing code that is callable on the cancel path, be sure to null check everything! + + VM* vm; // These can be raw pointers because we visit them during every GC in checkLivenessAndVisitChildren. CodeBlock* codeBlock; diff --git a/Source/JavaScriptCore/dfg/DFGSafepoint.cpp b/Source/JavaScriptCore/dfg/DFGSafepoint.cpp index 11ba5ad..550212e 100644 --- a/Source/JavaScriptCore/dfg/DFGSafepoint.cpp +++ b/Source/JavaScriptCore/dfg/DFGSafepoint.cpp @@ -1,5 +1,5 @@ /* - * Copyright (C) 2014 Apple Inc. All rights reserved. + * Copyright (C) 2014, 2016 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -47,7 +47,8 @@ bool Safepoint::Result::didGetCancelled() } Safepoint::Safepoint(Plan& plan, Result& result) - : m_plan(plan) + : m_vm(plan.vm) + , m_plan(plan) , m_didCallBegin(false) , m_result(result) { @@ -114,11 +115,12 @@ void Safepoint::cancel() m_plan.cancel(); m_result.m_didGetCancelled = true; + m_vm = nullptr; } -VM& Safepoint::vm() const +VM* Safepoint::vm() const { - return m_plan.vm; + return m_vm; } } } // namespace JSC::DFG diff --git a/Source/JavaScriptCore/dfg/DFGSafepoint.h b/Source/JavaScriptCore/dfg/DFGSafepoint.h index 96f4b8e..34cfaa6 100644 --- a/Source/JavaScriptCore/dfg/DFGSafepoint.h +++ b/Source/JavaScriptCore/dfg/DFGSafepoint.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2014 Apple Inc. All rights reserved. + * Copyright (C) 2014, 2016 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -72,9 +72,10 @@ public: bool isKnownToBeLiveDuringGC(); void cancel(); - VM& vm() const; + VM* vm() const; // May return null if we've been cancelled. private: + VM* m_vm; Plan& m_plan; Vector m_scannables; bool m_didCallBegin; diff --git a/Source/JavaScriptCore/dfg/DFGWorklist.cpp b/Source/JavaScriptCore/dfg/DFGWorklist.cpp index 9d7e68a0..4f566b8 100644 --- a/Source/JavaScriptCore/dfg/DFGWorklist.cpp +++ b/Source/JavaScriptCore/dfg/DFGWorklist.cpp @@ -1,5 +1,5 @@ /* - * Copyright (C) 2013, 2014 Apple Inc. All rights reserved. + * Copyright (C) 2013-2014, 2016 Apple Inc. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions @@ -80,7 +80,7 @@ bool Worklist::isActiveForVM(VM& vm) const LockHolder locker(m_lock); PlanMap::const_iterator end = m_plans.end(); for (PlanMap::const_iterator iter = m_plans.begin(); iter != end; ++iter) { - if (&iter->value->vm == &vm) + if (iter->value->vm == &vm) return true; } return false; @@ -129,7 +129,7 @@ void Worklist::waitUntilAllPlansForVMAreReady(VM& vm) bool allAreCompiled = true; PlanMap::iterator end = m_plans.end(); for (PlanMap::iterator iter = m_plans.begin(); iter != end; ++iter) { - if (&iter->value->vm != &vm) + if (iter->value->vm != &vm) continue; if (iter->value->stage != Plan::Ready) { allAreCompiled = false; @@ -150,7 +150,7 @@ void Worklist::removeAllReadyPlansForVM(VM& vm, Vector, 8>& myReady LockHolder locker(m_lock); for (size_t i = 0; i < m_readyPlans.size(); ++i) { RefPtr plan = m_readyPlans[i]; - if (&plan->vm != &vm) + if (plan->vm != &vm) continue; if (plan->stage != Plan::Ready) continue; @@ -212,7 +212,7 @@ void Worklist::rememberCodeBlocks(VM& vm) LockHolder locker(m_lock); for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) { Plan* plan = iter->value.get(); - if (&plan->vm != &vm) + if (plan->vm != &vm) continue; plan->rememberCodeBlocks(); } @@ -239,7 +239,7 @@ void Worklist::visitWeakReferences(SlotVisitor& visitor) LockHolder locker(m_lock); for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) { Plan* plan = iter->value.get(); - if (&plan->vm != vm) + if (plan->vm != vm) continue; plan->checkLivenessAndVisitChildren(visitor); } @@ -251,7 +251,7 @@ void Worklist::visitWeakReferences(SlotVisitor& visitor) for (unsigned i = m_threads.size(); i--;) { ThreadData* data = m_threads[i].get(); Safepoint* safepoint = data->m_safepoint; - if (safepoint && &safepoint->vm() == vm) + if (safepoint && safepoint->vm() == vm) safepoint->checkLivenessAndVisitChildren(visitor); } } @@ -263,7 +263,7 @@ void Worklist::removeDeadPlans(VM& vm) HashSet deadPlanKeys; for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) { Plan* plan = iter->value.get(); - if (&plan->vm != &vm) + if (plan->vm != &vm) continue; if (plan->isKnownToBeLiveDuringGC()) continue; @@ -296,7 +296,7 @@ void Worklist::removeDeadPlans(VM& vm) Safepoint* safepoint = data->m_safepoint; if (!safepoint) continue; - if (&safepoint->vm() != &vm) + if (safepoint->vm() != &vm) continue; if (safepoint->isKnownToBeLiveDuringGC()) continue; @@ -365,9 +365,9 @@ void Worklist::runThread(ThreadData* data) if (Options::verboseCompilationQueue()) dataLog(*this, ": Compiling ", plan->key(), " asynchronously\n"); - RELEASE_ASSERT(!plan->vm.heap.isCollecting()); + RELEASE_ASSERT(!plan->vm->heap.isCollecting()); plan->compileInThread(longLivedState, data); - RELEASE_ASSERT(plan->stage == Plan::Cancelled || !plan->vm.heap.isCollecting()); + RELEASE_ASSERT(plan->stage == Plan::Cancelled || !plan->vm->heap.isCollecting()); { LockHolder locker(m_lock); @@ -377,7 +377,7 @@ void Worklist::runThread(ThreadData* data) } plan->notifyCompiled(); } - RELEASE_ASSERT(!plan->vm.heap.isCollecting()); + RELEASE_ASSERT(!plan->vm->heap.isCollecting()); } { diff --git a/Source/JavaScriptCore/ftl/FTLJITFinalizer.cpp b/Source/JavaScriptCore/ftl/FTLJITFinalizer.cpp index e66c89f..00a88f0 100644 --- a/Source/JavaScriptCore/ftl/FTLJITFinalizer.cpp +++ b/Source/JavaScriptCore/ftl/FTLJITFinalizer.cpp @@ -83,7 +83,7 @@ bool JITFinalizer::finalizeFunction() m_plan.codeBlock->setJITCode(jitCode); if (m_plan.compilation) - m_plan.vm.m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation); + m_plan.vm->m_perBytecodeProfiler->addCompilation(m_plan.codeBlock, m_plan.compilation); return true; } -- 1.8.3.1