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

[DO NOT LAND] src: invalidate Utf8/TwoByteValue upon error #11952

Closed
wants to merge 7 commits into from
Closed

[DO NOT LAND] src: invalidate Utf8/TwoByteValue upon error #11952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dd40f5a
child_process: move args stringification to JS
TimothyGu Mar 20, 2017
e47b0d3
src: invalidate Utf8/TwoByteValue in case of error
TimothyGu Mar 20, 2017
360cbae
src: add Context-based Value class constructors
TimothyGu Mar 20, 2017
af4f5f0
url: check Utf8/TwoByteValue validity
TimothyGu Mar 20, 2017
27e49e0
i18n: check Utf8Value validity
TimothyGu Mar 21, 2017
a7857a0
src, env: check Utf8Value validity
TimothyGu Mar 20, 2017
b5df96f
node: check Utf8Value validity
TimothyGu Mar 21, 2017
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
2 changes: 2 additions & 0 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,8 @@ function normalizeSpawnArguments(file, args, options) {

if (Array.isArray(args)) {
args = args.slice(0);
for (var i = 0; i < args.length; i++)
args[i] = `${args[i]}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just args[i] += ''; ?

Copy link
Member Author

@TimothyGu TimothyGu Apr 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

That calls args[i][Symbol.toPrimitive]() (if there is one) with 'default' instead of 'string', thus breaking compatibility.

} else if (args !== undefined &&
(args === null || typeof args !== 'object')) {
throw new TypeError('Incorrect value of args option');
Expand Down
12 changes: 8 additions & 4 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ void Environment::PrintSyncTrace() const {
const int column = stack_frame->GetColumn();

if (stack_frame->IsEval()) {
if (stack_frame->GetScriptId() == Message::kNoScriptIdInfo) {
if (script_name.IsInvalidated() ||
stack_frame->GetScriptId() == Message::kNoScriptIdInfo) {
fprintf(stderr, " at [eval]:%i:%i\n", line_number, column);
} else {
fprintf(stderr,
Expand All @@ -139,13 +140,16 @@ void Environment::PrintSyncTrace() const {
break;
}

if (fn_name_s.length() == 0) {
fprintf(stderr, " at %s:%i:%i\n", *script_name, line_number, column);
if (fn_name_s.IsInvalidated() || fn_name_s.length() == 0) {
fprintf(stderr,
" at %s:%i:%i\n",
script_name.IsInvalidated() ? "<anonymous>" : *script_name,
line_number, column);
} else {
fprintf(stderr,
" at %s (%s:%i:%i)\n",
*fn_name_s,
*script_name,
script_name.IsInvalidated() ? "<anonymous>" : *script_name,
line_number,
column);
}
Expand Down
67 changes: 60 additions & 7 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1485,6 +1485,8 @@ enum encoding ParseEncoding(Isolate* isolate,
return default_encoding;

node::Utf8Value encoding(isolate, encoding_v);
if (encoding.IsInvalidated())
return default_encoding;

return ParseEncoding(*encoding, default_encoding);
}
Expand Down Expand Up @@ -1555,10 +1557,14 @@ void AppendExceptionLine(Environment* env,

// Print (filename):(line number): (message).
node::Utf8Value filename(env->isolate(), message->GetScriptResourceName());
if (filename.IsInvalidated())
return;
const char* filename_string = *filename;
int linenum = message->GetLineNumber();
// Print line of source code.
node::Utf8Value sourceline(env->isolate(), message->GetSourceLine());
if (sourceline.IsInvalidated())
return;
const char* sourceline_string = *sourceline;

// Because of how node modules work, all scripts are wrapped with a
Expand Down Expand Up @@ -1667,13 +1673,18 @@ static void ReportException(Environment* env,
}

node::Utf8Value trace(env->isolate(), trace_value);
if (trace.IsInvalidated()) {
// Something is wrong; abort.
return;
}

// range errors have a trace member set to undefined
if (trace.length() > 0 && !trace_value->IsUndefined()) {
if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
PrintErrorString("%s\n", *trace);
} else {
node::Utf8Value arrow_string(env->isolate(), arrow);
CHECK(!arrow_string.IsInvalidated());
PrintErrorString("%s\n%s\n", *arrow_string, *trace);
}
} else {
Expand All @@ -1699,17 +1710,25 @@ static void ReportException(Environment* env,
PrintErrorString("%s\n", *message ? *message :
"<toString() threw exception>");
} else {
node::Utf8Value name_string(env->isolate(), name);
node::Utf8Value message_string(env->isolate(), message);
const char* name_string = "<toString() threw exception>";
node::Utf8Value name_val(env->isolate(), name);
if (!name_val.IsInvalidated())
name_string = *name_val;

const char* message_string = "<toString() threw exception>";
node::Utf8Value message_val(env->isolate(), message);
if (!message_val.IsInvalidated())
message_string = *message_val;

if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
PrintErrorString("%s: %s\n", *name_string, *message_string);
PrintErrorString("%s: %s\n", name_string, message_string);
} else {
node::Utf8Value arrow_string(env->isolate(), arrow);
node::Utf8Value arrow_val(env->isolate(), arrow);
CHECK(!arrow_val.IsInvalidated());
PrintErrorString("%s\n%s: %s\n",
*arrow_string,
*name_string,
*message_string);
*arrow_val,
name_string,
message_string);
}
}
}
Expand Down Expand Up @@ -1858,6 +1877,8 @@ static void Chdir(const FunctionCallbackInfo<Value>& args) {
}

node::Utf8Value path(args.GetIsolate(), args[0]);
CHECK(!path.IsInvalidated());

int err = uv_chdir(*path);
if (err) {
return env->ThrowUVException(err, "uv_chdir");
Expand Down Expand Up @@ -1904,6 +1925,7 @@ static void Umask(const FunctionCallbackInfo<Value>& args) {
} else {
oct = 0;
node::Utf8Value str(env->isolate(), args[0]);
CHECK(!str.IsInvalidated());

// Parse the octal string.
for (size_t i = 0; i < str.length(); i++) {
Expand Down Expand Up @@ -2011,6 +2033,8 @@ static uid_t uid_by_name(Isolate* isolate, Local<Value> value) {
return static_cast<uid_t>(value->Uint32Value());
} else {
node::Utf8Value name(isolate, value);
if (name.IsInvalidated())
return uid_not_found;
return uid_by_name(*name);
}
}
Expand All @@ -2021,6 +2045,8 @@ static gid_t gid_by_name(Isolate* isolate, Local<Value> value) {
return static_cast<gid_t>(value->Uint32Value());
} else {
node::Utf8Value name(isolate, value);
if (name.IsInvalidated())
return gid_not_found;
return gid_by_name(*name);
}
}
Expand Down Expand Up @@ -2206,6 +2232,9 @@ static void InitGroups(const FunctionCallbackInfo<Value>& args) {
}

node::Utf8Value arg0(env->isolate(), args[0]);
if (arg0.IsInvalidated())
return;

gid_t extra_group;
bool must_free;
char* user;
Expand Down Expand Up @@ -2437,6 +2466,8 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {

Local<Object> module = args[0]->ToObject(env->isolate()); // Cast
node::Utf8Value filename(env->isolate(), args[1]); // Cast
if (filename.IsInvalidated())
return;
const bool is_dlopen_error = uv_dlopen(*filename, &lib);

// Objects containing v14 or later modules will have registered themselves
Expand Down Expand Up @@ -2643,6 +2674,8 @@ static void Binding(const FunctionCallbackInfo<Value>& args) {

Local<String> module = args[0]->ToString(env->isolate());
node::Utf8Value module_v(env->isolate(), module);
if (module_v.IsInvalidated())
return;

Local<Object> cache = env->binding_cache_object();
Local<Object> exports;
Expand Down Expand Up @@ -2695,6 +2728,8 @@ static void LinkedBinding(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args.GetIsolate());

Local<String> module_name = args[0]->ToString(env->isolate());
if (module_name.IsEmpty())
return;

Local<Object> cache = env->binding_cache_object();
Local<Value> exports_v = cache->Get(module_name);
Expand All @@ -2703,6 +2738,7 @@ static void LinkedBinding(const FunctionCallbackInfo<Value>& args) {
return args.GetReturnValue().Set(exports_v.As<Object>());

node::Utf8Value module_name_v(env->isolate(), module_name);
CHECK(!module_name_v.IsInvalidated());
node_module* mod = get_linked_module(*module_name_v);

if (mod == nullptr) {
Expand Down Expand Up @@ -2748,6 +2784,8 @@ static void ProcessTitleSetter(Local<Name> property,
Local<Value> value,
const PropertyCallbackInfo<void>& info) {
node::Utf8Value title(info.GetIsolate(), value);
if (title.IsInvalidated())
return;
// TODO(piscisaureus): protect with a lock
uv_set_process_title(*title);
}
Expand All @@ -2761,12 +2799,14 @@ static void EnvGetter(Local<Name> property,
}
#ifdef __POSIX__
node::Utf8Value key(isolate, property);
CHECK(!key.IsInvalidated());
const char* val = getenv(*key);
if (val) {
return info.GetReturnValue().Set(String::NewFromUtf8(isolate, val));
}
#else // _WIN32
node::TwoByteValue key(isolate, property);
CHECK(!key.IsInvalidated());
WCHAR buffer[32767]; // The maximum size allowed for environment variables.
DWORD result = GetEnvironmentVariableW(reinterpret_cast<WCHAR*>(*key),
buffer,
Expand All @@ -2789,11 +2829,19 @@ static void EnvSetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
#ifdef __POSIX__
node::Utf8Value key(info.GetIsolate(), property);
if (key.IsInvalidated())
return;
node::Utf8Value val(info.GetIsolate(), value);
if (val.IsInvalidated())
return;
setenv(*key, *val, 1);
#else // _WIN32
node::TwoByteValue key(info.GetIsolate(), property);
if (key.IsInvalidated())
return;
node::TwoByteValue val(info.GetIsolate(), value);
if (val.IsInvalidated())
return;
WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
// Environment variables that start with '=' are read-only.
if (key_ptr[0] != L'=') {
Expand All @@ -2811,10 +2859,12 @@ static void EnvQuery(Local<Name> property,
if (property->IsString()) {
#ifdef __POSIX__
node::Utf8Value key(info.GetIsolate(), property);
CHECK(!key.IsInvalidated());
if (getenv(*key))
rc = 0;
#else // _WIN32
node::TwoByteValue key(info.GetIsolate(), property);
CHECK(!key.IsInvalidated());
WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 ||
GetLastError() == ERROR_SUCCESS) {
Expand All @@ -2838,9 +2888,11 @@ static void EnvDeleter(Local<Name> property,
if (property->IsString()) {
#ifdef __POSIX__
node::Utf8Value key(info.GetIsolate(), property);
CHECK(!key.IsInvalidated());
unsetenv(*key);
#else
node::TwoByteValue key(info.GetIsolate(), property);
CHECK(!key.IsInvalidated());
WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
SetEnvironmentVariableW(key_ptr, nullptr);
#endif
Expand Down Expand Up @@ -3441,6 +3493,7 @@ static void RawDebug(const FunctionCallbackInfo<Value>& args) {
CHECK(args.Length() == 1 && args[0]->IsString() &&
"must be called with a single string");
node::Utf8Value message(args.GetIsolate(), args[0]);
CHECK(!message.IsInvalidated());
PrintErrorString("%s\n", *message);
fflush(stderr);
}
Expand Down
5 changes: 5 additions & 0 deletions src/node_i18n.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ static void GetVersion(const FunctionCallbackInfo<Value>& args) {
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value val(env->isolate(), args[0]);
CHECK(!val.IsInvalidated());
UErrorCode status = U_ZERO_ERROR;
char buf[U_MAX_VERSION_STRING_LENGTH] = ""; // Possible output buffer.
const char* versionString = GetVersion(*val, buf, &status);
Expand Down Expand Up @@ -513,6 +514,7 @@ static void ToUnicode(const FunctionCallbackInfo<Value>& args) {
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value val(env->isolate(), args[0]);
CHECK(!val.IsInvalidated());
// optional arg
bool lenient = args[1]->BooleanValue(env->context()).FromJust();

Expand All @@ -535,6 +537,7 @@ static void ToASCII(const FunctionCallbackInfo<Value>& args) {
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value val(env->isolate(), args[0]);
CHECK(!val.IsInvalidated());
// optional arg
bool lenient = args[1]->BooleanValue(env->context()).FromJust();

Expand Down Expand Up @@ -609,6 +612,8 @@ static void GetStringWidth(const FunctionCallbackInfo<Value>& args) {
}

TwoByteValue value(env->isolate(), args[0]);
if (value.IsInvalidated())
return;
// reinterpret_cast is required by windows to compile
UChar* str = reinterpret_cast<UChar*>(*value);
static_assert(sizeof(*str) == sizeof(**value),
Expand Down
17 changes: 10 additions & 7 deletions src/node_url.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ using v8::Value;
{ \
Local<Value> val = GET(env, obj, #name); \
if (val->IsString()) { \
Utf8Value value(env->isolate(), val.As<String>()); \
Utf8Value value(env->isolate(), val); \
CHECK(!value.IsInvalidated()); \
data->name = *value; \
data->flags |= flag; \
} \
Expand Down Expand Up @@ -509,7 +510,8 @@ namespace url {
for (int32_t n = 0; n < len; n++) {
Local<Value> val = ary->Get(env->context(), n).ToLocalChecked();
if (val->IsString()) {
Utf8Value value(env->isolate(), val.As<String>());
Utf8Value value(env->isolate(), val);
CHECK(!value.IsInvalidated());
vec->push_back(std::string(*value, value.length()));
}
}
Expand Down Expand Up @@ -562,6 +564,7 @@ namespace url {
Local<Value> scheme = GET(env, context_obj, "scheme");
if (scheme->IsString()) {
Utf8Value value(env->isolate(), scheme);
CHECK(!value.IsInvalidated());
context->scheme.assign(*value, value.length());
}
Local<Value> port = GET(env, context_obj, "port");
Expand Down Expand Up @@ -1297,7 +1300,6 @@ namespace url {
static void Parse(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_GE(args.Length(), 5);
CHECK(args[0]->IsString());
CHECK(args[2]->IsUndefined() ||
args[2]->IsNull() ||
args[2]->IsObject());
Expand All @@ -1306,6 +1308,7 @@ namespace url {
args[3]->IsObject());
CHECK(args[4]->IsFunction());
Utf8Value input(env->isolate(), args[0]);
CHECK(!input.IsInvalidated());
Copy link
Member

Choose a reason for hiding this comment

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

If you remove the CHECK(args[0]->IsString()); (which I agree makes sense), can this be turned into just returning instead of crashing?

ditto for the remainder of the file (or am I missing something here?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll return in this and some other cases, but there are instances where Utf8Value is used where either returning is inconvenient (because it is an ordinary C++ method, not a V8 binding one) or technically impossible (there is an IsString() guard before attempting to read it through Utf8Value).

enum url_parse_state state_override = kUnknownState;
if (args[1]->IsNumber()) {
state_override = static_cast<enum url_parse_state>(
Expand All @@ -1323,8 +1326,8 @@ namespace url {
static void EncodeAuthSet(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value value(env->isolate(), args[0]);
CHECK(!value.IsInvalidated());
std::string output;
const size_t len = value.length();
output.reserve(len);
Expand All @@ -1341,10 +1344,10 @@ namespace url {
static void ToUSVString(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_GE(args.Length(), 2);
CHECK(args[0]->IsString());
CHECK(args[1]->IsNumber());

TwoByteValue value(env->isolate(), args[0]);
CHECK(!value.IsInvalidated());
const size_t n = value.length();

const int64_t start = args[1]->IntegerValue(env->context()).FromJust();
Expand Down Expand Up @@ -1376,8 +1379,8 @@ namespace url {
static void DomainToASCII(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value value(env->isolate(), args[0]);
CHECK(!value.IsInvalidated());

url_host host{{""}, HOST_TYPE_DOMAIN};
ParseHost(&host, *value, value.length());
Expand All @@ -1396,8 +1399,8 @@ namespace url {
static void DomainToUnicode(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
CHECK_GE(args.Length(), 1);
CHECK(args[0]->IsString());
Utf8Value value(env->isolate(), args[0]);
CHECK(!value.IsInvalidated());

url_host host{{""}, HOST_TYPE_DOMAIN};
ParseHost(&host, *value, value.length(), true);
Expand Down
Loading