From 7994004493df2c9a24372587312ef6c458c7ed2b Mon Sep 17 00:00:00 2001 From: Jaroslav Sevcik Date: Tue, 14 Sep 2021 21:08:03 +0200 Subject: [PATCH] [inspector] Use ephemeron table for exception metadata EphemeronHashTable does not trigger interrupts when accessed (as opposed to calling the WeakMapGet builtin), so it avoids the use-after-free problem when reading exception metadata triggers session disconnect while holding a reference to the session. Bug: chromium:1241860 Change-Id: I29264b04b8daf682e7c33a97faedf50e323d57c4 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3158326 Commit-Queue: Jaroslav Sevcik Reviewed-by: Michael Lippautz Reviewed-by: Benedikt Meurer Cr-Commit-Position: refs/heads/main@{#76864} --- src/api/api.h | 4 +- src/debug/debug-interface.cc | 76 ++++++++++++------------------ src/debug/debug-interface.h | 16 +++---- src/inspector/inspected-context.cc | 15 +++--- src/inspector/inspected-context.h | 2 +- src/inspector/v8-inspector-impl.cc | 27 ++++++----- src/inspector/v8-inspector-impl.h | 13 +++-- test/cctest/test-debug.cc | 9 ++-- test/cctest/test-inspector.cc | 36 ++++++++++++++ test/inspector/inspector.status | 3 -- 10 files changed, 111 insertions(+), 90 deletions(-) diff --git a/src/api/api.h b/src/api/api.h index e24c951306..c255dad1e6 100644 --- a/src/api/api.h +++ b/src/api/api.h @@ -42,7 +42,7 @@ namespace debug { class AccessorPair; class GeneratorObject; class Script; -class WeakMap; +class EphemeronTable; } // namespace debug // Constants used in the implementation of the API. The most natural thing @@ -135,7 +135,7 @@ class RegisteredExtension { V(Proxy, JSProxy) \ V(debug::GeneratorObject, JSGeneratorObject) \ V(debug::Script, Script) \ - V(debug::WeakMap, JSWeakMap) \ + V(debug::EphemeronTable, EphemeronHashTable) \ V(debug::AccessorPair, AccessorPair) \ V(Promise, JSPromise) \ V(Primitive, Object) \ diff --git a/src/debug/debug-interface.cc b/src/debug/debug-interface.cc index 5ec5c023f7..add2b3dbb4 100644 --- a/src/debug/debug-interface.cc +++ b/src/debug/debug-interface.cc @@ -1177,61 +1177,43 @@ TypeProfile::ScriptData TypeProfile::GetScriptData(size_t i) const { return ScriptData(i, type_profile_); } -v8::MaybeLocal WeakMap::Get(v8::Local context, - v8::Local key) { - PREPARE_FOR_EXECUTION(context, WeakMap, Get, Value); - auto self = Utils::OpenHandle(this); - Local result; - i::Handle argv[] = {Utils::OpenHandle(*key)}; - has_pending_exception = - !ToLocal(i::Execution::CallBuiltin(isolate, isolate->weakmap_get(), - self, arraysize(argv), argv), - &result); - RETURN_ON_FAILED_EXECUTION(Value); - RETURN_ESCAPED(result); +MaybeLocal EphemeronTable::Get(v8::Isolate* isolate, + v8::Local key) { + i::Isolate* internal_isolate = reinterpret_cast(isolate); + auto self = i::Handle::cast(Utils::OpenHandle(this)); + i::Handle internal_key = Utils::OpenHandle(*key); + DCHECK(internal_key->IsJSReceiver()); + + i::Handle value(self->Lookup(internal_key), internal_isolate); + + if (value->IsTheHole()) return {}; + return Utils::ToLocal(value); } -v8::Maybe WeakMap::Delete(v8::Local context, - v8::Local key) { - PREPARE_FOR_EXECUTION_WITH_CONTEXT(context, WeakMap, Delete, Nothing(), - InternalEscapableScope, false); - auto self = Utils::OpenHandle(this); - Local result; - i::Handle argv[] = {Utils::OpenHandle(*key)}; - has_pending_exception = !ToLocal( - i::Execution::CallBuiltin(isolate, isolate->weakmap_delete(), self, - arraysize(argv), argv), - &result); - RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool); - return Just(result->IsTrue()); +Local EphemeronTable::Set(v8::Isolate* isolate, + v8::Local key, + v8::Local value) { + auto self = i::Handle::cast(Utils::OpenHandle(this)); + i::Handle internal_key = Utils::OpenHandle(*key); + i::Handle internal_value = Utils::OpenHandle(*value); + DCHECK(internal_key->IsJSReceiver()); + + i::Handle result( + i::EphemeronHashTable::Put(self, internal_key, internal_value)); + + return ToApiHandle(result); } -v8::MaybeLocal WeakMap::Set(v8::Local context, - v8::Local key, - v8::Local value) { - PREPARE_FOR_EXECUTION(context, WeakMap, Set, WeakMap); - auto self = Utils::OpenHandle(this); - i::Handle result; - i::Handle argv[] = {Utils::OpenHandle(*key), - Utils::OpenHandle(*value)}; - has_pending_exception = - !i::Execution::CallBuiltin(isolate, isolate->weakmap_set(), self, - arraysize(argv), argv) - .ToHandle(&result); - RETURN_ON_FAILED_EXECUTION(WeakMap); - RETURN_ESCAPED(Local::Cast(Utils::ToLocal(result))); -} - -Local WeakMap::New(v8::Isolate* isolate) { +Local EphemeronTable::New(v8::Isolate* isolate) { i::Isolate* i_isolate = reinterpret_cast(isolate); - LOG_API(i_isolate, WeakMap, New); ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate); - i::Handle obj = i_isolate->factory()->NewJSWeakMap(); - return ToApiHandle(obj); + i::Handle table = + i::EphemeronHashTable::New(i_isolate, 0); + return ToApiHandle(table); } -WeakMap* WeakMap::Cast(v8::Value* value) { - return static_cast(value); +EphemeronTable* EphemeronTable::Cast(v8::Value* value) { + return static_cast(value); } Local AccessorPair::getter() { diff --git a/src/debug/debug-interface.h b/src/debug/debug-interface.h index 104cc02105..8c0ddb46cb 100644 --- a/src/debug/debug-interface.h +++ b/src/debug/debug-interface.h @@ -570,19 +570,17 @@ class V8_NODISCARD DisableBreakScope { std::unique_ptr scope_; }; -class WeakMap : public v8::Object { +class EphemeronTable : public v8::Object { public: - WeakMap() = delete; + EphemeronTable() = delete; V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::MaybeLocal Get( - v8::Local context, v8::Local key); - V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::Maybe Delete( - v8::Local context, v8::Local key); - V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::MaybeLocal Set( - v8::Local context, v8::Local key, + v8::Isolate* isolate, v8::Local key); + V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::Local Set( + v8::Isolate* isolate, v8::Local key, v8::Local value); - V8_EXPORT_PRIVATE static Local New(v8::Isolate* isolate); - V8_INLINE static WeakMap* Cast(Value* obj); + V8_EXPORT_PRIVATE static Local New(v8::Isolate* isolate); + V8_INLINE static EphemeronTable* Cast(Value* obj); }; /** diff --git a/src/inspector/inspected-context.cc b/src/inspector/inspected-context.cc index 6786f06b2f..59d186e43f 100644 --- a/src/inspector/inspected-context.cc +++ b/src/inspector/inspected-context.cc @@ -126,12 +126,15 @@ void InspectedContext::discardInjectedScript(int sessionId) { bool InspectedContext::addInternalObject(v8::Local object, V8InternalValueType type) { if (m_internalObjects.IsEmpty()) { - m_internalObjects.Reset(isolate(), v8::debug::WeakMap::New(isolate())); + m_internalObjects.Reset(isolate(), + v8::debug::EphemeronTable::New(isolate())); } - return !m_internalObjects.Get(isolate()) - ->Set(m_context.Get(isolate()), object, - v8::Integer::New(isolate(), static_cast(type))) - .IsEmpty(); + v8::Local new_map = + m_internalObjects.Get(isolate())->Set( + isolate(), object, + v8::Integer::New(isolate(), static_cast(type))); + m_internalObjects.Reset(isolate(), new_map); + return true; } V8InternalValueType InspectedContext::getInternalType( @@ -139,7 +142,7 @@ V8InternalValueType InspectedContext::getInternalType( if (m_internalObjects.IsEmpty()) return V8InternalValueType::kNone; v8::Local typeValue; if (!m_internalObjects.Get(isolate()) - ->Get(m_context.Get(isolate()), object) + ->Get(isolate(), object) .ToLocal(&typeValue) || !typeValue->IsUint32()) { return V8InternalValueType::kNone; diff --git a/src/inspector/inspected-context.h b/src/inspector/inspected-context.h index f8811d0469..50e5a87bb3 100644 --- a/src/inspector/inspected-context.h +++ b/src/inspector/inspected-context.h @@ -77,7 +77,7 @@ class InspectedContext { std::unordered_set m_reportedSessionIds; std::unordered_map> m_injectedScripts; WeakCallbackData* m_weakCallbackData; - v8::Global m_internalObjects; + v8::Global m_internalObjects; }; } // namespace v8_inspector diff --git a/src/inspector/v8-inspector-impl.cc b/src/inspector/v8-inspector-impl.cc index ae598244b8..3f48449a99 100644 --- a/src/inspector/v8-inspector-impl.cc +++ b/src/inspector/v8-inspector-impl.cc @@ -348,7 +348,14 @@ v8::MaybeLocal V8InspectorImpl::regexContext() { } v8::MaybeLocal V8InspectorImpl::exceptionMetaDataContext() { - return {}; + if (m_exceptionMetaDataContext.IsEmpty()) { + m_exceptionMetaDataContext.Reset(m_isolate, v8::Context::New(m_isolate)); + if (m_exceptionMetaDataContext.IsEmpty()) { + DCHECK(m_isolate->IsExecutionTerminating()); + return {}; + } + } + return m_exceptionMetaDataContext.Get(m_isolate); } void V8InspectorImpl::discardInspectedContext(int contextGroupId, @@ -479,19 +486,17 @@ bool V8InspectorImpl::associateExceptionData(v8::Local, v8::Context::Scope contextScope(context); v8::HandleScope handles(m_isolate); if (m_exceptionMetaData.IsEmpty()) - m_exceptionMetaData.Reset(m_isolate, v8::debug::WeakMap::New(m_isolate)); + m_exceptionMetaData.Reset(m_isolate, + v8::debug::EphemeronTable::New(m_isolate)); - v8::Local map = m_exceptionMetaData.Get(m_isolate); - v8::MaybeLocal entry = map->Get(context, exception); + v8::Local map = m_exceptionMetaData.Get(m_isolate); + v8::MaybeLocal entry = map->Get(m_isolate, exception); v8::Local object; if (entry.IsEmpty() || !entry.ToLocalChecked()->IsObject()) { object = v8::Object::New(m_isolate, v8::Null(m_isolate), nullptr, nullptr, 0); - v8::MaybeLocal new_map = - map->Set(context, exception, object); - if (!new_map.IsEmpty()) { - m_exceptionMetaData.Reset(m_isolate, new_map.ToLocalChecked()); - } + m_exceptionMetaData.Reset(m_isolate, + map->Set(m_isolate, exception, object)); } else { object = entry.ToLocalChecked().As(); } @@ -511,8 +516,8 @@ v8::MaybeLocal V8InspectorImpl::getAssociatedExceptionData( !exceptionMetaDataContext().ToLocal(&context)) { return v8::MaybeLocal(); } - v8::Local map = m_exceptionMetaData.Get(m_isolate); - auto entry = map->Get(context, exception); + v8::Local map = m_exceptionMetaData.Get(m_isolate); + auto entry = map->Get(m_isolate, exception); v8::Local object; if (!entry.ToLocal(&object) || !object->IsObject()) return v8::MaybeLocal(); diff --git a/src/inspector/v8-inspector-impl.h b/src/inspector/v8-inspector-impl.h index 5c797bbfc7..d628c57a20 100644 --- a/src/inspector/v8-inspector-impl.h +++ b/src/inspector/v8-inspector-impl.h @@ -56,7 +56,7 @@ class V8StackTraceImpl; class V8InspectorImpl : public V8Inspector { public: - V8InspectorImpl(v8::Isolate*, V8InspectorClient*); + V8_EXPORT_PRIVATE V8InspectorImpl(v8::Isolate*, V8InspectorClient*); ~V8InspectorImpl() override; V8InspectorImpl(const V8InspectorImpl&) = delete; V8InspectorImpl& operator=(const V8InspectorImpl&) = delete; @@ -110,10 +110,9 @@ class V8InspectorImpl : public V8Inspector { void externalAsyncTaskStarted(const V8StackTraceId& parent) override; void externalAsyncTaskFinished(const V8StackTraceId& parent) override; - bool associateExceptionData(v8::Local, - v8::Local exception, - v8::Local key, - v8::Local value) override; + V8_EXPORT_PRIVATE bool associateExceptionData( + v8::Local, v8::Local exception, + v8::Local key, v8::Local value) override; unsigned nextExceptionId() { return ++m_lastExceptionId; } void enableStackCapturingIfNeeded(); @@ -134,7 +133,7 @@ class V8InspectorImpl : public V8Inspector { int contextGroupId, const std::function& callback); int64_t generateUniqueId(); - v8::MaybeLocal getAssociatedExceptionData( + V8_EXPORT_PRIVATE v8::MaybeLocal getAssociatedExceptionData( v8::Local exception); class EvaluateScope { @@ -160,7 +159,7 @@ class V8InspectorImpl : public V8Inspector { std::unique_ptr m_debugger; v8::Global m_regexContext; v8::Global m_exceptionMetaDataContext; - v8::Global m_exceptionMetaData; + v8::Global m_exceptionMetaData; int m_capturingStackTracesCount; unsigned m_lastExceptionId; int m_lastContextId; diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc index e62fef4b1c..171c963ea0 100644 --- a/test/cctest/test-debug.cc +++ b/test/cctest/test-debug.cc @@ -598,10 +598,11 @@ TEST(BreakPointApiIntrinsics) { CHECK_EQ(2, break_point_hit_count); break_point_hit_count = 0; - v8::Local weakmap = - v8::debug::WeakMap::New(env->GetIsolate()); - CHECK(!weakmap->Set(env.local(), weakmap, v8_num(1)).IsEmpty()); - CHECK(!weakmap->Get(env.local(), weakmap).IsEmpty()); + v8::Local weakmap = + v8::debug::EphemeronTable::New(env->GetIsolate()); + v8::Local key = v8::Object::New(env->GetIsolate()); + CHECK(!weakmap->Set(env->GetIsolate(), key, v8_num(1)).IsEmpty()); + CHECK(!weakmap->Get(env->GetIsolate(), key).IsEmpty()); CHECK_EQ(0, break_point_hit_count); } diff --git a/test/cctest/test-inspector.cc b/test/cctest/test-inspector.cc index c1651eaceb..49a08bc42e 100644 --- a/test/cctest/test-inspector.cc +++ b/test/cctest/test-inspector.cc @@ -9,6 +9,7 @@ #include "include/v8-primitive.h" #include "src/inspector/protocol/Runtime.h" #include "src/inspector/string-util.h" +#include "src/inspector/v8-inspector-impl.h" #include "test/cctest/cctest.h" using v8_inspector::StringBuffer; @@ -169,3 +170,38 @@ TEST(BinaryBase64RoundTrip) { CHECK_EQ(values[i], roundtrip_binary.data()[i]); } } + +TEST(NoInterruptOnGetAssociatedData) { + LocalContext env; + v8::Isolate* isolate = env->GetIsolate(); + v8::HandleScope handle_scope(isolate); + + v8_inspector::V8InspectorClient default_client; + std::unique_ptr inspector( + new v8_inspector::V8InspectorImpl(isolate, &default_client)); + + v8::Local context = env->GetIsolate()->GetCurrentContext(); + v8::Local error = v8::Exception::Error(v8_str("custom error")); + v8::Local key = v8_str("key"); + v8::Local value = v8_str("value"); + inspector->associateExceptionData(context, error, key, value); + + struct InterruptRecorder { + static void handler(v8::Isolate* isolate, void* data) { + reinterpret_cast(data)->WasInvoked = true; + } + + bool WasInvoked = false; + } recorder; + + isolate->RequestInterrupt(&InterruptRecorder::handler, &recorder); + + v8::Local data = + inspector->getAssociatedExceptionData(error).ToLocalChecked(); + CHECK(!recorder.WasInvoked); + + CHECK_EQ(data->Get(context, key).ToLocalChecked(), value); + + CompileRun("0"); + CHECK(recorder.WasInvoked); +} diff --git a/test/inspector/inspector.status b/test/inspector/inspector.status index 92dfeaf682..2fec7679dc 100644 --- a/test/inspector/inspector.status +++ b/test/inspector/inspector.status @@ -20,9 +20,6 @@ # loop instead of properly reporting a RangeError for a stack overflow. 'regress/regress-crbug-1080638': [SKIP], - # https://crbug.com/1241860 - 'runtime/exception-thrown-metadata': [SKIP], - # Tests that need to run sequentially (e.g. due to memory consumption). 'runtime/console-messages-limits': [PASS, HEAVY], 'runtime/regression-732717': [PASS, HEAVY],