net: fix abort on bad address input by BridgeAR · Pull Request #13726 · nodejs/node

BridgeAR · 2017-06-16T20:15:33Z

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

addaleax

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)

jasnell · 2017-06-16T20:27:32Z

lib/net.js

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

jasnell · 2017-06-16T20:28:04Z

test/parallel/test-net-better-error-messages-path.js

Use assert.throws() here.

jasnell · 2017-06-16T20:51:41Z

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

addaleax · 2017-06-16T20:53:57Z

@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 · 2017-06-16T20:56:12Z

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 · 2017-06-16T20:56:16Z

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.

mscdex · 2017-06-16T21:01:08Z

lib/net.js


  var err;

+  if (typeof address !== 'string') {


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).

refack

LGTM % moving the check

refack · 2017-06-16T21:40:19Z

Quick sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6688/nodes=rhel72-s390x/ ✔️

refack · 2017-06-16T21:57:17Z

Full CI: https://ci.nodejs.org/job/node-test-commit/10616/

refack

Error messages need assertion

refack · 2017-06-16T21:58:50Z

lib/internal/errors.js

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

refack · 2017-06-16T22:01:54Z

test/parallel/test-net-better-error-messages-path.js

Re: asserting error messages, Could you use expectsError

refack · 2017-06-16T22:02:32Z

test/parallel/test-net-better-error-messages-path.js

expectsError ditto

refack · 2017-06-16T22:05:09Z

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 · 2017-06-16T23:22:38Z

Full CI: https://ci.nodejs.org/job/node-test-commit/10617/

cjihrig

LGTM as a semver patch ~~with the comment addressed~~.

refack · 2017-06-19T16:40:11Z

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 · 2017-06-19T16:56:38Z

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 · 2017-06-19T18:24:59Z

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.

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 · 2017-06-19T23:06:36Z

Landed in f40caf7

refack · 2017-06-19T23:12:02Z

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

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 · 2017-07-17T18:42:41Z

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

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 · 2017-07-20T13:08:50Z

I opened a PR with a backport for 6.x

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>

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

addaleax approved these changes Jun 16, 2017

View reviewed changes

addaleax added the lts-watch-v6.x label Jun 16, 2017

net: fix abort on bad address input

097aea4

jasnell reviewed Jun 16, 2017

View reviewed changes

fixup

a4d753f

BridgeAR force-pushed the fix-segfault branch from bff6ede to 097aea4 Compare June 16, 2017 20:40

BridgeAR changed the title ~~net: fix segfault on bad address input~~ net: fix abort on bad address input Jun 16, 2017

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 16, 2017

mscdex reviewed Jun 16, 2017

View reviewed changes

refack approved these changes Jun 16, 2017

View reviewed changes

move check to connect function

dcb6cde

refack suggested changes Jun 16, 2017

View reviewed changes

fix line length

8ee82af

BridgeAR force-pushed the fix-segfault branch from 6caeae6 to 8ee82af Compare June 16, 2017 22:14

address comments

7a6f414

refack approved these changes Jun 16, 2017

View reviewed changes

refack self-assigned this Jun 17, 2017

cjihrig approved these changes Jun 17, 2017

View reviewed changes

lpinca approved these changes Jun 17, 2017

View reviewed changes

jasnell approved these changes Jun 19, 2017

View reviewed changes

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

refack closed this Jun 19, 2017

addaleax mentioned this pull request Jun 21, 2017

v8.2.0 proposal #13744

Merged

addaleax mentioned this pull request Jun 21, 2017

v8.1.3 proposal #13861

Merged

MylesBorins added the backport-requested-v6.x label Jul 17, 2017

BridgeAR mentioned this pull request Jul 20, 2017

[v6.x backport] net: fix abort on bad address input #14390

Closed

4 tasks

BridgeAR added the backported-to-v8.x label Sep 14, 2017

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Sep 19, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

BridgeAR removed the lts-watch-v6.x label Dec 11, 2017

BridgeAR deleted the fix-segfault branch April 15, 2018 19:57

BridgeAR restored the fix-segfault branch April 15, 2018 19:57

refack removed their assignment Oct 12, 2018

BridgeAR deleted the fix-segfault branch April 1, 2019 23:36

Uh oh!

Conversation

BridgeAR commented Jun 16, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 16, 2017

Uh oh!

addaleax commented Jun 16, 2017

Uh oh!

jasnell commented Jun 16, 2017

Uh oh!

BridgeAR commented Jun 16, 2017

Uh oh!

mscdex Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 16, 2017

Uh oh!

refack commented Jun 16, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jun 16, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 16, 2017

Uh oh!

refack commented Jun 16, 2017

Uh oh!

cjihrig left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 19, 2017

Uh oh!

jasnell commented Jun 19, 2017

Uh oh!

refack commented Jun 19, 2017

Uh oh!

refack commented Jun 19, 2017

Uh oh!

refack commented Jun 19, 2017

Uh oh!

MylesBorins commented Jul 17, 2017

Uh oh!

BridgeAR commented Jul 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

addaleax left a comment •

edited

Loading

mscdex Jun 16, 2017 •

edited

Loading

cjihrig left a comment •

edited

Loading