Skip to content

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

Closed
TimothyGu wants to merge 7 commits into
nodejs:masterfrom
TimothyGu:utf8value-error
Closed

[DO NOT LAND] src: invalidate Utf8/TwoByteValue upon error#11952
TimothyGu wants to merge 7 commits into
nodejs:masterfrom
TimothyGu:utf8value-error

Conversation

@TimothyGu

@TimothyGu TimothyGu commented Mar 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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 Node.js internationalization support. 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
Comment thread lib/child_process.js
if (Array.isArray(args)) {
args = args.slice(0);
for (var i = 0; i < args.length; i++)
args[i] = `${args[i]}`;

@mscdex mscdex Mar 21, 2017

Copy link
Copy Markdown
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] += ''; ?

@TimothyGu TimothyGu Apr 16, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

bnoordhuis commented Mar 21, 2017

Copy link
Copy Markdown
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

TimothyGu commented Mar 21, 2017

Copy link
Copy Markdown
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

jasnell commented Mar 21, 2017

Copy link
Copy Markdown
Member

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
Comment thread src/node_url.cc
args[3]->IsObject());
CHECK(args[4]->IsFunction());
Utf8Value input(env->isolate(), args[0]);
CHECK(!input.IsInvalidated());

@addaleax addaleax Mar 24, 2017

Copy link
Copy Markdown
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?)

@TimothyGu TimothyGu Apr 16, 2017

Copy link
Copy Markdown
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).

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

CHECK(js_value->IsArray());

@addaleax addaleax Mar 24, 2017

Copy link
Copy Markdown
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?)

@TimothyGu TimothyGu Apr 16, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

addaleax commented Apr 14, 2017

Copy link
Copy Markdown
Member

ping @TimothyGu

@Trott

Trott commented Jul 30, 2017

Copy link
Copy Markdown
Member

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

TimothyGu commented Jul 30, 2017

Copy link
Copy Markdown
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
+1 more reviewer
@mscdex mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

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 Node.js internationalization support. 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