Skip to content

Commit 7994004

Browse files
jaro-sevcikV8 LUCI CQ
jaro-sevcik
authored and
V8 LUCI CQ
committed
[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 <[email protected]> Reviewed-by: Michael Lippautz <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/main@{#76864}
1 parent 9c601fb commit 7994004

File tree

10 files changed

+113
-92
lines changed

10 files changed

+113
-92
lines changed

‎src/api/api.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ namespace debug {
4242
class AccessorPair;
4343
class GeneratorObject;
4444
class Script;
45-
class WeakMap;
45+
class EphemeronTable;
4646
} // namespace debug
4747

4848
// Constants used in the implementation of the API. The most natural thing
@@ -135,7 +135,7 @@ class RegisteredExtension {
135135
V(Proxy, JSProxy) \
136136
V(debug::GeneratorObject, JSGeneratorObject) \
137137
V(debug::Script, Script) \
138-
V(debug::WeakMap, JSWeakMap) \
138+
V(debug::EphemeronTable, EphemeronHashTable) \
139139
V(debug::AccessorPair, AccessorPair) \
140140
V(Promise, JSPromise) \
141141
V(Primitive, Object) \

‎src/debug/debug-interface.cc‎

Lines changed: 31 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1177,61 +1177,43 @@ TypeProfile::ScriptData TypeProfile::GetScriptData(size_t i) const {
11771177
return ScriptData(i, type_profile_);
11781178
}
11791179

1180-
v8::MaybeLocal<v8::Value> WeakMap::Get(v8::Local<v8::Context> context,
1181-
v8::Local<v8::Value> key) {
1182-
PREPARE_FOR_EXECUTION(context, WeakMap, Get, Value);
1183-
auto self = Utils::OpenHandle(this);
1184-
Local<Value> result;
1185-
i::Handle<i::Object> argv[] = {Utils::OpenHandle(*key)};
1186-
has_pending_exception =
1187-
!ToLocal<Value>(i::Execution::CallBuiltin(isolate, isolate->weakmap_get(),
1188-
self, arraysize(argv), argv),
1189-
&result);
1190-
RETURN_ON_FAILED_EXECUTION(Value);
1191-
RETURN_ESCAPED(result);
1180+
MaybeLocal<v8::Value> EphemeronTable::Get(v8::Isolate* isolate,
1181+
v8::Local<v8::Value> key) {
1182+
i::Isolate* internal_isolate = reinterpret_cast<i::Isolate*>(isolate);
1183+
auto self = i::Handle<i::EphemeronHashTable>::cast(Utils::OpenHandle(this));
1184+
i::Handle<i::Object> internal_key = Utils::OpenHandle(*key);
1185+
DCHECK(internal_key->IsJSReceiver());
1186+
1187+
i::Handle<i::Object> value(self->Lookup(internal_key), internal_isolate);
1188+
1189+
if (value->IsTheHole()) return {};
1190+
return Utils::ToLocal(value);
11921191
}
11931192

1194-
v8::Maybe<bool> WeakMap::Delete(v8::Local<v8::Context> context,
1195-
v8::Local<v8::Value> key) {
1196-
PREPARE_FOR_EXECUTION_WITH_CONTEXT(context, WeakMap, Delete, Nothing<bool>(),
1197-
InternalEscapableScope, false);
1198-
auto self = Utils::OpenHandle(this);
1199-
Local<Value> result;
1200-
i::Handle<i::Object> argv[] = {Utils::OpenHandle(*key)};
1201-
has_pending_exception = !ToLocal<Value>(
1202-
i::Execution::CallBuiltin(isolate, isolate->weakmap_delete(), self,
1203-
arraysize(argv), argv),
1204-
&result);
1205-
RETURN_ON_FAILED_EXECUTION_PRIMITIVE(bool);
1206-
return Just(result->IsTrue());
1207-
}
1208-
1209-
v8::MaybeLocal<WeakMap> WeakMap::Set(v8::Local<v8::Context> context,
1210-
v8::Local<v8::Value> key,
1211-
v8::Local<v8::Value> value) {
1212-
PREPARE_FOR_EXECUTION(context, WeakMap, Set, WeakMap);
1213-
auto self = Utils::OpenHandle(this);
1214-
i::Handle<i::Object> result;
1215-
i::Handle<i::Object> argv[] = {Utils::OpenHandle(*key),
1216-
Utils::OpenHandle(*value)};
1217-
has_pending_exception =
1218-
!i::Execution::CallBuiltin(isolate, isolate->weakmap_set(), self,
1219-
arraysize(argv), argv)
1220-
.ToHandle(&result);
1221-
RETURN_ON_FAILED_EXECUTION(WeakMap);
1222-
RETURN_ESCAPED(Local<WeakMap>::Cast(Utils::ToLocal(result)));
1223-
}
1224-
1225-
Local<WeakMap> WeakMap::New(v8::Isolate* isolate) {
1193+
Local<EphemeronTable> EphemeronTable::Set(v8::Isolate* isolate,
1194+
v8::Local<v8::Value> key,
1195+
v8::Local<v8::Value> value) {
1196+
auto self = i::Handle<i::EphemeronHashTable>::cast(Utils::OpenHandle(this));
1197+
i::Handle<i::Object> internal_key = Utils::OpenHandle(*key);
1198+
i::Handle<i::Object> internal_value = Utils::OpenHandle(*value);
1199+
DCHECK(internal_key->IsJSReceiver());
1200+
1201+
i::Handle<i::EphemeronHashTable> result(
1202+
i::EphemeronHashTable::Put(self, internal_key, internal_value));
1203+
1204+
return ToApiHandle<EphemeronTable>(result);
1205+
}
1206+
1207+
Local<EphemeronTable> EphemeronTable::New(v8::Isolate* isolate) {
12261208
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
1227-
LOG_API(i_isolate, WeakMap, New);
12281209
ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
1229-
i::Handle<i::JSWeakMap> obj = i_isolate->factory()->NewJSWeakMap();
1230-
return ToApiHandle<WeakMap>(obj);
1210+
i::Handle<i::EphemeronHashTable> table =
1211+
i::EphemeronHashTable::New(i_isolate, 0);
1212+
return ToApiHandle<EphemeronTable>(table);
12311213
}
12321214

1233-
WeakMap* WeakMap::Cast(v8::Value* value) {
1234-
return static_cast<WeakMap*>(value);
1215+
EphemeronTable* EphemeronTable::Cast(v8::Value* value) {
1216+
return static_cast<EphemeronTable*>(value);
12351217
}
12361218

12371219
Local<Value> AccessorPair::getter() {

‎src/debug/debug-interface.h‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -570,19 +570,17 @@ class V8_NODISCARD DisableBreakScope {
570570
std::unique_ptr<i::DisableBreak> scope_;
571571
};
572572

573-
class WeakMap : public v8::Object {
573+
class EphemeronTable : public v8::Object {
574574
public:
575-
WeakMap() = delete;
575+
EphemeronTable() = delete;
576576
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::MaybeLocal<v8::Value> Get(
577-
v8::Local<v8::Context> context, v8::Local<v8::Value> key);
578-
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::Maybe<bool> Delete(
579-
v8::Local<v8::Context> context, v8::Local<v8::Value> key);
580-
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::MaybeLocal<WeakMap> Set(
581-
v8::Local<v8::Context> context, v8::Local<v8::Value> key,
577+
v8::Isolate* isolate, v8::Local<v8::Value> key);
578+
V8_EXPORT_PRIVATE V8_WARN_UNUSED_RESULT v8::Local<EphemeronTable> Set(
579+
v8::Isolate* isolate, v8::Local<v8::Value> key,
582580
v8::Local<v8::Value> value);
583581

584-
V8_EXPORT_PRIVATE static Local<WeakMap> New(v8::Isolate* isolate);
585-
V8_INLINE static WeakMap* Cast(Value* obj);
582+
V8_EXPORT_PRIVATE static Local<EphemeronTable> New(v8::Isolate* isolate);
583+
V8_INLINE static EphemeronTable* Cast(Value* obj);
586584
};
587585

588586
/**

‎src/inspector/inspected-context.cc‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,20 +126,23 @@ void InspectedContext::discardInjectedScript(int sessionId) {
126126
bool InspectedContext::addInternalObject(v8::Local<v8::Object> object,
127127
V8InternalValueType type) {
128128
if (m_internalObjects.IsEmpty()) {
129-
m_internalObjects.Reset(isolate(), v8::debug::WeakMap::New(isolate()));
129+
m_internalObjects.Reset(isolate(),
130+
v8::debug::EphemeronTable::New(isolate()));
130131
}
131-
return !m_internalObjects.Get(isolate())
132-
->Set(m_context.Get(isolate()), object,
133-
v8::Integer::New(isolate(), static_cast<int>(type)))
134-
.IsEmpty();
132+
v8::Local<v8::debug::EphemeronTable> new_map =
133+
m_internalObjects.Get(isolate())->Set(
134+
isolate(), object,
135+
v8::Integer::New(isolate(), static_cast<int>(type)));
136+
m_internalObjects.Reset(isolate(), new_map);
137+
return true;
135138
}
136139

137140
V8InternalValueType InspectedContext::getInternalType(
138141
v8::Local<v8::Object> object) {
139142
if (m_internalObjects.IsEmpty()) return V8InternalValueType::kNone;
140143
v8::Local<v8::Value> typeValue;
141144
if (!m_internalObjects.Get(isolate())
142-
->Get(m_context.Get(isolate()), object)
145+
->Get(isolate(), object)
143146
.ToLocal(&typeValue) ||
144147
!typeValue->IsUint32()) {
145148
return V8InternalValueType::kNone;

‎src/inspector/inspected-context.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ class InspectedContext {
7777
std::unordered_set<int> m_reportedSessionIds;
7878
std::unordered_map<int, std::unique_ptr<InjectedScript>> m_injectedScripts;
7979
WeakCallbackData* m_weakCallbackData;
80-
v8::Global<v8::debug::WeakMap> m_internalObjects;
80+
v8::Global<v8::debug::EphemeronTable> m_internalObjects;
8181
};
8282

8383
} // namespace v8_inspector

‎src/inspector/v8-inspector-impl.cc‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,14 @@ v8::MaybeLocal<v8::Context> V8InspectorImpl::regexContext() {
348348
}
349349

350350
v8::MaybeLocal<v8::Context> V8InspectorImpl::exceptionMetaDataContext() {
351-
return {};
351+
if (m_exceptionMetaDataContext.IsEmpty()) {
352+
m_exceptionMetaDataContext.Reset(m_isolate, v8::Context::New(m_isolate));
353+
if (m_exceptionMetaDataContext.IsEmpty()) {
354+
DCHECK(m_isolate->IsExecutionTerminating());
355+
return {};
356+
}
357+
}
358+
return m_exceptionMetaDataContext.Get(m_isolate);
352359
}
353360

354361
void V8InspectorImpl::discardInspectedContext(int contextGroupId,
@@ -479,19 +486,17 @@ bool V8InspectorImpl::associateExceptionData(v8::Local<v8::Context>,
479486
v8::Context::Scope contextScope(context);
480487
v8::HandleScope handles(m_isolate);
481488
if (m_exceptionMetaData.IsEmpty())
482-
m_exceptionMetaData.Reset(m_isolate, v8::debug::WeakMap::New(m_isolate));
489+
m_exceptionMetaData.Reset(m_isolate,
490+
v8::debug::EphemeronTable::New(m_isolate));
483491

484-
v8::Local<v8::debug::WeakMap> map = m_exceptionMetaData.Get(m_isolate);
485-
v8::MaybeLocal<v8::Value> entry = map->Get(context, exception);
492+
v8::Local<v8::debug::EphemeronTable> map = m_exceptionMetaData.Get(m_isolate);
493+
v8::MaybeLocal<v8::Value> entry = map->Get(m_isolate, exception);
486494
v8::Local<v8::Object> object;
487495
if (entry.IsEmpty() || !entry.ToLocalChecked()->IsObject()) {
488496
object =
489497
v8::Object::New(m_isolate, v8::Null(m_isolate), nullptr, nullptr, 0);
490-
v8::MaybeLocal<v8::debug::WeakMap> new_map =
491-
map->Set(context, exception, object);
492-
if (!new_map.IsEmpty()) {
493-
m_exceptionMetaData.Reset(m_isolate, new_map.ToLocalChecked());
494-
}
498+
m_exceptionMetaData.Reset(m_isolate,
499+
map->Set(m_isolate, exception, object));
495500
} else {
496501
object = entry.ToLocalChecked().As<v8::Object>();
497502
}
@@ -511,8 +516,8 @@ v8::MaybeLocal<v8::Object> V8InspectorImpl::getAssociatedExceptionData(
511516
!exceptionMetaDataContext().ToLocal(&context)) {
512517
return v8::MaybeLocal<v8::Object>();
513518
}
514-
v8::Local<v8::debug::WeakMap> map = m_exceptionMetaData.Get(m_isolate);
515-
auto entry = map->Get(context, exception);
519+
v8::Local<v8::debug::EphemeronTable> map = m_exceptionMetaData.Get(m_isolate);
520+
auto entry = map->Get(m_isolate, exception);
516521
v8::Local<v8::Value> object;
517522
if (!entry.ToLocal(&object) || !object->IsObject())
518523
return v8::MaybeLocal<v8::Object>();

‎src/inspector/v8-inspector-impl.h‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class V8StackTraceImpl;
5656

5757
class V8InspectorImpl : public V8Inspector {
5858
public:
59-
V8InspectorImpl(v8::Isolate*, V8InspectorClient*);
59+
V8_EXPORT_PRIVATE V8InspectorImpl(v8::Isolate*, V8InspectorClient*);
6060
~V8InspectorImpl() override;
6161
V8InspectorImpl(const V8InspectorImpl&) = delete;
6262
V8InspectorImpl& operator=(const V8InspectorImpl&) = delete;
@@ -110,10 +110,9 @@ class V8InspectorImpl : public V8Inspector {
110110
void externalAsyncTaskStarted(const V8StackTraceId& parent) override;
111111
void externalAsyncTaskFinished(const V8StackTraceId& parent) override;
112112

113-
bool associateExceptionData(v8::Local<v8::Context>,
114-
v8::Local<v8::Value> exception,
115-
v8::Local<v8::Name> key,
116-
v8::Local<v8::Value> value) override;
113+
V8_EXPORT_PRIVATE bool associateExceptionData(
114+
v8::Local<v8::Context>, v8::Local<v8::Value> exception,
115+
v8::Local<v8::Name> key, v8::Local<v8::Value> value) override;
117116

118117
unsigned nextExceptionId() { return ++m_lastExceptionId; }
119118
void enableStackCapturingIfNeeded();
@@ -134,7 +133,7 @@ class V8InspectorImpl : public V8Inspector {
134133
int contextGroupId,
135134
const std::function<void(V8InspectorSessionImpl*)>& callback);
136135
int64_t generateUniqueId();
137-
v8::MaybeLocal<v8::Object> getAssociatedExceptionData(
136+
V8_EXPORT_PRIVATE v8::MaybeLocal<v8::Object> getAssociatedExceptionData(
138137
v8::Local<v8::Value> exception);
139138

140139
class EvaluateScope {
@@ -160,7 +159,7 @@ class V8InspectorImpl : public V8Inspector {
160159
std::unique_ptr<V8Debugger> m_debugger;
161160
v8::Global<v8::Context> m_regexContext;
162161
v8::Global<v8::Context> m_exceptionMetaDataContext;
163-
v8::Global<v8::debug::WeakMap> m_exceptionMetaData;
162+
v8::Global<v8::debug::EphemeronTable> m_exceptionMetaData;
164163
int m_capturingStackTracesCount;
165164
unsigned m_lastExceptionId;
166165
int m_lastContextId;

‎test/cctest/test-debug.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -598,10 +598,11 @@ TEST(BreakPointApiIntrinsics) {
598598
CHECK_EQ(2, break_point_hit_count);
599599

600600
break_point_hit_count = 0;
601-
v8::Local<v8::debug::WeakMap> weakmap =
602-
v8::debug::WeakMap::New(env->GetIsolate());
603-
CHECK(!weakmap->Set(env.local(), weakmap, v8_num(1)).IsEmpty());
604-
CHECK(!weakmap->Get(env.local(), weakmap).IsEmpty());
601+
v8::Local<v8::debug::EphemeronTable> weakmap =
602+
v8::debug::EphemeronTable::New(env->GetIsolate());
603+
v8::Local<v8::Object> key = v8::Object::New(env->GetIsolate());
604+
CHECK(!weakmap->Set(env->GetIsolate(), key, v8_num(1)).IsEmpty());
605+
CHECK(!weakmap->Get(env->GetIsolate(), key).IsEmpty());
605606
CHECK_EQ(0, break_point_hit_count);
606607
}
607608

‎test/cctest/test-inspector.cc‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include "include/v8-primitive.h"
1010
#include "src/inspector/protocol/Runtime.h"
1111
#include "src/inspector/string-util.h"
12+
#include "src/inspector/v8-inspector-impl.h"
1213
#include "test/cctest/cctest.h"
1314

1415
using v8_inspector::StringBuffer;
@@ -169,3 +170,38 @@ TEST(BinaryBase64RoundTrip) {
169170
CHECK_EQ(values[i], roundtrip_binary.data()[i]);
170171
}
171172
}
173+
174+
TEST(NoInterruptOnGetAssociatedData) {
175+
LocalContext env;
176+
v8::Isolate* isolate = env->GetIsolate();
177+
v8::HandleScope handle_scope(isolate);
178+
179+
v8_inspector::V8InspectorClient default_client;
180+
std::unique_ptr<v8_inspector::V8InspectorImpl> inspector(
181+
new v8_inspector::V8InspectorImpl(isolate, &default_client));
182+
183+
v8::Local<v8::Context> context = env->GetIsolate()->GetCurrentContext();
184+
v8::Local<v8::Value> error = v8::Exception::Error(v8_str("custom error"));
185+
v8::Local<v8::Name> key = v8_str("key");
186+
v8::Local<v8::Value> value = v8_str("value");
187+
inspector->associateExceptionData(context, error, key, value);
188+
189+
struct InterruptRecorder {
190+
static void handler(v8::Isolate* isolate, void* data) {
191+
reinterpret_cast<InterruptRecorder*>(data)->WasInvoked = true;
192+
}
193+
194+
bool WasInvoked = false;
195+
} recorder;
196+
197+
isolate->RequestInterrupt(&InterruptRecorder::handler, &recorder);
198+
199+
v8::Local<v8::Object> data =
200+
inspector->getAssociatedExceptionData(error).ToLocalChecked();
201+
CHECK(!recorder.WasInvoked);
202+
203+
CHECK_EQ(data->Get(context, key).ToLocalChecked(), value);
204+
205+
CompileRun("0");
206+
CHECK(recorder.WasInvoked);
207+
}

‎test/inspector/inspector.status‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,6 @@
2020
# loop instead of properly reporting a RangeError for a stack overflow.
2121
'regress/regress-crbug-1080638': [SKIP],
2222

23-
# https://crbug.com/1241860
24-
'runtime/exception-thrown-metadata': [SKIP],
25-
2623
# Tests that need to run sequentially (e.g. due to memory consumption).
2724
'runtime/console-messages-limits': [PASS, HEAVY],
2825
'runtime/regression-732717': [PASS, HEAVY],

0 commit comments

Comments
 (0)