tls: do not wrap net.Socket with StreamWrap #12799

krydos · 2017-05-02T19:10:05Z

Hi :)
In this PR I just wanted to avoid "c-like" error from this issue - #3655. You can see below how error looks like now. At least there is stacktrace exist.

Please update me if I did something horribly wrong here or maybe if this is not a fix at all so I will close this PR.

_stream_wrap.js:42
  this.stream.pause();
              ^

TypeError: this.stream.pause is not a function
    at new StreamWrap (_stream_wrap.js:42:15)
    at new TLSSocket (_tls_wrap.js:280:12)
    at Object.<anonymous> (/path/to/test-file.js:4:1)
    at Module._compile (module.js:582:30)
    at Object.Module._extensions..js (module.js:593:10)
    at Module.load (module.js:516:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:618:10)
    at startup (bootstrap_node.js:144:16)

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)

mscdex · 2017-05-02T21:36:40Z

Can you include a test that fails without this change?

krydos · 2017-05-03T06:40:11Z

test added. Thank you @mscdex .

test expect TypeError and do not check the exact error message. I did it like that mostly because test should check the fact that exception was thrown.

mscdex · 2017-05-03T06:52:00Z

/cc @nodejs/crypto

addaleax

Looks good to me, but to be careful we should maybe consider this semver-major (@nodejs/ctc), and it would be good to have @indutny give this a look if possible.

addaleax · 2017-05-03T12:59:56Z

Also, can you edit the commit message to add a Fixes: https://github.com/nodejs/node/issues/3655 line? :)

krydos · 2017-05-03T13:02:51Z

@addaleax I've update commit message, thank you. And actually thank you for helping me to understand and fix the issue :)

addaleax · 2017-05-04T14:16:14Z

Labelled semver-major to be on the safe side

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

krydos · 2017-05-05T07:40:17Z

I think CI hates me 😢

Could you please approve I understand CI output correctly, and it is not related?

lpinca · 2017-05-05T07:45:46Z

@krydos gonna start a new one.

CI 2: https://ci.nodejs.org/job/node-test-pull-request/7880/

cjihrig · 2017-05-09T20:08:34Z

test/parallel/test-tls-wrap-event-emmiter.js

+const EventEmitter = require('events').EventEmitter;
+assert.throws(
+  () => { new TlsSocket(new EventEmitter()); },
+  /^TypeError:/


I know you commented on this already, but please check the full error message.

indutny

LGTM

indutny · 2017-05-09T20:15:57Z

lib/_tls_wrap.js

-  else if ((socket instanceof net.Socket) && !socket._handle)
-    wrap = new StreamWrap(socket);
-  else
+  if (((socket instanceof net.Socket) && socket._handle) || !socket)


Minor nit, lots of unnecessary parens. Do we allow them with lint?

linter was quiet in this case. I'm also wasn't able to find rule to check this...

krydos · 2017-05-10T07:52:08Z

@cjihrig I've added full error message to the test.
@indutny unnecessary parantecies are removed.

Thank you for the feedback!

addaleax · 2017-05-10T17:36:30Z

lib/_tls_wrap.js

-  else if ((socket instanceof net.Socket) && !socket._handle)
-    wrap = new StreamWrap(socket);
-  else
+  if (socket instanceof net.Socket && socket._handle || !socket)


I think @indutny was just talking about the inner parentheses – I would really prefer to keep the ones around the && expression for readability

@addaleax oops, didn't understand it correctly. Sorry.
I've added parentheses around && expression. Thank you :)

Fixes: nodejs#3655

addressed

addaleax · 2017-05-15T18:01:32Z

Landed in b23d414

Fixes: #3655 PR-URL: #12799 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

addaleax · 2017-05-15T18:27:00Z

Looking back, I don’t think labelling this semver-major is necessary – I’d prefer to wait a bit before backporting, though.

krydos · 2017-05-15T18:30:07Z

@addaleax agree with you. Looks more as a bugfix. I hope there are no users of Node.js who rely on this error.

gibfahn · 2017-05-18T16:25:03Z

I hope there are no users of Node.js who rely on this error.

CitGM run: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/796/

addaleax · 2017-05-18T16:26:46Z

@gibfahn The “error” in this case is a failed C++ assertion (i.e. hard crash), just in case you were thinking this is about error messages. :)

Fixes: nodejs#3655 PR-URL: nodejs#12799 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

MylesBorins · 2017-06-22T19:03:20Z

I've thrown a baking for lts label on this PR... if the PR shouldn't land on v6.x please remove and add don't land label

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label May 2, 2017

krydos force-pushed the issue-3655 branch from afd01c2 to fcf1fde Compare May 3, 2017 06:36

addaleax approved these changes May 3, 2017

View reviewed changes

krydos force-pushed the issue-3655 branch from fcf1fde to 917b9dd Compare May 3, 2017 13:01

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 4, 2017

cjihrig previously requested changes May 9, 2017

View reviewed changes

indutny approved these changes May 9, 2017

View reviewed changes

indutny reviewed May 9, 2017

View reviewed changes

krydos force-pushed the issue-3655 branch from 917b9dd to 2432adf Compare May 10, 2017 07:49

addaleax reviewed May 10, 2017

View reviewed changes

tls: do not wrap net.Socket with StreamWrap

4e5d50f

Fixes: nodejs#3655

krydos force-pushed the issue-3655 branch from 2432adf to 4e5d50f Compare May 10, 2017 17:48

addaleax closed this May 15, 2017

addaleax pushed a commit that referenced this pull request May 15, 2017

tls: do not wrap net.Socket with StreamWrap

b23d414

Fixes: #3655 PR-URL: #12799 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

krydos deleted the issue-3655 branch May 15, 2017 18:02

addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label May 15, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017

tls: do not wrap net.Socket with StreamWrap

651e96c

Fixes: nodejs#3655 PR-URL: nodejs#12799 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Fedor Indutny <[email protected]>

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels Jun 22, 2017

arthurschreiber mentioned this pull request Jan 25, 2018

Remove tls.createSecurePair usage, closes #515 tediousjs/tedious#689

Closed

MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: do not wrap net.Socket with StreamWrap #12799

tls: do not wrap net.Socket with StreamWrap #12799

krydos commented May 2, 2017 •

edited

Loading

mscdex commented May 2, 2017

krydos commented May 3, 2017

mscdex commented May 3, 2017

addaleax left a comment

addaleax commented May 3, 2017

krydos commented May 3, 2017

addaleax commented May 4, 2017

krydos commented May 5, 2017

lpinca commented May 5, 2017

cjihrig May 9, 2017

indutny left a comment

indutny May 9, 2017

krydos May 10, 2017

krydos commented May 10, 2017

addaleax May 10, 2017

krydos May 10, 2017

addaleax commented May 15, 2017

addaleax commented May 15, 2017

krydos commented May 15, 2017

gibfahn commented May 18, 2017

addaleax commented May 18, 2017

MylesBorins commented Jun 22, 2017

tls: do not wrap net.Socket with StreamWrap #12799

tls: do not wrap net.Socket with StreamWrap #12799

Conversation

krydos commented May 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex commented May 2, 2017

krydos commented May 3, 2017

mscdex commented May 3, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented May 3, 2017

krydos commented May 3, 2017

addaleax commented May 4, 2017

krydos commented May 5, 2017

lpinca commented May 5, 2017

cjihrig May 9, 2017

Choose a reason for hiding this comment

indutny left a comment

Choose a reason for hiding this comment

indutny May 9, 2017

Choose a reason for hiding this comment

krydos May 10, 2017

Choose a reason for hiding this comment

krydos commented May 10, 2017

addaleax May 10, 2017

Choose a reason for hiding this comment

krydos May 10, 2017

Choose a reason for hiding this comment

addaleax commented May 15, 2017

addaleax commented May 15, 2017

krydos commented May 15, 2017

gibfahn commented May 18, 2017

addaleax commented May 18, 2017

MylesBorins commented Jun 22, 2017

krydos commented May 2, 2017 •

edited

Loading