[api] Prepare DisallowJavascriptExecution scope around api callbacks · v8/v8@b36cf9f · GitHub
Skip to content

Commit b36cf9f

Browse files
o-v8-scoped@luci-project-accounts.iam.gserviceaccount.com
o-
authored and
v8-scoped@luci-project-accounts.iam.gserviceaccount.com
committed
[api] Prepare DisallowJavascriptExecution scope around api callbacks
In `v8::Isolate::RequestInterrupt` the API contract says: API interrupt callbacks are forbidden from executing JavaScript on the interrupted Isolate. This CL prepares the infrastructure to enforce this contract. Bug: 540137721, 40067940 TAG=agy CONV=78573798-a951-4bd3-9c6f-3e3b6775da4b Change-Id: I43cc83c3cc3ebd34052a927dde55bbf4d49d2d29 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8173727 Auto-Submit: Olivier Flückiger <olivf@chromium.org> Reviewed-by: Simon Zünd <szuend@chromium.org> Commit-Queue: Olivier Flückiger <olivf@chromium.org> Cr-Commit-Position: refs/heads/main@{#109086}
1 parent f14dd2d commit b36cf9f

11 files changed

Lines changed: 95 additions & 50 deletions

File tree

‎BUILD.bazel‎

Lines changed: 6 additions & 0 deletions

‎BUILD.gn‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,12 @@ declare_args() {
7171
# Enable compiler warnings when using V8_DEPRECATED apis.
7272
v8_deprecation_warnings = true
7373

74+
# Check that executing JavaScript inside API interrupt callbacks is
75+
# disallowed.
76+
# TODO(40067940): Ensure this does not break DevTools.
77+
# TODO(node): Upstream issue https://github.com/nodejs/node/pull/65028
78+
v8_disallow_js_in_api_interrupts_is_checked = false
79+
7480
# Enable compiler warnings when using V8_DEPRECATE_SOON apis.
7581
v8_imminent_deprecation_warnings = true
7682

@@ -1062,6 +1068,7 @@ external_v8_defines = [
10621068
"V8_COMPRESS_ZONES",
10631069
"V8_ENABLE_SANDBOX",
10641070
"V8_DEPRECATION_WARNINGS",
1071+
"V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED",
10651072
"V8_IMMINENT_DEPRECATION_WARNINGS",
10661073
"V8_USE_PERFETTO",
10671074
"V8_USE_PERFETTO_JSON_EXPORT",
@@ -1113,6 +1120,10 @@ if (v8_enable_sandbox) {
11131120
if (v8_deprecation_warnings) {
11141121
enabled_external_v8_defines += [ "V8_DEPRECATION_WARNINGS" ]
11151122
}
1123+
if (v8_disallow_js_in_api_interrupts_is_checked) {
1124+
enabled_external_v8_defines +=
1125+
[ "V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED" ]
1126+
}
11161127
if (v8_imminent_deprecation_warnings) {
11171128
enabled_external_v8_defines += [ "V8_IMMINENT_DEPRECATION_WARNINGS" ]
11181129
}
@@ -3223,6 +3234,8 @@ generated_file("v8_generate_features_json") {
32233234
output_conversion = "json"
32243235
contents = {
32253236
v8_deprecation_warnings = v8_deprecation_warnings
3237+
v8_disallow_js_in_api_interrupts_is_checked =
3238+
v8_disallow_js_in_api_interrupts_is_checked
32263239
v8_enable_31bit_smis_on_64bit_arch = v8_enable_31bit_smis_on_64bit_arch
32273240
v8_enable_direct_handle = v8_enable_direct_handle
32283241
v8_enable_extensible_ro_snapshot = v8_enable_extensible_ro_snapshot

‎src/common/assert-scope.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ class V8_NODISCARD PerThreadAssertScope
110110
ScopeType& operator=(const ScopeType&) = delete; \
111111
V8_EXPORT_PRIVATE ~ScopeType(); \
112112
\
113-
static bool IsAllowed(Isolate* isolate); \
113+
V8_EXPORT_PRIVATE static bool IsAllowed(Isolate* isolate); \
114114
\
115115
V8_EXPORT_PRIVATE static void Open(Isolate* isolate, \
116116
bool* was_execution_allowed); \

‎src/execution/isolate.cc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2622,6 +2622,13 @@ void Isolate::InvokeApiInterruptCallbacks() {
26222622
}
26232623
VMState<EXTERNAL> state(this);
26242624
HandleScope handle_scope(this);
2625+
// API interrupt callbacks are forbidden from executing JavaScript on the
2626+
// interrupted Isolate (see v8::Isolate::RequestInterrupt contract in
2627+
// v8-isolate.h: "Registered |callback| must not reenter interrupted
2628+
// Isolate.").
2629+
#ifdef V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED
2630+
DisallowJavascriptExecution no_js(this);
2631+
#endif // V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED
26252632
entry.first(reinterpret_cast<v8::Isolate*>(this), entry.second);
26262633
}
26272634
}

‎src/inspector/v8-debugger.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -535,6 +535,7 @@ void V8Debugger::handleProgramBreak(
535535
}
536536
});
537537
{
538+
v8::Isolate::AllowJavascriptExecutionScope allow_script(m_isolate);
538539
v8::Context::Scope scope(pausedContext);
539540

540541
m_inspector->forEachSession(

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ int V8InspectorImpl::resolveUniqueContextId(
9999

100100
v8::MaybeLocal<v8::Value> V8InspectorImpl::compileAndRunInternalScript(
101101
v8::Local<v8::Context> context, v8::Local<v8::String> source) {
102+
v8::Isolate::AllowJavascriptExecutionScope allow_script(m_isolate);
102103
v8::Local<v8::UnboundScript> unboundScript;
103104
if (!v8::debug::CompileInspectorScript(m_isolate, source)
104105
.ToLocal(&unboundScript)) {

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,8 @@ void V8InspectorSessionImpl::reportAllContexts(V8RuntimeAgentImpl* agent) {
367367

368368
void V8InspectorSessionImpl::dispatchProtocolMessage(
369369
StringView message, StringView associated_data) {
370+
v8::Isolate::AllowJavascriptExecutionScope allow_script(
371+
m_inspector->isolate());
370372
KeepSessionAliveScope keepAlive(*this);
371373

372374
using v8_crdtp::span;

‎src/inspector/v8-regex.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ V8Regex::V8Regex(V8InspectorImpl* inspector, const String16& pattern,
3838
v8::Local<v8::RegExp> regex;
3939
// Protect against reentrant debugger calls via interrupts.
4040
v8::debug::PostponeInterruptsScope no_interrupts(m_inspector->isolate());
41+
v8::Isolate::AllowJavascriptExecutionScope allow_js(m_inspector->isolate());
4142
if (v8::RegExp::New(context, toV8String(isolate, pattern),
4243
static_cast<v8::RegExp::Flags>(flags))
4344
.ToLocal(&regex)) {
@@ -70,6 +71,7 @@ int V8Regex::match(const String16& string, int startFrom,
7071
v8::MicrotasksScope::kDoNotRunMicrotasks);
7172
// Protect against reentrant debugger calls via interrupts.
7273
v8::debug::PostponeInterruptsScope no_interrupts(m_inspector->isolate());
74+
v8::Isolate::AllowJavascriptExecutionScope allow_js(m_inspector->isolate());
7375
v8::TryCatch tryCatch(isolate);
7476

7577
v8::Local<v8::RegExp> regex = m_regex.Get(isolate);

‎test/cctest/test-api.cc‎

Lines changed: 20 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -22514,6 +22514,26 @@ TEST(RequestInterruptSmallScripts) {
2251422514
CHECK(interrupt_was_called);
2251522515
}
2251622516

22517+
#ifdef V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED
22518+
static bool interrupt_check_no_js = false;
22519+
void DisallowJsInterruptCallback(v8::Isolate* isolate, void* data) {
22520+
CHECK(!i::AllowJavascriptExecution::IsAllowed(
22521+
reinterpret_cast<i::Isolate*>(isolate)));
22522+
interrupt_check_no_js = true;
22523+
}
22524+
22525+
TEST(RequestInterruptDisallowsJavascript) {
22526+
LocalContext env;
22527+
v8::Isolate* isolate = CcTest::isolate();
22528+
v8::HandleScope scope(isolate);
22529+
22530+
interrupt_check_no_js = false;
22531+
isolate->RequestInterrupt(&DisallowJsInterruptCallback, nullptr);
22532+
CompileRun("(function(x){return x;})(1);");
22533+
CHECK(interrupt_check_no_js);
22534+
}
22535+
#endif // V8_DISALLOW_JS_IN_API_INTERRUPTS_IS_CHECKED
22536+
2251722537
static v8::Global<Value> function_new_expected_env_global;
2251822538
static void FunctionNewCallback(const v8::FunctionCallbackInfo<Value>& info) {
2251922539
v8::Isolate* isolate = info.GetIsolate();
@@ -26243,55 +26263,7 @@ TEST(FutexInterruption) {
2624326263
timeout_thread.Join();
2624426264
}
2624526265

26246-
namespace {
26247-
class ReentrantWaitThread : public v8::base::Thread {
26248-
public:
26249-
explicit ReentrantWaitThread(v8::Isolate* isolate)
26250-
: Thread(Options("ReentrantWaitThread")), isolate_(isolate) {}
26251-
26252-
static void InterruptCallback(v8::Isolate* isolate, void* data) {
26253-
v8::HandleScope scope(isolate);
26254-
v8::TryCatch try_catch(isolate);
26255-
CompileRun(
26256-
"var sab2 = new SharedArrayBuffer(4);"
26257-
"var i32a2 = new Int32Array(sab2);"
26258-
"Atomics.wait(i32a2, 0, 0, 10);");
26259-
26260-
CHECK(try_catch.HasCaught());
26261-
v8::String::Utf8Value exception_msg(isolate, try_catch.Exception());
26262-
CHECK_NOT_NULL(strstr(*exception_msg, "cannot be called in this context"));
26263-
}
2626426266

26265-
void Run() override {
26266-
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate_);
26267-
// Wait until main thread enters WaitSyncImpl and marks node->waiting_ =
26268-
// true
26269-
while (!i_isolate->futex_wait_list_node()->IsWaiting()) {
26270-
v8::base::OS::Sleep(v8::base::TimeDelta::FromMilliseconds(1));
26271-
}
26272-
isolate_->RequestInterrupt(InterruptCallback, nullptr);
26273-
}
26274-
26275-
private:
26276-
v8::Isolate* isolate_;
26277-
};
26278-
} // namespace
26279-
26280-
TEST(FutexReentrantWait) {
26281-
v8::Isolate* isolate = CcTest::isolate();
26282-
v8::HandleScope scope(isolate);
26283-
LocalContext env;
26284-
26285-
ReentrantWaitThread thread(isolate);
26286-
CHECK(thread.Start());
26287-
26288-
CompileRun(
26289-
"var ab = new SharedArrayBuffer(4);"
26290-
"var i32a = new Int32Array(ab);"
26291-
"Atomics.wait(i32a, 0, 0, 500);");
26292-
26293-
thread.Join();
26294-
}
2629526267

2629626268
TEST(StackCheckTermination) {
2629726269
v8::Isolate* isolate = CcTest::isolate();

‎test/cctest/test-regexp.cc‎

Lines changed: 7 additions & 1 deletion

0 commit comments

Comments
 (0)