Skip to content

Commit c24df23

Browse files
legendecasV8 LUCI CQ
legendecas
authored and
V8 LUCI CQ
committed
[execution] Respect isolate stack limit in GetCentralStackView
A stack limit can be set for each v8::Isolate. The limit size can be greater than the one specified with --stack-size. `Heap::CollectGarbage` should not crash due to a `CHECK` on `Isolate::IsOnCentralStack()` with an isolate stack limit. Refs: nodejs/node#57114 Fixed: 400996806 Bug: 42202153 Change-Id: I80d0826fcd6a64261b8d745f8f47aa096bc83fb8 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6329659 Commit-Queue: Chengzhong Wu <[email protected]> Reviewed-by: Jakob Kummerow <[email protected]> Cr-Commit-Position: refs/heads/main@{#99228}
1 parent 23b91ca commit c24df23

File tree

5 files changed

+92
-5
lines changed

5 files changed

+92
-5
lines changed

‎src/api/api.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10024,6 +10024,7 @@ void Isolate::Initialize(Isolate* v8_isolate,
1002410024
uintptr_t limit =
1002510025
reinterpret_cast<uintptr_t>(params.constraints.stack_limit());
1002610026
i_isolate->stack_guard()->SetStackLimit(limit);
10027+
i_isolate->set_stack_size(base::Stack::GetStackStart() - limit);
1002710028
}
1002810029

1002910030
// TODO(v8:2487): Once we got rid of Isolate::Current(), we can remove this.
@@ -10670,6 +10671,7 @@ void Isolate::SetStackLimit(uintptr_t stack_limit) {
1067010671
i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(this);
1067110672
CHECK(stack_limit);
1067210673
i_isolate->stack_guard()->SetStackLimit(stack_limit);
10674+
i_isolate->set_stack_size(base::Stack::GetStackStart() - stack_limit);
1067310675
}
1067410676

1067510677
void Isolate::GetCodeRange(void** start, size_t* length_in_bytes) {

‎src/execution/isolate.cc‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4205,14 +4205,13 @@ Isolate::Isolate(IsolateGroup* isolate_group)
42054205
next_unique_sfi_id_(0),
42064206
next_module_async_evaluation_ordinal_(
42074207
SourceTextModule::kFirstAsyncEvaluationOrdinal),
4208-
cancelable_task_manager_(new CancelableTaskManager())
4208+
cancelable_task_manager_(new CancelableTaskManager()),
42094209
#if defined(V8_ENABLE_ETW_STACK_WALKING)
4210-
,
42114210
etw_tracing_enabled_(false),
42124211
etw_trace_interpreted_frames_(v8_flags.interpreted_frames_native_stack),
4213-
etw_in_rundown_(false)
4212+
etw_in_rundown_(false),
42144213
#endif // V8_ENABLE_ETW_STACK_WALKING
4215-
{
4214+
stack_size_(v8_flags.stack_size * KB) {
42164215
TRACE_ISOLATE(constructor);
42174216
CheckIsolateLayout();
42184217

‎src/execution/isolate.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1749,6 +1749,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
17491749

17501750
bool jitless() const { return jitless_; }
17511751

1752+
void set_stack_size(size_t v) { stack_size_ = v; }
1753+
size_t stack_size() { return stack_size_; }
1754+
17521755
base::RandomNumberGenerator* random_number_generator();
17531756

17541757
base::RandomNumberGenerator* fuzzer_rng();
@@ -2902,6 +2905,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
29022905
// The mutex only guards adding pages, the retrieval is signal safe.
29032906
base::Mutex code_pages_mutex_;
29042907

2908+
// Stack size set with ResourceConstraints or Isolate::SetStackLimit, in
2909+
// bytes. This is initialized with value of --stack-size.
2910+
size_t stack_size_;
29052911
#ifdef V8_ENABLE_WEBASSEMBLY
29062912
wasm::WasmCodeLookupCache* wasm_code_look_up_cache_ = nullptr;
29072913
std::vector<std::unique_ptr<wasm::StackMemory>> wasm_stacks_;

‎src/execution/simulator.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ class SimulatorStack : public v8::internal::AllStatic {
108108
v8::internal::Isolate* isolate) {
109109
uintptr_t upper_bound = base::Stack::GetStackStart();
110110
size_t size =
111-
v8_flags.stack_size * KB + wasm::StackMemory::kJSLimitOffsetKB * KB;
111+
isolate->stack_size() + wasm::StackMemory::kJSLimitOffsetKB * KB;
112112
uintptr_t lower_bound = upper_bound - size;
113113
return base::VectorOf(reinterpret_cast<uint8_t*>(lower_bound), size);
114114
}

‎test/cctest/test-api.cc‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17331,6 +17331,86 @@ TEST(SetStackLimitInThread) {
1733117331
}
1733217332
}
1733317333

17334+
static bool TestStackOverflow(v8::Isolate* isolate) {
17335+
v8::Isolate::Scope isolate_scope(isolate);
17336+
v8::HandleScope scope(isolate);
17337+
LocalContext context(isolate);
17338+
v8::TryCatch try_catch(isolate);
17339+
const char* code =
17340+
"var errored = false;"
17341+
"function fn(...args) {"
17342+
" try { fn(...args); }"
17343+
" catch (e) {"
17344+
// Only trigger GC once the stack is full to speedup the test.
17345+
" if (!errored) {"
17346+
" gc();"
17347+
" errored = true;"
17348+
" }"
17349+
" throw e;"
17350+
" }"
17351+
"}"
17352+
"try {"
17353+
" fn.apply(null, new Array(10).fill(1).map(() => {}));"
17354+
" false;"
17355+
"} catch (e) {"
17356+
" e.name === 'RangeError'" // StackOverflow is a RangeError
17357+
"}";
17358+
Local<Value> value = CompileRun(code);
17359+
17360+
// A StackOverflow error is thrown, without crashing.
17361+
return value->IsTrue();
17362+
}
17363+
17364+
class StackOverflowThread : public v8::base::Thread {
17365+
public:
17366+
explicit StackOverflowThread(int stack_size, int js_stack_size)
17367+
: Thread(Options("StackOverflowThread", stack_size)),
17368+
js_stack_size_(js_stack_size),
17369+
result_(false) {}
17370+
17371+
void Run() override {
17372+
uintptr_t stack_top = v8::base::Stack::GetStackStart();
17373+
// Compute isolate stack limit by js stack size.
17374+
uintptr_t stack_base = stack_top - js_stack_size_;
17375+
v8::Isolate::CreateParams create_params = CreateTestParams();
17376+
v8::Isolate* isolate = v8::Isolate::New(create_params);
17377+
isolate->SetStackLimit(stack_base);
17378+
result_ = TestStackOverflow(isolate);
17379+
isolate->Dispose();
17380+
}
17381+
17382+
int result() { return result_; }
17383+
17384+
private:
17385+
int js_stack_size_;
17386+
bool result_;
17387+
};
17388+
17389+
TEST(SetStackLimitInThreadAndStackOverflow) {
17390+
// Set a small --stack-size flag.
17391+
i::FlagScope<int> f_stack_size(&i::v8_flags.stack_size, 100);
17392+
// Trigger GC aggressively to verify that GC does not crash with stack litmit.
17393+
i::FlagScope<size_t> f_heap_size(&i::v8_flags.max_heap_size, 8);
17394+
i::FlagScope<bool> f_expose_gc(&i::v8_flags.expose_gc, true);
17395+
17396+
// ASAN requires more stack space.
17397+
#ifdef V8_USE_ADDRESS_SANITIZER
17398+
constexpr int stack_size = 32 * v8::internal::MB;
17399+
constexpr int js_stack_size = 2 * v8::internal::MB;
17400+
#else
17401+
constexpr int stack_size = 2 * v8::internal::MB;
17402+
constexpr int js_stack_size = 1 * v8::internal::MB;
17403+
#endif
17404+
// Spawn an Isolate on a thread with larger stack limits than --stack-size.
17405+
StackOverflowThread thread1(stack_size, js_stack_size);
17406+
17407+
CHECK(thread1.Start());
17408+
17409+
thread1.Join();
17410+
17411+
CHECK(thread1.result());
17412+
}
17413+
1733417414
THREADED_TEST(GetHeapStatistics) {
1733517415
LocalContext c1;
1733617416
v8::HandleScope scope(c1->GetIsolate());

0 commit comments

Comments
 (0)