Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: make accessors immune to context confusion #1238

Merged
merged 2 commits into from
Mar 23, 2015
Merged

src: make accessors immune to context confusion #1238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e88a93
src: make accessors immune to context confusion
bnoordhuis Mar 22, 2015
2e5b87a
src: remove unnecessary environment lookups
bnoordhuis Mar 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/env-inl.h
View file
Open in desktop
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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like this is actually needed anywhere. Is it just a new feature?

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
Open in desktop
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
67 changes: 25 additions & 42 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2280,34 +2280,29 @@ static void LinkedBinding(const FunctionCallbackInfo<Value>& args) {

static void ProcessTitleGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
char buffer[512];
uv_get_process_title(buffer, sizeof(buffer));
info.GetReturnValue().Set(String::NewFromUtf8(env->isolate(), buffer));
info.GetReturnValue().Set(String::NewFromUtf8(info.GetIsolate(), buffer));
}


static void ProcessTitleSetter(Local<String> property,
Local<Value> value,
const PropertyCallbackInfo<void>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
node::Utf8Value title(env->isolate(), value);
node::Utf8Value title(info.GetIsolate(), value);
// TODO(piscisaureus): protect with a lock
uv_set_process_title(*title);
}


static void EnvGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
Isolate* isolate = info.GetIsolate();
#ifdef __POSIX__
node::Utf8Value key(env->isolate(), property);
node::Utf8Value key(isolate, property);
const char* val = getenv(*key);
if (val) {
return info.GetReturnValue().Set(String::NewFromUtf8(env->isolate(), val));
return info.GetReturnValue().Set(String::NewFromUtf8(isolate, val));
}
#else // _WIN32
String::Value key(property);
Expand All @@ -2321,24 +2316,19 @@ static void EnvGetter(Local<String> property,
if ((result > 0 || GetLastError() == ERROR_SUCCESS) &&
result < ARRAY_SIZE(buffer)) {
const uint16_t* two_byte_buffer = reinterpret_cast<const uint16_t*>(buffer);
Local<String> rc = String::NewFromTwoByte(env->isolate(), two_byte_buffer);
Local<String> rc = String::NewFromTwoByte(isolate, two_byte_buffer);
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());
HandleScope scope(env->isolate());
#ifdef __POSIX__
node::Utf8Value key(env->isolate(), property);
node::Utf8Value val(env->isolate(), value);
node::Utf8Value key(info.GetIsolate(), property);
node::Utf8Value val(info.GetIsolate(), value);
setenv(*key, *val, 1);
#else // _WIN32
String::Value key(property);
Expand All @@ -2356,11 +2346,9 @@ static void EnvSetter(Local<String> property,

static void EnvQuery(Local<String> property,
const PropertyCallbackInfo<Integer>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
int32_t rc = -1; // Not found unless proven otherwise.
#ifdef __POSIX__
node::Utf8Value key(env->isolate(), property);
node::Utf8Value key(info.GetIsolate(), property);
if (getenv(*key))
rc = 0;
#else // _WIN32
Expand All @@ -2384,11 +2372,9 @@ static void EnvQuery(Local<String> property,

static void EnvDeleter(Local<String> property,
const PropertyCallbackInfo<Boolean>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
bool rc = true;
#ifdef __POSIX__
node::Utf8Value key(env->isolate(), property);
node::Utf8Value key(info.GetIsolate(), property);
rc = getenv(*key) != nullptr;
if (rc)
unsetenv(*key);
Expand All @@ -2407,20 +2393,19 @@ static void EnvDeleter(Local<String> property,


static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
Isolate* isolate = info.GetIsolate();
#ifdef __POSIX__
int size = 0;
while (environ[size])
size++;

Local<Array> envarr = Array::New(env->isolate(), size);
Local<Array> envarr = Array::New(isolate, size);

for (int i = 0; i < size; ++i) {
const char* var = environ[i];
const char* s = strchr(var, '=');
const int length = s ? s - var : strlen(var);
Local<String> name = String::NewFromUtf8(env->isolate(),
Local<String> name = String::NewFromUtf8(isolate,
var,
String::kNormalString,
length);
Expand All @@ -2430,7 +2415,7 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
WCHAR* environment = GetEnvironmentStringsW();
if (environment == nullptr)
return; // This should not happen.
Local<Array> envarr = Array::New(env->isolate());
Local<Array> envarr = Array::New(isolate);
WCHAR* p = environment;
int i = 0;
while (*p) {
Expand All @@ -2447,7 +2432,7 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
}
const uint16_t* two_byte_buffer = reinterpret_cast<const uint16_t*>(p);
const size_t two_byte_buffer_len = s - p;
Local<String> value = String::NewFromTwoByte(env->isolate(),
Local<String> value = String::NewFromTwoByte(isolate,
two_byte_buffer,
String::kNormalString,
two_byte_buffer_len);
Expand Down Expand Up @@ -2508,17 +2493,13 @@ static Handle<Object> GetFeatures(Environment* env) {

static void DebugPortGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
info.GetReturnValue().Set(debug_port);
}


static void DebugPortSetter(Local<String> property,
Local<Value> value,
const PropertyCallbackInfo<void>& info) {
Environment* env = Environment::GetCurrent(info.GetIsolate());
HandleScope scope(env->isolate());
debug_port = value->Int32Value();
}

Expand All @@ -2530,7 +2511,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 @@ -2542,8 +2523,7 @@ static void NeedImmediateCallbackSetter(
Local<String> property,
Local<Value> value,
const PropertyCallbackInfo<void>& info) {
HandleScope handle_scope(info.GetIsolate());
Environment* env = Environment::GetCurrent(info.GetIsolate());
Environment* env = Environment::GetCurrent(info);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nvm. I see now.


uv_check_t* immediate_check_handle = env->immediate_check_handle();
bool active = uv_is_active(
Expand Down Expand Up @@ -2626,7 +2606,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 +2722,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 +2794,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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading