From 7e88a9322c8f1b5393723d6f99590d750b097569 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 23 Mar 2015 00:26:59 +0100 Subject: [PATCH] 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 d3a723a0c2..237da7159d 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 cad03c4531..027c79dfae 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 b8decf8c39..f47f31ab1f 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 a650d4fef7..b6cb4f10e5 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 9141355df6..c99b2feeb0 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 8f7f5fea41..26ba54b376 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 d57809cb1b..3183d1f9f5 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 8f0a2742f0..da2a4474e0 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';