lukechilds
/
node
mirror of https://github.com/lukechilds/node.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

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 <fedor@indutny.com>
v1.8.0-commit
Ben Noordhuis 10 years ago
parent
20c4498e76
commit
7e88a9322c
8 changed files with 78 additions and 60 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Unified View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 17
      src/env-inl.h
  2. 7
      src/env.h
  3. 36
      src/node.cc
  4. 33
      src/node_crypto.cc
  5. 15
      src/node_internals.h
  6. 2
      src/stream_base-inl.h
  7. 2
      src/udp_wrap.cc
  8. 24
      test/parallel/test-vm-debug-context.js

17
src/env-inl.h
View File

@ -153,10 +153,14 @@ inline Environment* Environment::GetCurrent(
return static_cast<Environment*>(info.Data().As<v8::External>()->Value()); return static_cast<Environment*>(info.Data().As<v8::External>()->Value());
} }
template <typename T>
inline Environment* Environment::GetCurrent( inline Environment* Environment::GetCurrent(
const v8::PropertyCallbackInfo<v8::Value>& info) { const v8::PropertyCallbackInfo<T>& info) {
ASSERT(info.Data()->IsExternal()); 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, 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. // We'll be creating new objects so make sure we've entered the context.
v8::HandleScope handle_scope(isolate()); v8::HandleScope handle_scope(isolate());
v8::Context::Scope context_scope(context); v8::Context::Scope context_scope(context);
set_as_external(v8::External::New(isolate(), this));
set_binding_cache_object(v8::Object::New(isolate())); set_binding_cache_object(v8::Object::New(isolate()));
set_module_load_list_array(v8::Array::New(isolate())); set_module_load_list_array(v8::Array::New(isolate()));
RB_INIT(&cares_task_list_); RB_INIT(&cares_task_list_);
@ -396,13 +401,7 @@ inline void Environment::ThrowUVException(int errorno,
inline v8::Local<v8::FunctionTemplate> inline v8::Local<v8::FunctionTemplate>
Environment::NewFunctionTemplate(v8::FunctionCallback callback, Environment::NewFunctionTemplate(v8::FunctionCallback callback,
v8::Local<v8::Signature> signature) { v8::Local<v8::Signature> signature) {
v8::Local<v8::External> external; v8::Local<v8::External> external = as_external();
if (external_.IsEmpty()) {
external = v8::External::New(isolate(), this);
external_.Reset(isolate(), external);
} else {
external = StrongPersistentToLocal(external_);
}
return v8::FunctionTemplate::New(isolate(), callback, external, signature); return v8::FunctionTemplate::New(isolate(), callback, external, signature);
} }

7
src/env.h
View File

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

36
src/node.cc
View File

@ -2280,7 +2280,7 @@ static void LinkedBinding(const FunctionCallbackInfo<Value>& args) {
static void ProcessTitleGetter(Local<String> property, static void ProcessTitleGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) { const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
char buffer[512]; char buffer[512];
uv_get_process_title(buffer, sizeof(buffer)); uv_get_process_title(buffer, sizeof(buffer));
@ -2291,7 +2291,7 @@ static void ProcessTitleGetter(Local<String> property,
static void ProcessTitleSetter(Local<String> property, static void ProcessTitleSetter(Local<String> property,
Local<Value> value, Local<Value> value,
const PropertyCallbackInfo<void>& info) { const PropertyCallbackInfo<void>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
node::Utf8Value title(env->isolate(), value); node::Utf8Value title(env->isolate(), value);
// TODO(piscisaureus): protect with a lock // TODO(piscisaureus): protect with a lock
@ -2301,7 +2301,7 @@ static void ProcessTitleSetter(Local<String> property,
static void EnvGetter(Local<String> property, static void EnvGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) { const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
#ifdef __POSIX__ #ifdef __POSIX__
node::Utf8Value key(env->isolate(), property); node::Utf8Value key(env->isolate(), property);
@ -2325,16 +2325,13 @@ static void EnvGetter(Local<String> property,
return info.GetReturnValue().Set(rc); return info.GetReturnValue().Set(rc);
} }
#endif #endif
// Not found. Fetch from prototype.
info.GetReturnValue().Set(
info.Data().As<Object>()->Get(property));
} }
static void EnvSetter(Local<String> property, static void EnvSetter(Local<String> property,
Local<Value> value, Local<Value> value,
const PropertyCallbackInfo<Value>& info) { const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
#ifdef __POSIX__ #ifdef __POSIX__
node::Utf8Value key(env->isolate(), property); node::Utf8Value key(env->isolate(), property);
@ -2356,7 +2353,7 @@ static void EnvSetter(Local<String> property,
static void EnvQuery(Local<String> property, static void EnvQuery(Local<String> property,
const PropertyCallbackInfo<Integer>& info) { const PropertyCallbackInfo<Integer>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
int32_t rc = -1; // Not found unless proven otherwise. int32_t rc = -1; // Not found unless proven otherwise.
#ifdef __POSIX__ #ifdef __POSIX__
@ -2384,7 +2381,7 @@ static void EnvQuery(Local<String> property,
static void EnvDeleter(Local<String> property, static void EnvDeleter(Local<String> property,
const PropertyCallbackInfo<Boolean>& info) { const PropertyCallbackInfo<Boolean>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
bool rc = true; bool rc = true;
#ifdef __POSIX__ #ifdef __POSIX__
@ -2407,7 +2404,7 @@ static void EnvDeleter(Local<String> property,
static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) { static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
#ifdef __POSIX__ #ifdef __POSIX__
int size = 0; int size = 0;
@ -2508,7 +2505,7 @@ static Handle<Object> GetFeatures(Environment* env) {
static void DebugPortGetter(Local<String> property, static void DebugPortGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) { const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
info.GetReturnValue().Set(debug_port); info.GetReturnValue().Set(debug_port);
} }
@ -2517,7 +2514,7 @@ static void DebugPortGetter(Local<String> property,
static void DebugPortSetter(Local<String> property, static void DebugPortSetter(Local<String> property,
Local<Value> value, Local<Value> value,
const PropertyCallbackInfo<void>& info) { const PropertyCallbackInfo<void>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate()); Environment* env = Environment::GetCurrent(info);
HandleScope scope(env->isolate()); HandleScope scope(env->isolate());
debug_port = value->Int32Value(); debug_port = value->Int32Value();
} }
@ -2530,7 +2527,7 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args);
void NeedImmediateCallbackGetter(Local<String> property, void NeedImmediateCallbackGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) { 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(); const uv_check_t* immediate_check_handle = env->immediate_check_handle();
bool active = uv_is_active( bool active = uv_is_active(
reinterpret_cast<const uv_handle_t*>(immediate_check_handle)); reinterpret_cast<const uv_handle_t*>(immediate_check_handle));
@ -2543,7 +2540,7 @@ static void NeedImmediateCallbackSetter(
Local<Value> value, Local<Value> value,
const PropertyCallbackInfo<void>& info) { const PropertyCallbackInfo<void>& info) {
HandleScope handle_scope(info.GetIsolate()); 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(); uv_check_t* immediate_check_handle = env->immediate_check_handle();
bool active = uv_is_active( bool active = uv_is_active(
@ -2626,7 +2623,8 @@ void SetupProcessObject(Environment* env,
process->SetAccessor(env->title_string(), process->SetAccessor(env->title_string(),
ProcessTitleGetter, ProcessTitleGetter,
ProcessTitleSetter); ProcessTitleSetter,
env->as_external());
// process.version // process.version
READONLY_PROPERTY(process, READONLY_PROPERTY(process,
@ -2741,7 +2739,7 @@ void SetupProcessObject(Environment* env,
EnvQuery, EnvQuery,
EnvDeleter, EnvDeleter,
EnvEnumerator, EnvEnumerator,
Object::New(env->isolate())); env->as_external());
Local<Object> process_env = process_env_template->NewInstance(); Local<Object> process_env = process_env_template->NewInstance();
process->Set(env->env_string(), process_env); process->Set(env->env_string(), process_env);
@ -2749,7 +2747,8 @@ void SetupProcessObject(Environment* env,
READONLY_PROPERTY(process, "features", GetFeatures(env)); READONLY_PROPERTY(process, "features", GetFeatures(env));
process->SetAccessor(env->need_imm_cb_string(), process->SetAccessor(env->need_imm_cb_string(),
NeedImmediateCallbackGetter, NeedImmediateCallbackGetter,
NeedImmediateCallbackSetter); NeedImmediateCallbackSetter,
env->as_external());
// -e, --eval // -e, --eval
if (eval_string) { if (eval_string) {
@ -2812,7 +2811,8 @@ void SetupProcessObject(Environment* env,
process->SetAccessor(env->debug_port_string(), process->SetAccessor(env->debug_port_string(),
DebugPortGetter, DebugPortGetter,
DebugPortSetter); DebugPortSetter,
env->as_external());
// define various internal methods // define various internal methods
env->SetMethod(process, env->SetMethod(process,

33
src/node_crypto.cc
View File

@ -60,6 +60,8 @@ namespace crypto {
using v8::Array; using v8::Array;
using v8::Boolean; using v8::Boolean;
using v8::Context; using v8::Context;
using v8::DEFAULT;
using v8::DontDelete;
using v8::EscapableHandleScope; using v8::EscapableHandleScope;
using v8::Exception; using v8::Exception;
using v8::External; using v8::External;
@ -76,6 +78,7 @@ using v8::Object;
using v8::Persistent; using v8::Persistent;
using v8::PropertyAttribute; using v8::PropertyAttribute;
using v8::PropertyCallbackInfo; using v8::PropertyCallbackInfo;
using v8::ReadOnly;
using v8::String; using v8::String;
using v8::V8; using v8::V8;
using v8::Value; 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, "getCertificate", SecureContext::GetCertificate<true>);
env->SetProtoMethod(t, "getIssuer", SecureContext::GetCertificate<false>); env->SetProtoMethod(t, "getIssuer", SecureContext::GetCertificate<false>);
NODE_SET_EXTERNAL( t->PrototypeTemplate()->SetAccessor(
t->PrototypeTemplate(), FIXED_ONE_BYTE_STRING(env->isolate(), "_external"),
"_external", CtxGetter,
CtxGetter); nullptr,
env->as_external(),
DEFAULT,
static_cast<PropertyAttribute>(ReadOnly | DontDelete));
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "SecureContext"), target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "SecureContext"),
t->GetFunction()); t->GetFunction());
@ -991,10 +997,13 @@ void SSLWrap<Base>::AddMethods(Environment* env, Handle<FunctionTemplate> t) {
env->SetProtoMethod(t, "setNPNProtocols", SetNPNProtocols); env->SetProtoMethod(t, "setNPNProtocols", SetNPNProtocols);
#endif #endif
NODE_SET_EXTERNAL( t->PrototypeTemplate()->SetAccessor(
t->PrototypeTemplate(), FIXED_ONE_BYTE_STRING(env->isolate(), "_external"),
"_external", SSLGetter,
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(), t->InstanceTemplate()->SetAccessor(env->verify_error_string(),
DiffieHellman::VerifyErrorGetter, DiffieHellman::VerifyErrorGetter,
nullptr, nullptr,
Handle<Value>(), env->as_external(),
v8::DEFAULT, DEFAULT,
attributes); attributes);
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellman"), 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(), t2->InstanceTemplate()->SetAccessor(env->verify_error_string(),
DiffieHellman::VerifyErrorGetter, DiffieHellman::VerifyErrorGetter,
nullptr, nullptr,
Handle<Value>(), env->as_external(),
v8::DEFAULT, DEFAULT,
attributes); attributes);
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellmanGroup"), target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellmanGroup"),

15
src/node_internals.h
View File

@ -195,21 +195,6 @@ NODE_DEPRECATED("Use ThrowUVException(isolate)",
return ThrowUVException(isolate, errorno, syscall, message, path); 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 }; enum NodeInstanceType { MAIN, WORKER };
class NodeInstanceData { class NodeInstanceData {

2
src/stream_base-inl.h
View File

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

2
src/udp_wrap.cc
View File

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

24
test/parallel/test-vm-debug-context.js
View File

@ -27,6 +27,30 @@ assert.strictEqual(vm.runInDebugContext(0), 0);
assert.strictEqual(vm.runInDebugContext(null), null); assert.strictEqual(vm.runInDebugContext(null), null);
assert.strictEqual(vm.runInDebugContext(undefined), undefined); 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 // See https://github.com/iojs/io.js/issues/1190, fatal errors should not
// crash the process. // crash the process.
var script = common.fixturesDir + '/vm-run-in-debug-context.js'; var script = common.fixturesDir + '/vm-run-in-debug-context.js';

Write Preview
Loading…
Cancel
Save