Skip to content

Commit

Permalink
src: make accessors immune to context confusion
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@bnoordhuis
bnoordhuis committed Mar 23, 2015
1 parent 20c4498 commit 7e88a93
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 60 deletions.
17 changes: 8 additions & 9 deletions src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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_);
Expand Down Expand Up @@ -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);
}

Expand Down
7 changes: 4 additions & 3 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down
38 changes: 19 additions & 19 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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__
Expand Down Expand Up @@ -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__
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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();
}
Expand All @@ -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));
Expand All @@ -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(
Expand Down Expand Up @@ -2626,7 +2623,8 @@ void SetupProcessObject(Environment* env,

process->SetAccessor(env->title_string(),
ProcessTitleGetter,
ProcessTitleSetter);
ProcessTitleSetter,
env->as_external());

// process.version
READONLY_PROPERTY(process,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
33 changes: 21 additions & 12 deletions src/node_crypto.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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));
}


Expand Down Expand Up @@ -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"),
Expand All @@ -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"),
Expand Down
15 changes: 0 additions & 15 deletions src/node_internals.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/stream_base-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion src/udp_wrap.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
24 changes: 24 additions & 0 deletions test/parallel/test-vm-debug-context.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down

0 comments on commit 7e88a93

Please sign in to comment.