From 7e88a9322c8f1b5393723d6f99590d750b097569 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 23 Mar 2015 00:26:59 +0100 Subject: [PATCH 1/2] src: make accessors immune to context confusion It's possible for an accessor or named interceptor to get called with a different execution context than the one it lives in, see the test case for an example using the debug API. This commit fortifies against that by passing the environment as a data property instead of looking it up through the current context. Fixes: https://github.com/iojs/io.js/issues/1190 (again) PR-URL: https://github.com/iojs/io.js/pull/1238 Reviewed-By: Fedor Indutny --- src/env-inl.h | 17 ++++++------ src/env.h | 7 +++-- src/node.cc | 38 +++++++++++++------------- src/node_crypto.cc | 33 ++++++++++++++-------- src/node_internals.h | 15 ---------- src/stream_base-inl.h | 2 +- src/udp_wrap.cc | 2 +- test/parallel/test-vm-debug-context.js | 24 ++++++++++++++++ 8 files changed, 78 insertions(+), 60 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index d3a723a0c246e5..237da7159d1ecf 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -153,10 +153,14 @@ inline Environment* Environment::GetCurrent( return static_cast(info.Data().As()->Value()); } +template inline Environment* Environment::GetCurrent( - const v8::PropertyCallbackInfo& info) { + const v8::PropertyCallbackInfo& info) { ASSERT(info.Data()->IsExternal()); - return static_cast(info.Data().As()->Value()); + // XXX(bnoordhuis) Work around a g++ 4.9.2 template type inferrer bug + // when the expression is written as info.Data().As(). + v8::Local data = info.Data(); + return static_cast(data.As()->Value()); } inline Environment::Environment(v8::Local context, @@ -173,6 +177,7 @@ inline Environment::Environment(v8::Local context, // We'll be creating new objects so make sure we've entered the context. v8::HandleScope handle_scope(isolate()); v8::Context::Scope context_scope(context); + set_as_external(v8::External::New(isolate(), this)); set_binding_cache_object(v8::Object::New(isolate())); set_module_load_list_array(v8::Array::New(isolate())); RB_INIT(&cares_task_list_); @@ -396,13 +401,7 @@ inline void Environment::ThrowUVException(int errorno, inline v8::Local Environment::NewFunctionTemplate(v8::FunctionCallback callback, v8::Local signature) { - v8::Local external; - if (external_.IsEmpty()) { - external = v8::External::New(isolate(), this); - external_.Reset(isolate(), external); - } else { - external = StrongPersistentToLocal(external_); - } + v8::Local external = as_external(); return v8::FunctionTemplate::New(isolate(), callback, external, signature); } diff --git a/src/env.h b/src/env.h index cad03c45310467..027c79dfae8510 100644 --- a/src/env.h +++ b/src/env.h @@ -224,6 +224,7 @@ namespace node { V(zero_return_string, "ZERO_RETURN") \ #define ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) \ + V(as_external, v8::External) \ V(async_hooks_init_function, v8::Function) \ V(async_hooks_pre_function, v8::Function) \ V(async_hooks_post_function, v8::Function) \ @@ -357,8 +358,10 @@ class Environment { static inline Environment* GetCurrent(v8::Local context); static inline Environment* GetCurrent( const v8::FunctionCallbackInfo& info); + + template static inline Environment* GetCurrent( - const v8::PropertyCallbackInfo& info); + const v8::PropertyCallbackInfo& info); // See CreateEnvironment() in src/node.cc. static inline Environment* New(v8::Local context, @@ -509,8 +512,6 @@ class Environment { &HandleCleanup::handle_cleanup_queue_> handle_cleanup_queue_; int handle_cleanup_waiting_; - v8::Persistent external_; - #define V(PropertyName, TypeName) \ v8::Persistent PropertyName ## _; ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) diff --git a/src/node.cc b/src/node.cc index b8decf8c3975bb..f47f31ab1fb30a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2280,7 +2280,7 @@ static void LinkedBinding(const FunctionCallbackInfo& args) { static void ProcessTitleGetter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); char buffer[512]; uv_get_process_title(buffer, sizeof(buffer)); @@ -2291,7 +2291,7 @@ static void ProcessTitleGetter(Local property, static void ProcessTitleSetter(Local property, Local value, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); node::Utf8Value title(env->isolate(), value); // TODO(piscisaureus): protect with a lock @@ -2301,7 +2301,7 @@ static void ProcessTitleSetter(Local property, static void EnvGetter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); #ifdef __POSIX__ node::Utf8Value key(env->isolate(), property); @@ -2325,16 +2325,13 @@ static void EnvGetter(Local property, return info.GetReturnValue().Set(rc); } #endif - // Not found. Fetch from prototype. - info.GetReturnValue().Set( - info.Data().As()->Get(property)); } static void EnvSetter(Local property, Local value, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); #ifdef __POSIX__ node::Utf8Value key(env->isolate(), property); @@ -2356,7 +2353,7 @@ static void EnvSetter(Local property, static void EnvQuery(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); int32_t rc = -1; // Not found unless proven otherwise. #ifdef __POSIX__ @@ -2384,7 +2381,7 @@ static void EnvQuery(Local property, static void EnvDeleter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); bool rc = true; #ifdef __POSIX__ @@ -2407,7 +2404,7 @@ static void EnvDeleter(Local property, static void EnvEnumerator(const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); #ifdef __POSIX__ int size = 0; @@ -2508,7 +2505,7 @@ static Handle GetFeatures(Environment* env) { static void DebugPortGetter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); info.GetReturnValue().Set(debug_port); } @@ -2517,7 +2514,7 @@ static void DebugPortGetter(Local property, static void DebugPortSetter(Local property, Local value, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); HandleScope scope(env->isolate()); debug_port = value->Int32Value(); } @@ -2530,7 +2527,7 @@ static void DebugEnd(const FunctionCallbackInfo& args); void NeedImmediateCallbackGetter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); const uv_check_t* immediate_check_handle = env->immediate_check_handle(); bool active = uv_is_active( reinterpret_cast(immediate_check_handle)); @@ -2543,7 +2540,7 @@ static void NeedImmediateCallbackSetter( Local value, const PropertyCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); - Environment* env = Environment::GetCurrent(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); uv_check_t* immediate_check_handle = env->immediate_check_handle(); bool active = uv_is_active( @@ -2626,7 +2623,8 @@ void SetupProcessObject(Environment* env, process->SetAccessor(env->title_string(), ProcessTitleGetter, - ProcessTitleSetter); + ProcessTitleSetter, + env->as_external()); // process.version READONLY_PROPERTY(process, @@ -2741,15 +2739,16 @@ void SetupProcessObject(Environment* env, EnvQuery, EnvDeleter, EnvEnumerator, - Object::New(env->isolate())); + env->as_external()); Local process_env = process_env_template->NewInstance(); process->Set(env->env_string(), process_env); READONLY_PROPERTY(process, "pid", Integer::New(env->isolate(), getpid())); READONLY_PROPERTY(process, "features", GetFeatures(env)); process->SetAccessor(env->need_imm_cb_string(), - NeedImmediateCallbackGetter, - NeedImmediateCallbackSetter); + NeedImmediateCallbackGetter, + NeedImmediateCallbackSetter, + env->as_external()); // -e, --eval if (eval_string) { @@ -2812,7 +2811,8 @@ void SetupProcessObject(Environment* env, process->SetAccessor(env->debug_port_string(), DebugPortGetter, - DebugPortSetter); + DebugPortSetter, + env->as_external()); // define various internal methods env->SetMethod(process, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index a650d4fef70cbd..b6cb4f10e5698f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -60,6 +60,8 @@ namespace crypto { using v8::Array; using v8::Boolean; using v8::Context; +using v8::DEFAULT; +using v8::DontDelete; using v8::EscapableHandleScope; using v8::Exception; using v8::External; @@ -76,6 +78,7 @@ using v8::Object; using v8::Persistent; using v8::PropertyAttribute; using v8::PropertyCallbackInfo; +using v8::ReadOnly; using v8::String; using v8::V8; using v8::Value; @@ -264,10 +267,13 @@ void SecureContext::Initialize(Environment* env, Handle target) { env->SetProtoMethod(t, "getCertificate", SecureContext::GetCertificate); env->SetProtoMethod(t, "getIssuer", SecureContext::GetCertificate); - NODE_SET_EXTERNAL( - t->PrototypeTemplate(), - "_external", - CtxGetter); + t->PrototypeTemplate()->SetAccessor( + FIXED_ONE_BYTE_STRING(env->isolate(), "_external"), + CtxGetter, + nullptr, + env->as_external(), + DEFAULT, + static_cast(ReadOnly | DontDelete)); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "SecureContext"), t->GetFunction()); @@ -991,10 +997,13 @@ void SSLWrap::AddMethods(Environment* env, Handle t) { env->SetProtoMethod(t, "setNPNProtocols", SetNPNProtocols); #endif - NODE_SET_EXTERNAL( - t->PrototypeTemplate(), - "_external", - SSLGetter); + t->PrototypeTemplate()->SetAccessor( + FIXED_ONE_BYTE_STRING(env->isolate(), "_external"), + SSLGetter, + nullptr, + env->as_external(), + DEFAULT, + static_cast(ReadOnly | DontDelete)); } @@ -3652,8 +3661,8 @@ void DiffieHellman::Initialize(Environment* env, Handle target) { t->InstanceTemplate()->SetAccessor(env->verify_error_string(), DiffieHellman::VerifyErrorGetter, nullptr, - Handle(), - v8::DEFAULT, + env->as_external(), + DEFAULT, attributes); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellman"), @@ -3672,8 +3681,8 @@ void DiffieHellman::Initialize(Environment* env, Handle target) { t2->InstanceTemplate()->SetAccessor(env->verify_error_string(), DiffieHellman::VerifyErrorGetter, nullptr, - Handle(), - v8::DEFAULT, + env->as_external(), + DEFAULT, attributes); target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellmanGroup"), diff --git a/src/node_internals.h b/src/node_internals.h index 9141355df68aa9..c99b2feeb0bdcd 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -195,21 +195,6 @@ NODE_DEPRECATED("Use ThrowUVException(isolate)", return ThrowUVException(isolate, errorno, syscall, message, path); }) -inline void NODE_SET_EXTERNAL(v8::Handle target, - const char* key, - v8::AccessorGetterCallback getter) { - v8::Isolate* isolate = v8::Isolate::GetCurrent(); - v8::HandleScope handle_scope(isolate); - v8::Local prop = v8::String::NewFromUtf8(isolate, key); - target->SetAccessor(prop, - getter, - nullptr, - v8::Handle(), - v8::DEFAULT, - static_cast(v8::ReadOnly | - v8::DontDelete)); -} - enum NodeInstanceType { MAIN, WORKER }; class NodeInstanceData { diff --git a/src/stream_base-inl.h b/src/stream_base-inl.h index 8f7f5fea413393..26ba54b3768afc 100644 --- a/src/stream_base-inl.h +++ b/src/stream_base-inl.h @@ -32,7 +32,7 @@ void StreamBase::AddMethods(Environment* env, t->InstanceTemplate()->SetAccessor(env->fd_string(), GetFD, nullptr, - Handle(), + env->as_external(), v8::DEFAULT, attributes); diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index d57809cb1b405f..3183d1f9f5367c 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -84,7 +84,7 @@ void UDPWrap::Initialize(Handle target, t->InstanceTemplate()->SetAccessor(env->fd_string(), UDPWrap::GetFD, nullptr, - Handle(), + env->as_external(), v8::DEFAULT, attributes); diff --git a/test/parallel/test-vm-debug-context.js b/test/parallel/test-vm-debug-context.js index 8f0a2742f0dbca..da2a4474e044cb 100644 --- a/test/parallel/test-vm-debug-context.js +++ b/test/parallel/test-vm-debug-context.js @@ -27,6 +27,30 @@ assert.strictEqual(vm.runInDebugContext(0), 0); assert.strictEqual(vm.runInDebugContext(null), null); assert.strictEqual(vm.runInDebugContext(undefined), undefined); +// See https://github.com/iojs/io.js/issues/1190, accessing named interceptors +// and accessors inside a debug event listener should not crash. +(function() { + var Debug = vm.runInDebugContext('Debug'); + var breaks = 0; + + function ondebugevent(evt, exc) { + if (evt !== Debug.DebugEvent.Break) return; + exc.frame(0).evaluate('process.env').properties(); // Named interceptor. + exc.frame(0).evaluate('process.title').getTruncatedValue(); // Accessor. + breaks += 1; + } + + function breakpoint() { + debugger; + } + + assert.equal(breaks, 0); + Debug.setListener(ondebugevent); + assert.equal(breaks, 0); + breakpoint(); + assert.equal(breaks, 1); +})(); + // See https://github.com/iojs/io.js/issues/1190, fatal errors should not // crash the process. var script = common.fixturesDir + '/vm-run-in-debug-context.js'; From 2e5b87a147f0bb1e92de4a62f392bb4f3ac12f8f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 23 Mar 2015 00:38:42 +0100 Subject: [PATCH 2/2] src: remove unnecessary environment lookups Remove some unnecessary environment lookups and delete a few superfluous HandleScope variables. PR-URL: https://github.com/iojs/io.js/pull/1238 Reviewed-By: Fedor Indutny --- src/node.cc | 47 +++++++++++++++-------------------------------- 1 file changed, 15 insertions(+), 32 deletions(-) diff --git a/src/node.cc b/src/node.cc index f47f31ab1fb30a..92ace4fe3ec8b7 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2280,20 +2280,16 @@ static void LinkedBinding(const FunctionCallbackInfo& args) { static void ProcessTitleGetter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); char buffer[512]; uv_get_process_title(buffer, sizeof(buffer)); - info.GetReturnValue().Set(String::NewFromUtf8(env->isolate(), buffer)); + info.GetReturnValue().Set(String::NewFromUtf8(info.GetIsolate(), buffer)); } static void ProcessTitleSetter(Local property, Local value, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); - node::Utf8Value title(env->isolate(), value); + node::Utf8Value title(info.GetIsolate(), value); // TODO(piscisaureus): protect with a lock uv_set_process_title(*title); } @@ -2301,13 +2297,12 @@ static void ProcessTitleSetter(Local property, static void EnvGetter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); + Isolate* isolate = info.GetIsolate(); #ifdef __POSIX__ - node::Utf8Value key(env->isolate(), property); + node::Utf8Value key(isolate, property); const char* val = getenv(*key); if (val) { - return info.GetReturnValue().Set(String::NewFromUtf8(env->isolate(), val)); + return info.GetReturnValue().Set(String::NewFromUtf8(isolate, val)); } #else // _WIN32 String::Value key(property); @@ -2321,7 +2316,7 @@ static void EnvGetter(Local property, if ((result > 0 || GetLastError() == ERROR_SUCCESS) && result < ARRAY_SIZE(buffer)) { const uint16_t* two_byte_buffer = reinterpret_cast(buffer); - Local rc = String::NewFromTwoByte(env->isolate(), two_byte_buffer); + Local rc = String::NewFromTwoByte(isolate, two_byte_buffer); return info.GetReturnValue().Set(rc); } #endif @@ -2331,11 +2326,9 @@ static void EnvGetter(Local property, static void EnvSetter(Local property, Local value, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); #ifdef __POSIX__ - node::Utf8Value key(env->isolate(), property); - node::Utf8Value val(env->isolate(), value); + node::Utf8Value key(info.GetIsolate(), property); + node::Utf8Value val(info.GetIsolate(), value); setenv(*key, *val, 1); #else // _WIN32 String::Value key(property); @@ -2353,11 +2346,9 @@ static void EnvSetter(Local property, static void EnvQuery(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); int32_t rc = -1; // Not found unless proven otherwise. #ifdef __POSIX__ - node::Utf8Value key(env->isolate(), property); + node::Utf8Value key(info.GetIsolate(), property); if (getenv(*key)) rc = 0; #else // _WIN32 @@ -2381,11 +2372,9 @@ static void EnvQuery(Local property, static void EnvDeleter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); bool rc = true; #ifdef __POSIX__ - node::Utf8Value key(env->isolate(), property); + node::Utf8Value key(info.GetIsolate(), property); rc = getenv(*key) != nullptr; if (rc) unsetenv(*key); @@ -2404,20 +2393,19 @@ static void EnvDeleter(Local property, static void EnvEnumerator(const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); + Isolate* isolate = info.GetIsolate(); #ifdef __POSIX__ int size = 0; while (environ[size]) size++; - Local envarr = Array::New(env->isolate(), size); + Local envarr = Array::New(isolate, size); for (int i = 0; i < size; ++i) { const char* var = environ[i]; const char* s = strchr(var, '='); const int length = s ? s - var : strlen(var); - Local name = String::NewFromUtf8(env->isolate(), + Local name = String::NewFromUtf8(isolate, var, String::kNormalString, length); @@ -2427,7 +2415,7 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { WCHAR* environment = GetEnvironmentStringsW(); if (environment == nullptr) return; // This should not happen. - Local envarr = Array::New(env->isolate()); + Local envarr = Array::New(isolate); WCHAR* p = environment; int i = 0; while (*p) { @@ -2444,7 +2432,7 @@ static void EnvEnumerator(const PropertyCallbackInfo& info) { } const uint16_t* two_byte_buffer = reinterpret_cast(p); const size_t two_byte_buffer_len = s - p; - Local value = String::NewFromTwoByte(env->isolate(), + Local value = String::NewFromTwoByte(isolate, two_byte_buffer, String::kNormalString, two_byte_buffer_len); @@ -2505,8 +2493,6 @@ static Handle GetFeatures(Environment* env) { static void DebugPortGetter(Local property, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); info.GetReturnValue().Set(debug_port); } @@ -2514,8 +2500,6 @@ static void DebugPortGetter(Local property, static void DebugPortSetter(Local property, Local value, const PropertyCallbackInfo& info) { - Environment* env = Environment::GetCurrent(info); - HandleScope scope(env->isolate()); debug_port = value->Int32Value(); } @@ -2539,7 +2523,6 @@ static void NeedImmediateCallbackSetter( Local property, Local value, const PropertyCallbackInfo& info) { - HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); uv_check_t* immediate_check_handle = env->immediate_check_handle();