Skip to content

Commit 7e88a93

Browse files
bnoordhuisbnoordhuis
committed
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]>
1 parent 20c4498 commit 7e88a93

File tree

8 files changed

+78
-60
lines changed

8 files changed

+78
-60
lines changed

‎src/env-inl.h

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -153,10 +153,14 @@ inline Environment* Environment::GetCurrent(
153153
return static_cast<Environment*>(info.Data().As<v8::External>()->Value());
154154
}
155155

156+
template <typename T>
156157
inline Environment* Environment::GetCurrent(
157-
const v8::PropertyCallbackInfo<v8::Value>& info) {
158+
const v8::PropertyCallbackInfo<T>& info) {
158159
ASSERT(info.Data()->IsExternal());
159-
return static_cast<Environment*>(info.Data().As<v8::External>()->Value());
160+
// XXX(bnoordhuis) Work around a g++ 4.9.2 template type inferrer bug
161+
// when the expression is written as info.Data().As<v8::External>().
162+
v8::Local<v8::Value> data = info.Data();
163+
return static_cast<Environment*>(data.As<v8::External>()->Value());
160164
}
161165

162166
inline Environment::Environment(v8::Local<v8::Context> context,
@@ -173,6 +177,7 @@ inline Environment::Environment(v8::Local<v8::Context> context,
173177
// We'll be creating new objects so make sure we've entered the context.
174178
v8::HandleScope handle_scope(isolate());
175179
v8::Context::Scope context_scope(context);
180+
set_as_external(v8::External::New(isolate(), this));
176181
set_binding_cache_object(v8::Object::New(isolate()));
177182
set_module_load_list_array(v8::Array::New(isolate()));
178183
RB_INIT(&cares_task_list_);
@@ -396,13 +401,7 @@ inline void Environment::ThrowUVException(int errorno,
396401
inline v8::Local<v8::FunctionTemplate>
397402
Environment::NewFunctionTemplate(v8::FunctionCallback callback,
398403
v8::Local<v8::Signature> signature) {
399-
v8::Local<v8::External> external;
400-
if (external_.IsEmpty()) {
401-
external = v8::External::New(isolate(), this);
402-
external_.Reset(isolate(), external);
403-
} else {
404-
external = StrongPersistentToLocal(external_);
405-
}
404+
v8::Local<v8::External> external = as_external();
406405
return v8::FunctionTemplate::New(isolate(), callback, external, signature);
407406
}
408407

‎src/env.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,7 @@ namespace node {
224224
V(zero_return_string, "ZERO_RETURN") \
225225

226226
#define ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) \
227+
V(as_external, v8::External) \
227228
V(async_hooks_init_function, v8::Function) \
228229
V(async_hooks_pre_function, v8::Function) \
229230
V(async_hooks_post_function, v8::Function) \
@@ -357,8 +358,10 @@ class Environment {
357358
static inline Environment* GetCurrent(v8::Local<v8::Context> context);
358359
static inline Environment* GetCurrent(
359360
const v8::FunctionCallbackInfo<v8::Value>& info);
361+
362+
template <typename T>
360363
static inline Environment* GetCurrent(
361-
const v8::PropertyCallbackInfo<v8::Value>& info);
364+
const v8::PropertyCallbackInfo<T>& info);
362365

363366
// See CreateEnvironment() in src/node.cc.
364367
static inline Environment* New(v8::Local<v8::Context> context,
@@ -509,8 +512,6 @@ class Environment {
509512
&HandleCleanup::handle_cleanup_queue_> handle_cleanup_queue_;
510513
int handle_cleanup_waiting_;
511514

512-
v8::Persistent<v8::External> external_;
513-
514515
#define V(PropertyName, TypeName) \
515516
v8::Persistent<TypeName> PropertyName ## _;
516517
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)

‎src/node.cc

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2280,7 +2280,7 @@ static void LinkedBinding(const FunctionCallbackInfo<Value>& args) {
22802280

22812281
static void ProcessTitleGetter(Local<String> property,
22822282
const PropertyCallbackInfo<Value>& info) {
2283-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2283+
Environment* env = Environment::GetCurrent(info);
22842284
HandleScope scope(env->isolate());
22852285
char buffer[512];
22862286
uv_get_process_title(buffer, sizeof(buffer));
@@ -2291,7 +2291,7 @@ static void ProcessTitleGetter(Local<String> property,
22912291
static void ProcessTitleSetter(Local<String> property,
22922292
Local<Value> value,
22932293
const PropertyCallbackInfo<void>& info) {
2294-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2294+
Environment* env = Environment::GetCurrent(info);
22952295
HandleScope scope(env->isolate());
22962296
node::Utf8Value title(env->isolate(), value);
22972297
// TODO(piscisaureus): protect with a lock
@@ -2301,7 +2301,7 @@ static void ProcessTitleSetter(Local<String> property,
23012301

23022302
static void EnvGetter(Local<String> property,
23032303
const PropertyCallbackInfo<Value>& info) {
2304-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2304+
Environment* env = Environment::GetCurrent(info);
23052305
HandleScope scope(env->isolate());
23062306
#ifdef __POSIX__
23072307
node::Utf8Value key(env->isolate(), property);
@@ -2325,16 +2325,13 @@ static void EnvGetter(Local<String> property,
23252325
return info.GetReturnValue().Set(rc);
23262326
}
23272327
#endif
2328-
// Not found. Fetch from prototype.
2329-
info.GetReturnValue().Set(
2330-
info.Data().As<Object>()->Get(property));
23312328
}
23322329

23332330

23342331
static void EnvSetter(Local<String> property,
23352332
Local<Value> value,
23362333
const PropertyCallbackInfo<Value>& info) {
2337-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2334+
Environment* env = Environment::GetCurrent(info);
23382335
HandleScope scope(env->isolate());
23392336
#ifdef __POSIX__
23402337
node::Utf8Value key(env->isolate(), property);
@@ -2356,7 +2353,7 @@ static void EnvSetter(Local<String> property,
23562353

23572354
static void EnvQuery(Local<String> property,
23582355
const PropertyCallbackInfo<Integer>& info) {
2359-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2356+
Environment* env = Environment::GetCurrent(info);
23602357
HandleScope scope(env->isolate());
23612358
int32_t rc = -1; // Not found unless proven otherwise.
23622359
#ifdef __POSIX__
@@ -2384,7 +2381,7 @@ static void EnvQuery(Local<String> property,
23842381

23852382
static void EnvDeleter(Local<String> property,
23862383
const PropertyCallbackInfo<Boolean>& info) {
2387-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2384+
Environment* env = Environment::GetCurrent(info);
23882385
HandleScope scope(env->isolate());
23892386
bool rc = true;
23902387
#ifdef __POSIX__
@@ -2407,7 +2404,7 @@ static void EnvDeleter(Local<String> property,
24072404

24082405

24092406
static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
2410-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2407+
Environment* env = Environment::GetCurrent(info);
24112408
HandleScope scope(env->isolate());
24122409
#ifdef __POSIX__
24132410
int size = 0;
@@ -2508,7 +2505,7 @@ static Handle<Object> GetFeatures(Environment* env) {
25082505

25092506
static void DebugPortGetter(Local<String> property,
25102507
const PropertyCallbackInfo<Value>& info) {
2511-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2508+
Environment* env = Environment::GetCurrent(info);
25122509
HandleScope scope(env->isolate());
25132510
info.GetReturnValue().Set(debug_port);
25142511
}
@@ -2517,7 +2514,7 @@ static void DebugPortGetter(Local<String> property,
25172514
static void DebugPortSetter(Local<String> property,
25182515
Local<Value> value,
25192516
const PropertyCallbackInfo<void>& info) {
2520-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2517+
Environment* env = Environment::GetCurrent(info);
25212518
HandleScope scope(env->isolate());
25222519
debug_port = value->Int32Value();
25232520
}
@@ -2530,7 +2527,7 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args);
25302527

25312528
void NeedImmediateCallbackGetter(Local<String> property,
25322529
const PropertyCallbackInfo<Value>& info) {
2533-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2530+
Environment* env = Environment::GetCurrent(info);
25342531
const uv_check_t* immediate_check_handle = env->immediate_check_handle();
25352532
bool active = uv_is_active(
25362533
reinterpret_cast<const uv_handle_t*>(immediate_check_handle));
@@ -2543,7 +2540,7 @@ static void NeedImmediateCallbackSetter(
25432540
Local<Value> value,
25442541
const PropertyCallbackInfo<void>& info) {
25452542
HandleScope handle_scope(info.GetIsolate());
2546-
Environment* env = Environment::GetCurrent(info.GetIsolate());
2543+
Environment* env = Environment::GetCurrent(info);
25472544

25482545
uv_check_t* immediate_check_handle = env->immediate_check_handle();
25492546
bool active = uv_is_active(
@@ -2626,7 +2623,8 @@ void SetupProcessObject(Environment* env,
26262623

26272624
process->SetAccessor(env->title_string(),
26282625
ProcessTitleGetter,
2629-
ProcessTitleSetter);
2626+
ProcessTitleSetter,
2627+
env->as_external());
26302628

26312629
// process.version
26322630
READONLY_PROPERTY(process,
@@ -2741,15 +2739,16 @@ void SetupProcessObject(Environment* env,
27412739
EnvQuery,
27422740
EnvDeleter,
27432741
EnvEnumerator,
2744-
Object::New(env->isolate()));
2742+
env->as_external());
27452743
Local<Object> process_env = process_env_template->NewInstance();
27462744
process->Set(env->env_string(), process_env);
27472745

27482746
READONLY_PROPERTY(process, "pid", Integer::New(env->isolate(), getpid()));
27492747
READONLY_PROPERTY(process, "features", GetFeatures(env));
27502748
process->SetAccessor(env->need_imm_cb_string(),
2751-
NeedImmediateCallbackGetter,
2752-
NeedImmediateCallbackSetter);
2749+
NeedImmediateCallbackGetter,
2750+
NeedImmediateCallbackSetter,
2751+
env->as_external());
27532752

27542753
// -e, --eval
27552754
if (eval_string) {
@@ -2812,7 +2811,8 @@ void SetupProcessObject(Environment* env,
28122811

28132812
process->SetAccessor(env->debug_port_string(),
28142813
DebugPortGetter,
2815-
DebugPortSetter);
2814+
DebugPortSetter,
2815+
env->as_external());
28162816

28172817
// define various internal methods
28182818
env->SetMethod(process,

‎src/node_crypto.cc

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,8 @@ namespace crypto {
6060
using v8::Array;
6161
using v8::Boolean;
6262
using v8::Context;
63+
using v8::DEFAULT;
64+
using v8::DontDelete;
6365
using v8::EscapableHandleScope;
6466
using v8::Exception;
6567
using v8::External;
@@ -76,6 +78,7 @@ using v8::Object;
7678
using v8::Persistent;
7779
using v8::PropertyAttribute;
7880
using v8::PropertyCallbackInfo;
81+
using v8::ReadOnly;
7982
using v8::String;
8083
using v8::V8;
8184
using v8::Value;
@@ -264,10 +267,13 @@ void SecureContext::Initialize(Environment* env, Handle<Object> target) {
264267
env->SetProtoMethod(t, "getCertificate", SecureContext::GetCertificate<true>);
265268
env->SetProtoMethod(t, "getIssuer", SecureContext::GetCertificate<false>);
266269

267-
NODE_SET_EXTERNAL(
268-
t->PrototypeTemplate(),
269-
"_external",
270-
CtxGetter);
270+
t->PrototypeTemplate()->SetAccessor(
271+
FIXED_ONE_BYTE_STRING(env->isolate(), "_external"),
272+
CtxGetter,
273+
nullptr,
274+
env->as_external(),
275+
DEFAULT,
276+
static_cast<PropertyAttribute>(ReadOnly | DontDelete));
271277

272278
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "SecureContext"),
273279
t->GetFunction());
@@ -991,10 +997,13 @@ void SSLWrap<Base>::AddMethods(Environment* env, Handle<FunctionTemplate> t) {
991997
env->SetProtoMethod(t, "setNPNProtocols", SetNPNProtocols);
992998
#endif
993999

994-
NODE_SET_EXTERNAL(
995-
t->PrototypeTemplate(),
996-
"_external",
997-
SSLGetter);
1000+
t->PrototypeTemplate()->SetAccessor(
1001+
FIXED_ONE_BYTE_STRING(env->isolate(), "_external"),
1002+
SSLGetter,
1003+
nullptr,
1004+
env->as_external(),
1005+
DEFAULT,
1006+
static_cast<PropertyAttribute>(ReadOnly | DontDelete));
9981007
}
9991008

10001009

@@ -3652,8 +3661,8 @@ void DiffieHellman::Initialize(Environment* env, Handle<Object> target) {
36523661
t->InstanceTemplate()->SetAccessor(env->verify_error_string(),
36533662
DiffieHellman::VerifyErrorGetter,
36543663
nullptr,
3655-
Handle<Value>(),
3656-
v8::DEFAULT,
3664+
env->as_external(),
3665+
DEFAULT,
36573666
attributes);
36583667

36593668
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellman"),
@@ -3672,8 +3681,8 @@ void DiffieHellman::Initialize(Environment* env, Handle<Object> target) {
36723681
t2->InstanceTemplate()->SetAccessor(env->verify_error_string(),
36733682
DiffieHellman::VerifyErrorGetter,
36743683
nullptr,
3675-
Handle<Value>(),
3676-
v8::DEFAULT,
3684+
env->as_external(),
3685+
DEFAULT,
36773686
attributes);
36783687

36793688
target->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "DiffieHellmanGroup"),

‎src/node_internals.h

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -195,21 +195,6 @@ NODE_DEPRECATED("Use ThrowUVException(isolate)",
195195
return ThrowUVException(isolate, errorno, syscall, message, path);
196196
})
197197

198-
inline void NODE_SET_EXTERNAL(v8::Handle<v8::ObjectTemplate> target,
199-
const char* key,
200-
v8::AccessorGetterCallback getter) {
201-
v8::Isolate* isolate = v8::Isolate::GetCurrent();
202-
v8::HandleScope handle_scope(isolate);
203-
v8::Local<v8::String> prop = v8::String::NewFromUtf8(isolate, key);
204-
target->SetAccessor(prop,
205-
getter,
206-
nullptr,
207-
v8::Handle<v8::Value>(),
208-
v8::DEFAULT,
209-
static_cast<v8::PropertyAttribute>(v8::ReadOnly |
210-
v8::DontDelete));
211-
}
212-
213198
enum NodeInstanceType { MAIN, WORKER };
214199

215200
class NodeInstanceData {

‎src/stream_base-inl.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ void StreamBase::AddMethods(Environment* env,
3232
t->InstanceTemplate()->SetAccessor(env->fd_string(),
3333
GetFD<Base>,
3434
nullptr,
35-
Handle<Value>(),
35+
env->as_external(),
3636
v8::DEFAULT,
3737
attributes);
3838

‎src/udp_wrap.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ void UDPWrap::Initialize(Handle<Object> target,
8484
t->InstanceTemplate()->SetAccessor(env->fd_string(),
8585
UDPWrap::GetFD,
8686
nullptr,
87-
Handle<Value>(),
87+
env->as_external(),
8888
v8::DEFAULT,
8989
attributes);
9090

‎test/parallel/test-vm-debug-context.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,30 @@ assert.strictEqual(vm.runInDebugContext(0), 0);
2727
assert.strictEqual(vm.runInDebugContext(null), null);
2828
assert.strictEqual(vm.runInDebugContext(undefined), undefined);
2929

30+
// See https://github.com/iojs/io.js/issues/1190, accessing named interceptors
31+
// and accessors inside a debug event listener should not crash.
32+
(function() {
33+
var Debug = vm.runInDebugContext('Debug');
34+
var breaks = 0;
35+
36+
function ondebugevent(evt, exc) {
37+
if (evt !== Debug.DebugEvent.Break) return;
38+
exc.frame(0).evaluate('process.env').properties(); // Named interceptor.
39+
exc.frame(0).evaluate('process.title').getTruncatedValue(); // Accessor.
40+
breaks += 1;
41+
}
42+
43+
function breakpoint() {
44+
debugger;
45+
}
46+
47+
assert.equal(breaks, 0);
48+
Debug.setListener(ondebugevent);
49+
assert.equal(breaks, 0);
50+
breakpoint();
51+
assert.equal(breaks, 1);
52+
})();
53+
3054
// See https://github.com/iojs/io.js/issues/1190, fatal errors should not
3155
// crash the process.
3256
var script = common.fixturesDir + '/vm-run-in-debug-context.js';

0 commit comments

Comments
 (0)