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

[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

wants to merge 7 commits into from

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Mar 21, 2017 •
edited
Loading

So far all of the changes are semver-patch. Plans to replace certain usage of Utf8Value with BufferValue are semver-minor and have not been done yet.

  • cares_wrap.cc
  • env.cc
  • node.cc
  • node_buffer.cc
  • node_crypto.cc
  • node_dtrace.cc
  • node_file.cc
  • node_i18n.cc
  • node_lttng.cc
  • node_stat_watcher.cc
  • node_url.cc
  • pipe_wrap.cc
  • process_wrap.cc
  • tcp_wrap.cc
  • tls_wrap.cc
  • udp_wrap.cc

Fixes: #9819
Fixes: #9820

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@TimothyGu TimothyGu added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 21, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. dont-land-on-v4.x i18n-api Issues and PRs related to the i18n implementation. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Mar 21, 2017
@TimothyGu TimothyGu mentioned this pull request Mar 21, 2017
Closed
mscdex
mscdex reviewed Mar 21, 2017
View reviewed changes
lib/child_process.js
@@ -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.

@bnoordhuis
Copy link
Member

Plans to replace certain usage of Utf8Value with BufferValue are semver-minor and have not been done yet.

How are they semver-minor? Are there observable differences in behavior?

@TimothyGu
Copy link
Member Author

Are there observable differences in behavior?

They can be. For Buffers, before the change it would force all invalid UTF-16 sequences (unpaired surrogates) in the buffer to be turned into U+FFFD REPLACEMENT CHARACTER, whereas after that it would just keep them as they are.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2017

hmm... that may qualify as a semver-major depending on the context. Do you have some examples of where that would impact?

addaleax
addaleax reviewed Mar 24, 2017
View reviewed changes
src/node_url.cc
@@ -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).

src/spawn_sync.cc
if (!js_value->IsArray())
return UV_EINVAL;

CHECK(js_value->IsArray());
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes related? Why is crashing better here? I think this one might be semver-major if it’s kept this way (or are there type checks somewhere else? In that case, could ParseStdioOptions be made more specific so it takes a Local<Array> instead?)

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.

Yes, before this PR, js_value might be not an array since it is not sanitized in JS. After this PR, they should be sanitized and it would be a bug if it is not an array here.

Somewhat; js_value should already have been sanitized in JS, and the return UV_EINVAL; is dead code.

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Mar 24, 2017
@addaleax
Copy link
Member

ping @TimothyGu

@Trott
Copy link
Member

Trott commented Jul 30, 2017

Looks like this has stalled out a bit. (Also needs a rebase.) Should we add a help wanted label or would more cooks just create more problems?

@TimothyGu
Copy link
Member Author

I'll close this for now. I can think of a better way of fixing this.

@TimothyGu TimothyGu closed this Jul 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@mscdex mscdex mscdex left review comments

Assignees

@addaleax addaleax

@TimothyGu TimothyGu

Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. i18n-api Issues and PRs related to the i18n implementation. whatwg-url Issues and PRs related to the WHATWG URL implementation. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

spawnSync segfaults when given throwing toString Crypto Hashm/Hmac digest segfault on bad input
7 participants
@TimothyGu @bnoordhuis @jasnell @addaleax @Trott @mscdex @nodejs-github-bot