From 2e32a750712d8ed1a623bd72728d6c478b1290e7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 12 Sep 2018 15:01:50 +0200 Subject: [PATCH] src: refactor `Environment::GetCurrent()` usage Make `Environment::GetCurrent()` return `nullptr` if the current `Context` is not a Node.js context, and for the relevant usage of this function, either: - Switch to the better `GetCurrent(args)` variant - Turn functions in to no-ops where it makes sense - Make it a `CHECK`, i.e. an API requirement, where it make sense - Leave a `TODO` comment for verifying what, if anything, is to be done PR-URL: https://github.com/nodejs/node/pull/22819 Reviewed-By: Gus Caplan Reviewed-By: Gireesh Punathil Reviewed-By: Eugene Ostroukhov Reviewed-By: Refael Ackermann --- src/async_wrap.cc | 9 +++++++-- src/bootstrapper.cc | 1 + src/callback_scope.cc | 1 + src/env-inl.h | 9 +++++++++ src/env.cc | 13 ++----------- src/inspector_js_api.cc | 5 ++--- src/module_wrap.cc | 6 ++++-- src/node.cc | 17 +++++++++++++---- src/node_api.cc | 13 +++++++++---- src/node_buffer.cc | 7 ++++++- src/node_context_data.h | 5 ----- src/node_file.cc | 9 ++++----- src/node_internals.h | 4 +++- src/node_perf.cc | 1 + test/cctest/test_environment.cc | 20 ++++++++++++++++++++ 15 files changed, 82 insertions(+), 38 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 7f2a0ece2f0055..a8a4601e76ec43 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -681,12 +681,16 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) { - return Environment::GetCurrent(isolate)->execution_async_id(); + Environment* env = Environment::GetCurrent(isolate); + if (env == nullptr) return -1; + return env->execution_async_id(); } async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) { - return Environment::GetCurrent(isolate)->trigger_async_id(); + Environment* env = Environment::GetCurrent(isolate); + if (env == nullptr) return -1; + return env->trigger_async_id(); } @@ -705,6 +709,7 @@ async_context EmitAsyncInit(Isolate* isolate, v8::Local name, async_id trigger_async_id) { Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); // Initialize async context struct if (trigger_async_id == -1) diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 94bb9413772dd0..4994e0d90a9cd0 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -61,6 +61,7 @@ void PromiseRejectCallback(PromiseRejectMessage message) { PromiseRejectEvent event = message.GetEvent(); Environment* env = Environment::GetCurrent(isolate); + if (env == nullptr) return; Local callback; Local value; diff --git a/src/callback_scope.cc b/src/callback_scope.cc index 8b407474b333c6..9d0c4d5daf2627 100644 --- a/src/callback_scope.cc +++ b/src/callback_scope.cc @@ -43,6 +43,7 @@ InternalCallbackScope::InternalCallbackScope(Environment* env, object_(object), callback_scope_(env) { CHECK_IMPLIES(expect == kRequireResource, !object.IsEmpty()); + CHECK_NOT_NULL(env); if (!env->can_call_into_js()) { failed_ = true; diff --git a/src/env-inl.h b/src/env-inl.h index e1a3809f6721fa..4e0a95ed424cd9 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -302,6 +302,15 @@ inline Environment* Environment::GetCurrent(v8::Isolate* isolate) { } inline Environment* Environment::GetCurrent(v8::Local context) { + if (UNLIKELY(context.IsEmpty() || + context->GetNumberOfEmbedderDataFields() < + ContextEmbedderIndex::kContextTag || + context->GetAlignedPointerFromEmbedderData( + ContextEmbedderIndex::kContextTag) != + Environment::kNodeContextTagPtr)) { + return nullptr; + } + return static_cast( context->GetAlignedPointerFromEmbedderData( ContextEmbedderIndex::kEnvironment)); diff --git a/src/env.cc b/src/env.cc index 0d3bfc808d43dc..7356f04db38c95 100644 --- a/src/env.cc +++ b/src/env.cc @@ -449,18 +449,9 @@ void Environment::EnvPromiseHook(v8::PromiseHookType type, v8::Local parent) { Local context = promise->CreationContext(); - // Grow the embedder data if necessary to make sure we are not out of bounds - // when reading the magic number. - context->SetAlignedPointerInEmbedderData( - ContextEmbedderIndex::kContextTagBoundary, nullptr); - int* magicNumberPtr = reinterpret_cast( - context->GetAlignedPointerFromEmbedderData( - ContextEmbedderIndex::kContextTag)); - if (magicNumberPtr != Environment::kNodeContextTagPtr) { - return; - } - Environment* env = Environment::GetCurrent(context); + if (env == nullptr) return; + for (const PromiseHookCallback& hook : env->promise_hooks_) { hook.cb_(type, promise, parent, hook.arg_); } diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 18b9e610750e93..54b7c4d01c6d3b 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -149,12 +149,11 @@ void CallAndPauseOnStart(const FunctionCallbackInfo& args) { } void InspectorConsoleCall(const FunctionCallbackInfo& info) { - Isolate* isolate = info.GetIsolate(); - HandleScope handle_scope(isolate); + Environment* env = Environment::GetCurrent(info); + Isolate* isolate = env->isolate(); Local context = isolate->GetCurrentContext(); CHECK_LT(2, info.Length()); SlicedArguments call_args(info, /* start */ 3); - Environment* env = Environment::GetCurrent(isolate); if (InspectorEnabled(env)) { Local inspector_method = info[0]; CHECK(inspector_method->IsFunction()); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index d584a5a3f9bc0d..5249ccb8dccd28 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -362,6 +362,7 @@ MaybeLocal ModuleWrap::ResolveCallback(Local context, Local specifier, Local referrer) { Environment* env = Environment::GetCurrent(context); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. Isolate* isolate = env->isolate(); if (env->module_map.count(referrer->GetIdentityHash()) == 0) { env->ThrowError("linking error, unknown module"); @@ -700,6 +701,7 @@ static MaybeLocal ImportModuleDynamically( Local specifier) { Isolate* iso = context->GetIsolate(); Environment* env = Environment::GetCurrent(context); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. v8::EscapableHandleScope handle_scope(iso); if (env->context() != context) { @@ -750,8 +752,8 @@ void ModuleWrap::SetImportModuleDynamicallyCallback( void ModuleWrap::HostInitializeImportMetaObjectCallback( Local context, Local module, Local meta) { - Isolate* isolate = context->GetIsolate(); Environment* env = Environment::GetCurrent(context); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. ModuleWrap* module_wrap = GetFromModule(env, module); if (module_wrap == nullptr) { @@ -762,7 +764,7 @@ void ModuleWrap::HostInitializeImportMetaObjectCallback( Local callback = env->host_initialize_import_meta_object_callback(); Local args[] = { wrap, meta }; - callback->Call(context, Undefined(isolate), arraysize(args), args) + callback->Call(context, Undefined(env->isolate()), arraysize(args), args) .ToLocalChecked(); } diff --git a/src/node.cc b/src/node.cc index 2576f016c1477a..657822b68bad63 100644 --- a/src/node.cc +++ b/src/node.cc @@ -630,7 +630,8 @@ namespace { bool ShouldAbortOnUncaughtException(Isolate* isolate) { HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); - return env->should_abort_on_uncaught_toggle()[0] && + return env != nullptr && + env->should_abort_on_uncaught_toggle()[0] && !env->inside_should_not_abort_on_uncaught_scope(); } @@ -639,6 +640,7 @@ bool ShouldAbortOnUncaughtException(Isolate* isolate) { void AddPromiseHook(Isolate* isolate, promise_hook_func fn, void* arg) { Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); env->AddPromiseHook(fn, arg); } @@ -646,6 +648,7 @@ void AddEnvironmentCleanupHook(Isolate* isolate, void (*fun)(void* arg), void* arg) { Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); env->AddCleanupHook(fun, arg); } @@ -654,6 +657,7 @@ void RemoveEnvironmentCleanupHook(Isolate* isolate, void (*fun)(void* arg), void* arg) { Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); env->RemoveCleanupHook(fun, arg); } @@ -738,6 +742,7 @@ MaybeLocal MakeCallback(Isolate* isolate, // Because of the AssignToContext() call in src/node_contextify.cc, // the two contexts need not be the same. Environment* env = Environment::GetCurrent(callback->CreationContext()); + CHECK_NOT_NULL(env); Context::Scope context_scope(env->context()); MaybeLocal ret = InternalMakeCallback(env, recv, callback, argc, argv, asyncContext); @@ -1376,6 +1381,7 @@ void FatalException(Isolate* isolate, HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. Local process_object = env->process_object(); Local fatal_exception_string = env->fatal_exception_string(); Local fatal_exception_function = @@ -1601,7 +1607,7 @@ static void GetInternalBinding(const FunctionCallbackInfo& args) { } static void GetLinkedBinding(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args.GetIsolate()); + Environment* env = Environment::GetCurrent(args); CHECK(args[0]->IsString()); @@ -2705,10 +2711,13 @@ void RunAtExit(Environment* env) { uv_loop_t* GetCurrentEventLoop(Isolate* isolate) { HandleScope handle_scope(isolate); - auto context = isolate->GetCurrentContext(); + Local context = isolate->GetCurrentContext(); if (context.IsEmpty()) return nullptr; - return Environment::GetCurrent(context)->event_loop(); + Environment* env = Environment::GetCurrent(context); + if (env == nullptr) + return nullptr; + return env->event_loop(); } diff --git a/src/node_api.cc b/src/node_api.cc index 690579f573bdf2..3dd5b383310a03 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -946,7 +946,7 @@ class ThreadSafeFunction : public node::AsyncResource { return napi_ok; } - node::Environment::GetCurrent(env->isolate)->CloseHandle( + NodeEnv()->CloseHandle( reinterpret_cast(&async), [](uv_handle_t* handle) -> void { ThreadSafeFunction* ts_fn = @@ -1036,9 +1036,12 @@ class ThreadSafeFunction : public node::AsyncResource { } node::Environment* NodeEnv() { - // For some reason grabbing the Node.js environment requires a handle scope. + // Grabbing the Node.js environment requires a handle scope because it + // looks up fields on the current context. v8::HandleScope scope(env->isolate); - return node::Environment::GetCurrent(env->isolate); + node::Environment* node_env = node::Environment::GetCurrent(env->isolate); + CHECK_NOT_NULL(node_env); + return node_env; } void MaybeStartIdle() { @@ -1234,7 +1237,9 @@ void napi_module_register_by_symbol(v8::Local exports, v8::Local context, napi_addon_register_func init) { if (init == nullptr) { - node::Environment::GetCurrent(context)->ThrowError( + node::Environment* node_env = node::Environment::GetCurrent(context); + CHECK_NOT_NULL(node_env); + node_env->ThrowError( "Module has no declared entry point."); return; } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 465c42047911b0..dd285156b5644b 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -274,7 +274,9 @@ MaybeLocal New(Isolate* isolate, MaybeLocal New(Isolate* isolate, size_t length) { EscapableHandleScope handle_scope(isolate); Local obj; - if (Buffer::New(Environment::GetCurrent(isolate), length).ToLocal(&obj)) + Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. + if (Buffer::New(env, length).ToLocal(&obj)) return handle_scope.Escape(obj); return Local(); } @@ -316,6 +318,7 @@ MaybeLocal New(Environment* env, size_t length) { MaybeLocal Copy(Isolate* isolate, const char* data, size_t length) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. Local obj; if (Buffer::Copy(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); @@ -365,6 +368,7 @@ MaybeLocal New(Isolate* isolate, void* hint) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. Local obj; if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj)) return handle_scope.Escape(obj); @@ -403,6 +407,7 @@ MaybeLocal New(Environment* env, MaybeLocal New(Isolate* isolate, char* data, size_t length) { EscapableHandleScope handle_scope(isolate); Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); // TODO(addaleax): Handle nullptr here. Local obj; if (Buffer::New(env, data, length).ToLocal(&obj)) return handle_scope.Escape(obj); diff --git a/src/node_context_data.h b/src/node_context_data.h index 61ab81523cea59..807ba56c7c69fb 100644 --- a/src/node_context_data.h +++ b/src/node_context_data.h @@ -25,16 +25,11 @@ namespace node { #define NODE_CONTEXT_TAG 35 #endif -#ifndef NODE_CONTEXT_TAG_BOUNDARY -#define NODE_CONTEXT_TAG_BOUNDARY 36 -#endif - enum ContextEmbedderIndex { kEnvironment = NODE_CONTEXT_EMBEDDER_DATA_INDEX, kSandboxObject = NODE_CONTEXT_SANDBOX_OBJECT_INDEX, kAllowWasmCodeGeneration = NODE_CONTEXT_ALLOW_WASM_CODE_GENERATION_INDEX, kContextTag = NODE_CONTEXT_TAG, - kContextTagBoundary = NODE_CONTEXT_TAG_BOUNDARY, }; } // namespace node diff --git a/src/node_file.cc b/src/node_file.cc index d1c8abfc1e9e12..8490dbec023799 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -441,7 +441,7 @@ void FSReqWrap::SetReturnValue(const FunctionCallbackInfo& args) { void NewFSReqWrap(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); - Environment* env = Environment::GetCurrent(args.GetIsolate()); + Environment* env = Environment::GetCurrent(args); new FSReqWrap(env, args.This(), args[0]->IsTrue()); } @@ -782,7 +782,7 @@ inline FSReqBase* GetReqWrap(Environment* env, Local value, } void Access(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args.GetIsolate()); + Environment* env = Environment::GetCurrent(args); HandleScope scope(env->isolate()); const int argc = args.Length(); @@ -2233,9 +2233,8 @@ void Initialize(Local target, StatWatcher::Initialize(env, target); - // Create FunctionTemplate for FSReqWrap - Local fst = - FunctionTemplate::New(env->isolate(), NewFSReqWrap); + // Create FunctionTemplate for FSReqCallback + Local fst = env->NewFunctionTemplate(NewFSReqWrap); fst->InstanceTemplate()->SetInternalFieldCount(1); AsyncWrap::AddWrapMethods(env, fst); Local wrapString = diff --git a/src/node_internals.h b/src/node_internals.h index 39be3f14a7c82e..129cfddb46853f 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -501,7 +501,9 @@ class InternalCallbackScope { class ThreadPoolWork { public: - explicit inline ThreadPoolWork(Environment* env) : env_(env) {} + explicit inline ThreadPoolWork(Environment* env) : env_(env) { + CHECK_NOT_NULL(env); + } inline virtual ~ThreadPoolWork() = default; inline void ScheduleWork(); diff --git a/src/node_perf.cc b/src/node_perf.cc index cedb924e65eb98..828a1dd7ff4bde 100644 --- a/src/node_perf.cc +++ b/src/node_perf.cc @@ -310,6 +310,7 @@ void TimerFunctionCall(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); HandleScope scope(isolate); Environment* env = Environment::GetCurrent(isolate); + CHECK_NOT_NULL(env); // TODO(addaleax): Verify that this is correct. Local context = env->context(); Local fn = args.Data().As(); size_t count = args.Length(); diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 07170ac267adea..fa0f94a5add9bd 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -70,6 +70,26 @@ TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) { EXPECT_TRUE(called_cb_2); } +TEST_F(EnvironmentTest, NonNodeJSContext) { + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env test_env {handle_scope, argv}; + + EXPECT_EQ(node::Environment::GetCurrent(v8::Local()), nullptr); + + node::Environment* env = *test_env; + EXPECT_EQ(node::Environment::GetCurrent(isolate_), env); + EXPECT_EQ(node::Environment::GetCurrent(env->context()), env); + + v8::Local context = v8::Context::New(isolate_); + EXPECT_EQ(node::Environment::GetCurrent(context), nullptr); + EXPECT_EQ(node::Environment::GetCurrent(isolate_), env); + + v8::Context::Scope context_scope(context); + EXPECT_EQ(node::Environment::GetCurrent(context), nullptr); + EXPECT_EQ(node::Environment::GetCurrent(isolate_), nullptr); +} + static void at_exit_callback1(void* arg) { called_cb_1 = true; if (arg) {