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.

Sign up for GitHub

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

Jump to bottom

tls_wrap: migrate synchronous errors #18125

Closed
wants to merge 3 commits into from
Closed

tls_wrap: migrate synchronous errors #18125

wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 12, 2018 •
edited
Loading

tls_wrap: migrate C++ errors to internal/errors.js

  • Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a
    server-side socket instead of returning silently
  • Assert on wrap_->started and wrap_->ssl instead of throwing
    errors since these errors indicate that the user either uses
    private APIs, or monkey-patches internals, or hits a bug.

tls_wrap: migrate argument type-checking errors

  • Throw ERR_INVALID_ARG_TYPE from public APIs
  • Assert argument types in bindings instead of throwing errors
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

tls, errors

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem. labels Jan 12, 2018
@joyeecheung joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 12, 2018
@joyeecheung
Copy link
Member Author

cc @jasnell @nodejs/tsc

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

addaleax
addaleax approved these changes Jan 12, 2018
View reviewed changes
lib/_tls_wrap.js
const res = tls_wrap.wrap(handle._externalStream,
const externalStream = handle._externalStream;
assert(typeof externalStream === 'object',
'handle must be a LibuvStreamWrap');
Copy link
Member

Choose a reason for hiding this comment

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

It just needs to be a StreamBase, even if LibuvStreamWrap is the common case here.

Copy link
Member Author

@joyeecheung joyeecheung Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax I thought about checking the type of handle here but there doesn't seem to be a way of checking those in JS land since there isn't a proper inheritance of StreamBase.

Copy link
Member

Choose a reason for hiding this comment

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

@joyeecheung Yeah, externalStream doesn’t really tell you what kind of thing it is. :/

Ideally, what we could do is making the StreamBase inheritance hierarchy reflected in the actual JS handle classes; that requires more work than this PR, though…

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax Yeah that should probably better be done in a dedicated PR IMO

maclover7
maclover7 reviewed Jan 12, 2018
View reviewed changes
lib/_tls_wrap.js
@@ -407,7 +410,12 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
const context = options.secureContext ||
options.credentials ||
tls.createSecureContext(options);
const res = tls_wrap.wrap(handle._externalStream,
const externalStream = handle._externalStream;
assert(typeof externalStream === 'object',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be throw errors.TypeErrors instead of asserts?

Copy link
Member Author

Choose a reason for hiding this comment

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

@maclover7 I tried that before and the test looked really dubious. These are not user-facing errors, whoever hits then either is using the underscored API or patching internals incorrectly, or is hitting a bug. Either way assertions seem to be more appropriate.

@indutny
Copy link
Member

indutny commented Jan 14, 2018 via email

@joyeecheung
tls: migrate argument type-checking errors
0cfb5ca 
* Throw ERR_INVALID_ARG_TYPE from public APIs
* Assert argument types in bindings instead of throwing errors

PR-URL: nodejs#18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@joyeecheung
tls: migrate C++ errors to internal/errors.js
7e8ecc9 
* Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a
  server-side socket instead of returning silently
* Assert on wrap_->started and wrap_->ssl instead of throwing
  errors since these errors indicate that the user either uses
  private APIs, or monkey-patches internals, or hits a bug.

PR-URL: nodejs#18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@joyeecheung
Copy link
Member Author

Updated since #17882 has landed and introduced a check on a changed error. The assertion is probably equally unhelpful as before but since tls.createSecurePair() is deprecated I think it's fine not to add another error code for it.

New CI: https://ci.nodejs.org/job/node-test-pull-request/12537/

@joyeecheung joyeecheung mentioned this pull request Jan 15, 2018
Closed
@joyeecheung
Copy link
Member Author

CI is green. Can you take a loot again? Thanks! @addaleax @indutny

indutny
indutny approved these changes Jan 15, 2018
View reviewed changes
Copy link
Member

@indutny indutny 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 long as CI is happy

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 15, 2018
@joyeecheung
Copy link
Member Author

Landed in f75bc2c...1c29da8, thanks!

joyeecheung added a commit that referenced this pull request Jan 16, 2018
@joyeecheung
tls: migrate argument type-checking errors
9ffebea 
* Throw ERR_INVALID_ARG_TYPE from public APIs
* Assert argument types in bindings instead of throwing errors

PR-URL: #18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Jan 16, 2018
@joyeecheung
tls: migrate C++ errors to internal/errors.js
1c29da8 
* Throw ERR_TLS_SNI_FROM_SERVER when setting server names on a
  server-side socket instead of returning silently
* Assert on wrap_->started and wrap_->ssl instead of throwing
  errors since these errors indicate that the user either uses
  private APIs, or monkey-patches internals, or hits a bug.

PR-URL: #18125
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maclover7 maclover7 maclover7 left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@indutny indutny indutny approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@joyeecheung @indutny @jasnell @addaleax @maclover7 @nodejs-github-bot