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

crypto: Remove unused access of tlsext_hostname. #10882

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants
@davidben
Contributor

davidben commented Jan 18, 2017

The return value of loadSession is ultimately ignored, so don't fill it in. This inches Node closer to 1.1.0 compatibility and is less code.

Also remove a comment which appears to have long since become invalid. It dates to 048e0e7 when the SNI value was actually extracted from the session.

This also fixes a segfault should d2i_SSL_SESSION fail to parse the input and return NULL.

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)

crypto

@@ -1816,17 +1816,6 @@ void SSLWrap<Base>::LoadSession(const FunctionCallbackInfo<Value>& args) {
if (w->next_sess_ != nullptr)
SSL_SESSION_free(w->next_sess_);
w->next_sess_ = sess;

This comment has been minimized.

@davidben

davidben Jan 18, 2017

Contributor

I just left the error case silently ignoring it. Happy to change it if you'd prefer something else.

@davidben

davidben Jan 18, 2017

Contributor

I just left the error case silently ignoring it. Happy to change it if you'd prefer something else.

Show outdated Hide outdated test/parallel/test-tls-invalid-session.js
@@ -0,0 +1,94 @@
'use strict';
const common = require('../common');

This comment has been minimized.

@davidben

davidben Jan 18, 2017

Contributor

This file is basically entirely copied from test-tls-session-cache.js with tweaks, so it's probably doing more than it actually needs to.

@davidben

davidben Jan 18, 2017

Contributor

This file is basically entirely copied from test-tls-session-cache.js with tweaks, so it's probably doing more than it actually needs to.

This comment has been minimized.

@sam-github

sam-github Jan 18, 2017

Member

can you just add the condition you are testing to test-tls-session-cache.js?

@sam-github

sam-github Jan 18, 2017

Member

can you just add the condition you are testing to test-tls-session-cache.js?

This comment has been minimized.

@davidben

davidben Jan 18, 2017

Contributor

Done.

@davidben

davidben Jan 18, 2017

Contributor

Done.

@mscdex mscdex removed the crypto label Jan 18, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 18, 2017

Contributor

/cc @indutny

Contributor

mscdex commented Jan 18, 2017

/cc @indutny

@bnoordhuis

LGTM but would it be possible to merge the test into the existing test? It's duplicating quite a bit of finicky code right now.

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Jan 18, 2017

Contributor

@bnoordhuis Oh yeah, that's much tidier. Done.

Contributor

davidben commented Jan 18, 2017

@bnoordhuis Oh yeah, that's much tidier. Done.

@indutny

LGTM

@bnoordhuis

LGTM, thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/5946/

It's possible the ARM buildbots report failure while the status is green when you click through. Known issue, can be safely ignored.

@sam-github

Please run make test - the linter failure is real.

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Jan 19, 2017

Contributor

@sam-github Oops. Fixed.

Contributor

davidben commented Jan 19, 2017

@sam-github Oops. Fixed.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Jan 25, 2017

Contributor

I see some red in that link, but I'm not sure how to read it. What's the error?

Contributor

davidben commented Jan 25, 2017

I see some red in that link, but I'm not sure how to read it. What's the error?

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 25, 2017

Member

test/arm is known to fail, sqaush and force push, and I'll retest

Member

sam-github commented Jan 25, 2017

test/arm is known to fail, sqaush and force push, and I'll retest

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 25, 2017

Member

also, rebase against master

Member

sam-github commented Jan 25, 2017

also, rebase against master

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Jan 25, 2017

Member

And fix the commit message, it should be

crypto: remove unused access of tlsext_hostname

No capital, no period.

Member

sam-github commented Jan 25, 2017

And fix the commit message, it should be

crypto: remove unused access of tlsext_hostname

No capital, no period.

crypto: remove unused access of tlsext_hostname
The return value of loadSession is ultimately ignored, so don't fill it
in. This inches Node closer to 1.1.0 compatibility and is less code.

Also remove a comment which appears to have long since become invalid.
It dates to 048e0e7 when the SNI value
was actually extracted from the session.

This also fixes a segfault should d2i_SSL_SESSION fail to parse the
input and return NULL. Add a test for this case based on
test-tls-session-cache.js.
@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Jan 25, 2017

Contributor

Squashed and rebased (and comment reworded).

Contributor

davidben commented Jan 25, 2017

Squashed and rebased (and comment reworded).

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Feb 2, 2017

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Feb 13, 2017

Member

Landed in e34ee1d

Member

sam-github commented Feb 13, 2017

Landed in e34ee1d

@sam-github sam-github closed this Feb 13, 2017

sam-github added a commit that referenced this pull request Feb 13, 2017

crypto: remove unused access of tlsext_hostname
The return value of loadSession is ultimately ignored, so don't fill it
in. This inches Node closer to 1.1.0 compatibility and is less code.

Also remove a comment which appears to have long since become invalid.
It dates to 048e0e7 when the SNI value
was actually extracted from the session.

This also fixes a segfault should d2i_SSL_SESSION fail to parse the
input and return NULL. Add a test for this case based on
test-tls-session-cache.js.

PR-URL: #10882
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

crypto: remove unused access of tlsext_hostname
The return value of loadSession is ultimately ignored, so don't fill it
in. This inches Node closer to 1.1.0 compatibility and is less code.

Also remove a comment which appears to have long since become invalid.
It dates to 048e0e7 when the SNI value
was actually extracted from the session.

This also fixes a segfault should d2i_SSL_SESSION fail to parse the
input and return NULL. Add a test for this case based on
test-tls-session-cache.js.

PR-URL: nodejs#10882
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

@ChALkeR ChALkeR referenced this pull request Feb 15, 2017

Closed

src: remove unused variable #11361

2 of 2 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

needs a backport PR in order to land on v4 or v6

Member

jasnell commented Mar 7, 2017

needs a backport PR in order to land on v4 or v6

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Mar 7, 2017

Contributor

To clarify, is that a request for me to put one together or just a note for you all? (I'm not familiar with how your branching arrangement works.)

Contributor

davidben commented Mar 7, 2017

To clarify, is that a request for me to put one together or just a note for you all? (I'm not familiar with how your branching arrangement works.)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

@davidben ... just a note :-) if someone felt that this needed to go into v6 or v4, then a manual backport would be required because the commit is not landing cleanly as is. Feel free to ignore :-)

Member

jasnell commented Mar 7, 2017

@davidben ... just a note :-) if someone felt that this needed to go into v6 or v4, then a manual backport would be required because the commit is not landing cleanly as is. Feel free to ignore :-)

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Mar 8, 2017

Member

I think we should backport as much as possible, not because we desperately need this PR, but because if we don't land it, subsequent PRs might start not backporting as easily due to conflicts.

@davidben if this applies to 6.x and 4.x (it looks like it would), do you have time to backport?

Member

sam-github commented Mar 8, 2017

I think we should backport as much as possible, not because we desperately need this PR, but because if we don't land it, subsequent PRs might start not backporting as easily due to conflicts.

@davidben if this applies to 6.x and 4.x (it looks like it would), do you have time to backport?

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Mar 8, 2017

Contributor

Looks like the reason it doesn't apply cleanly is because of #10685 and #10698 which were not backported. Though those two really don't apply cleanly. :-)

I dunno how far back you want to go here. Doing just the merge is straightforward enough; I can either switch all my lets back to vars and strictEquals to equals or leave them alone and assume the existing lines will be done by a future backport. Dunno if that would just cause more trouble by getting things out of order. (Do you intend to backport those two?)

Contributor

davidben commented Mar 8, 2017

Looks like the reason it doesn't apply cleanly is because of #10685 and #10698 which were not backported. Though those two really don't apply cleanly. :-)

I dunno how far back you want to go here. Doing just the merge is straightforward enough; I can either switch all my lets back to vars and strictEquals to equals or leave them alone and assume the existing lines will be done by a future backport. Dunno if that would just cause more trouble by getting things out of order. (Do you intend to backport those two?)

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Mar 9, 2017

Member

Its not clear at this moment what should be backported. Lets wait and see. @gibfahn did the two PRs you mentioned by running eslint in fixup mode, it would probably be easier to repeat that than to actually backport.

Member

sam-github commented Mar 9, 2017

Its not clear at this moment what should be backported. Lets wait and see. @gibfahn did the two PRs you mentioned by running eslint in fixup mode, it would probably be easier to repeat that than to actually backport.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Mar 9, 2017

Member

@davidben Yep, let me backport those two PRs first.

Member

gibfahn commented Mar 9, 2017

@davidben Yep, let me backport those two PRs first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment