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: #1190 (again)
PR-URL: #1238
Reviewed-By: Fedor Indutny <[email protected]>

src/env-inl.h

@@ -153,10 +153,14 @@ inline Environment* Environment::GetCurrent(
   return static_cast<Environment*>(info.Data().As<v8::External>()->Value());
 }
 
+template <typename T>
 inline Environment* Environment::GetCurrent(
-    const v8::PropertyCallbackInfo<v8::Value>& info) {
+    const v8::PropertyCallbackInfo<T>& info) {
   ASSERT(info.Data()->IsExternal());
-  return static_cast<Environment*>(info.Data().As<v8::External>()->Value());
+  // XXX(bnoordhuis) Work around a g++ 4.9.2 template type inferrer bug
+  // when the expression is written as info.Data().As<v8::External>().
+  v8::Local<v8::Value> data = info.Data();
+  return static_cast<Environment*>(data.As<v8::External>()->Value());
 }
 
 inline Environment::Environment(v8::Local<v8::Context> context,
@@ -173,6 +177,7 @@ inline Environment::Environment(v8::Local<v8::Context> 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<v8::FunctionTemplate>
     Environment::NewFunctionTemplate(v8::FunctionCallback callback,
                                      v8::Local<v8::Signature> signature) {
-  v8::Local<v8::External> external;
-  if (external_.IsEmpty()) {
-    external = v8::External::New(isolate(), this);
-    external_.Reset(isolate(), external);
-  } else {
-    external = StrongPersistentToLocal(external_);
-  }
+  v8::Local<v8::External> external = as_external();
   return v8::FunctionTemplate::New(isolate(), callback, external, signature);
 }
 

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<v8::Context> context);
   static inline Environment* GetCurrent(
       const v8::FunctionCallbackInfo<v8::Value>& info);
+
+  template <typename T>
   static inline Environment* GetCurrent(
-      const v8::PropertyCallbackInfo<v8::Value>& info);
+      const v8::PropertyCallbackInfo<T>& info);
 
   // See CreateEnvironment() in src/node.cc.
   static inline Environment* New(v8::Local<v8::Context> context,
@@ -509,8 +512,6 @@ class Environment {
            &HandleCleanup::handle_cleanup_queue_> handle_cleanup_queue_;
   int handle_cleanup_waiting_;
 
-  v8::Persistent<v8::External> external_;
-
 #define V(PropertyName, TypeName)                                             \
   v8::Persistent<TypeName> PropertyName ## _;
   ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)

src/node.cc

@@ -2280,7 +2280,7 @@ static void LinkedBinding(const FunctionCallbackInfo<Value>& args) {
 
 static void ProcessTitleGetter(Local<String> property,
                                const PropertyCallbackInfo<Value>& 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<String> property,
 static void ProcessTitleSetter(Local<String> property,
                                Local<Value> value,
                                const PropertyCallbackInfo<void>& 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<String> property,
 
 static void EnvGetter(Local<String> property,
                       const PropertyCallbackInfo<Value>& 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<String> property,
     return info.GetReturnValue().Set(rc);
   }
 #endif
-  // Not found.  Fetch from prototype.
-  info.GetReturnValue().Set(
-      info.Data().As<Object>()->Get(property));
 }
 
 
 static void EnvSetter(Local<String> property,
                       Local<Value> value,
                       const PropertyCallbackInfo<Value>& 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<String> property,
 
 static void EnvQuery(Local<String> property,
                      const PropertyCallbackInfo<Integer>& 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<String> property,
 
 static void EnvDeleter(Local<String> property,
                        const PropertyCallbackInfo<Boolean>& 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<String> property,
 
 
 static void EnvEnumerator(const PropertyCallbackInfo<Array>& 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<Object> GetFeatures(Environment* env) {
 
 static void DebugPortGetter(Local<String> property,
                             const PropertyCallbackInfo<Value>& 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<String> property,
 static void DebugPortSetter(Local<String> property,
                             Local<Value> value,
                             const PropertyCallbackInfo<void>& 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<Value>& args);
 
 void NeedImmediateCallbackGetter(Local<String> property,
                                  const PropertyCallbackInfo<Value>& 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<const uv_handle_t*>(immediate_check_handle));
@@ -2543,7 +2540,7 @@ static void NeedImmediateCallbackSetter(
     Local<Value> value,
     const PropertyCallbackInfo<void>& 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<Object> 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,

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<Object> target) {
   env->SetProtoMethod(t, "getCertificate", SecureContext::GetCertificate<true>);
   env->SetProtoMethod(t, "getIssuer", SecureContext::GetCertificate<false>);
 
-  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<PropertyAttribute>(ReadOnly | DontDelete));
 
   target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "SecureContext"),
               t->GetFunction());
@@ -991,10 +997,13 @@ void SSLWrap<Base>::AddMethods(Environment* env, Handle<FunctionTemplate> 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<PropertyAttribute>(ReadOnly | DontDelete));
 }
 
 
@@ -3652,8 +3661,8 @@ void DiffieHellman::Initialize(Environment* env, Handle<Object> target) {
   t->InstanceTemplate()->SetAccessor(env->verify_error_string(),
                                      DiffieHellman::VerifyErrorGetter,
                                      nullptr,
-                                     Handle<Value>(),
-                                     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<Object> target) {
   t2->InstanceTemplate()->SetAccessor(env->verify_error_string(),
                                       DiffieHellman::VerifyErrorGetter,
                                       nullptr,
-                                      Handle<Value>(),
-                                      v8::DEFAULT,
+                                      env->as_external(),
+                                      DEFAULT,
                                       attributes);
 
   target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellmanGroup"),

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<v8::ObjectTemplate> target,
-                              const char* key,
-                              v8::AccessorGetterCallback getter) {
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  v8::HandleScope handle_scope(isolate);
-  v8::Local<v8::String> prop = v8::String::NewFromUtf8(isolate, key);
-  target->SetAccessor(prop,
-                      getter,
-                      nullptr,
-                      v8::Handle<v8::Value>(),
-                      v8::DEFAULT,
-                      static_cast<v8::PropertyAttribute>(v8::ReadOnly |
-                                                         v8::DontDelete));
-}
-
 enum NodeInstanceType { MAIN, WORKER };
 
 class NodeInstanceData {

src/stream_base-inl.h

@@ -32,7 +32,7 @@ void StreamBase::AddMethods(Environment* env,
   t->InstanceTemplate()->SetAccessor(env->fd_string(),
                                      GetFD<Base>,
                                      nullptr,
-                                     Handle<Value>(),
+                                     env->as_external(),
                                      v8::DEFAULT,
                                      attributes);
 

src/udp_wrap.cc

@@ -84,7 +84,7 @@ void UDPWrap::Initialize(Handle<Object> target,
   t->InstanceTemplate()->SetAccessor(env->fd_string(),
                                      UDPWrap::GetFD,
                                      nullptr,
-                                     Handle<Value>(),
+                                     env->as_external(),
                                      v8::DEFAULT,
                                      attributes);
 

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';