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.

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

net: fix abort on bad address input #13726

Closed
wants to merge 5 commits into from

Conversation

BridgeAR
Copy link
Member

Calling net.createConnection with a bad path results in a segfault. This fixes this by checking for the type and by throwing a TypeError in case it's not a string.

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)

net

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Jun 16, 2017
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM with the tiny nit I’d phrase the commit message as fix abort rather than fix segfault, because that’s what it is (at least on POSIX, but it shouldn’t be a segfault on Windows either)

lib/net.js Outdated
@@ -872,6 +872,10 @@ function internalConnect(

var err;

if (typeof address !== 'string') {
throw new TypeError('Invalid address: ' + address);
Copy link
Member

Choose a reason for hiding this comment

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

because this is introducing a new error, it should use internal/errors


{
try {
net.createConnection({ path: {} });
Copy link
Member

Choose a reason for hiding this comment

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

Use assert.throws() here.

@BridgeAR BridgeAR changed the title net: fix segfault on bad address input net: fix abort on bad address input Jun 16, 2017
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 16, 2017
@jasnell
Copy link
Member

jasnell commented Jun 16, 2017

Defensively marking as semver-major due to the new throw. Am argument could be made for patch tho because it would abort previously

@addaleax
Copy link
Member

@jasnell I’m surprised … we tag error changes as semver-major because we know people might rely non-throwing behaviour, or existing errors and their messages, but I find it hard to believe people actively rely on processes aborting?

@jasnell
Copy link
Member

jasnell commented Jun 16, 2017

It's not likely at all. As I said, an argument can be made that it's a patch, just need to make sure folks agree

@BridgeAR
Copy link
Member Author

I agree that it is very unlikely that anyone would rely on that. It's actually already rare to trigger this in the first place and I think it would be good to backport this, if it's accepted as semver-patch.

lib/net.js Outdated
@@ -872,6 +873,10 @@ function internalConnect(

var err;

if (typeof address !== 'string') {
Copy link
Contributor

@mscdex mscdex Jun 16, 2017

Choose a reason for hiding this comment

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

Can we move this to .connect() instead, under the if (pipe) check? internalConnect() is also called for non-pipe connections and so it would be an unnecessary/redundant check in those cases (since the address would be validated at time of lookup() there).

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % moving the check

@refack
Copy link
Contributor

refack commented Jun 16, 2017

@refack
Copy link
Contributor

refack commented Jun 16, 2017

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Error messages need assertion

@@ -182,7 +182,8 @@ E('ERR_V8BREAKITERATOR', 'full ICU data not installed. ' +
function invalidArgType(name, expected, actual) {
const assert = lazyAssert();
assert(name, 'name is required');
var msg = `The "${name}" argument must be ${oneOf(expected, 'type')}`;
const type = name.includes('.') ? 'property' : 'argument'
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes this a semver-major. Is it worth it?

{
assert.throws(() => {
net.createConnection({ path: {} });
}, TypeError);
Copy link
Contributor

Choose a reason for hiding this comment

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

Re: asserting error messages, Could you use expectsError

}));
c.on('connect', common.mustNotCall());
c.on('error', common.mustCall(function(e) {
assert.strictEqual(e.code, 'ENOENT');
Copy link
Contributor

Choose a reason for hiding this comment

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

expectsError ditto

@refack
Copy link
Contributor

refack commented Jun 16, 2017

3 tests fail
https://ci.nodejs.org/job/node-test-commit-linuxone/6689/nodes=rhel72-s390x/tapResults/
Just copied one:

917	parallel/test-process-cpuUsage	
duration_ms	0.58
severity	fail
stack	|-
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'The "preValue.user" property must be of type Number' === 'The "preValue.user" argument must be of type Number'
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:703:14)
    at expectedException (assert.js:520:19)
    at _throws (assert.js:568:8)
    at Function.throws (assert.js:577:3)
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-process-cpuUsage.js:48:8)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)

@refack
Copy link
Contributor

refack commented Jun 16, 2017

@refack refack self-assigned this Jun 17, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM as a semver patch with the comment addressed.

@refack refack removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 19, 2017
@refack
Copy link
Contributor

refack commented Jun 19, 2017

I see 3 CTC approvals, but still think it would be nice to have this as semver-patch
( from CTC I think I see +2 for patch and -0.1 from @jasnell )
Removing semver-major label, please feel free to correct me.

@jasnell
Copy link
Member

jasnell commented Jun 19, 2017

There's no -0.1 from me at all. I marked it as a semver-major because of our policy, but I'm fine with it being a patch as long as there is consensus for doing so.

@refack
Copy link
Contributor

refack commented Jun 19, 2017

There's no -0.1 from me at all. I marked it as a semver-major because of our policy, but I'm fine with it being a patch as long as there is consensus for doing so.

Cool! I see consensus.
Planning to land in 3 hours.

refack pushed a commit to refack/node that referenced this pull request Jun 19, 2017
PR-URL: nodejs#13726
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Jun 19, 2017

Landed in f40caf7

@refack refack closed this Jun 19, 2017
@refack
Copy link
Contributor

refack commented Jun 19, 2017

Quick extra sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6756/ ✔️

addaleax pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #13726
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
addaleax pushed a commit that referenced this pull request Jun 21, 2017
PR-URL: #13726
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 20, 2017
PR-URL: nodejs#13726
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

I opened a PR with a backport for 6.x

MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
Backport-PR-URL: #14390
PR-URL: #13726
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
@BridgeAR BridgeAR deleted the fix-segfault branch April 15, 2018 19:57
@BridgeAR BridgeAR restored the fix-segfault branch April 15, 2018 19:57
@refack refack removed their assignment Oct 12, 2018
@BridgeAR BridgeAR deleted the fix-segfault branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants