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

ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

Closed
ChALkeR opened this issue Jun 21, 2018 · 27 comments
Closed

ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined #21440

ChALkeR opened this issue Jun 21, 2018 · 27 comments
Labels
doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http2 Issues or PRs related to the http2 subsystem. tls Issues and PRs related to the tls subsystem.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Jun 21, 2018

While taking a look at #21435 / #21421, I did a quick and dirty check which errors are being instantiated and /lib and checked what happened.

  1. ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK seems to be used in loader, but never defined.

    ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK,
    ERR_UNKNOWN_MODULE_FORMAT
    } = require('internal/errors').codes;
    if (typeof this._dynamicInstantiate !== 'function')
    throw new ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK();
    Not defined in lib/internal/errors.js.

  2. ERR_TLS_RENEGOTIATE seems to be used in tls, but never defined.

    node/lib/_tls_wrap.js

    Lines 47 to 52 in b56f65e

    ERR_TLS_RENEGOTIATE,
    ERR_TLS_RENEGOTIATION_DISABLED,
    ERR_TLS_REQUIRED_SERVER_NAME,
    ERR_TLS_SESSION_ATTACK,
    ERR_TLS_SNI_FROM_SERVER
    } = require('internal/errors').codes;

    node/lib/_tls_wrap.js

    Lines 572 to 574 in b56f65e

    if (callback) {
    process.nextTick(callback, new ERR_TLS_RENEGOTIATE());
    }
    Not defined in lib/internal/errors.js.

  3. ERR_HTTP2_SETTINGS_CANCEL seems to be used in http2, but never defined.

    ERR_HTTP2_SETTINGS_CANCEL,
    ERR_HTTP2_SOCKET_BOUND,
    ERR_HTTP2_STATUS_101,
    ERR_HTTP2_STATUS_INVALID,
    ERR_HTTP2_STREAM_CANCEL,
    ERR_HTTP2_STREAM_ERROR,
    ERR_HTTP2_STREAM_SELF_DEPENDENCY,
    ERR_HTTP2_TRAILERS_ALREADY_SENT,
    ERR_HTTP2_TRAILERS_NOT_READY,
    ERR_HTTP2_UNSUPPORTED_PROTOCOL,
    ERR_INVALID_ARG_TYPE,
    ERR_INVALID_CALLBACK,
    ERR_INVALID_CHAR,
    ERR_INVALID_OPT_VALUE,
    ERR_OUT_OF_RANGE,
    ERR_SOCKET_CLOSED
    }
    } = require('internal/errors');
    if (typeof cb === 'function')
    cb(new ERR_HTTP2_SETTINGS_CANCEL());
    Not defined in lib/internal/errors.js.

This probably affects all 10.x versions, but was not backported to earlier branches as is a semver-major.

Blame points at 1d2fd8b / #19137.

/cc @targos

This also probably needs a testcase when fixed, I do not have one atm.

@ChALkeR ChALkeR added errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem. http2 Issues or PRs related to the http2 subsystem. labels Jun 21, 2018
@ChALkeR ChALkeR changed the title ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK is not defined ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined Jun 21, 2018
@BridgeAR
Copy link
Member

@ChALkeR the blame is wrong. The typo was introduced in 921fb84. Because of the typo the error did not seem to be used anymore so I removed that in 6e1c25c.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 21, 2018

@BridgeAR I am looking only at JS side, not docs.

At 1d2fd8b, there is no ERR_MISSING_DY***, ERR_TLS_RENEGOTIATE or ERR_HTTP2_SETTINGS_CANCEL defined, but those are directly used as properties (for the first time).

I could be wrong with interpreting blame output, and those might have been removed earlier.

@BridgeAR
Copy link
Member

Oh, I only looked at the first entry before the edits.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 21, 2018

This is the list of errors that seem to be missing documentation:

ERR_HTTP2_ERROR
ERR_UNKNOWN_BUILTIN_MODULE
ERR_HTTP2_SETTINGS_CANCEL
ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK
ERR_TLS_RENEGOTIATE

Those are the three mentioned above (that also miss implementation), plus:

  1. ERR_HTTP2_ERROR (used in lib/internal/http2/util.js).
  2. ERR_UNKNOWN_BUILTIN_MODULE (used in lib/internal/bootstrap/loaders.js).

@BridgeAR
Copy link
Member

  1. Was never properly ported: f67aa56 (the error code was not created at all).

@BridgeAR
Copy link
Member

  1. Was never properly implemented: bbaea12

@BridgeAR
Copy link
Member

ERR_UNKNOWN_BUILTIN_MODULE is a false positive. It is actually added to the error directly instead of using internal/errors.

@BridgeAR
Copy link
Member

ERR_HTTP2_ERROR is a false positive as well.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 21, 2018

@BridgeAR #21440 (comment) is specifically about documentation — I saw how those are created and it's ok that they are not present in internal/errors, but they should be documented nevertheless if the user might experience those. I do not think that those are false positives.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 21, 2018

Based on f67aa56, this might also affect v9.x branch.

@BridgeAR
Copy link
Member

Oh, true. They were falsely removed in 1cdb41f.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 21, 2018

@BridgeAR Seems to be also a problem on v8.x, as the doc change was backported in #16556.
/cc @nodejs/lts

Also, @nodejs/collaborators, any idea how to prevent accidential removal of used error codes from the documentation in the future?

I can share my testcase.

@ronkorving
Copy link
Contributor

This may be a good opportunity to rename ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK? :)

@targos
Copy link
Member

targos commented Jun 22, 2018

git blame points to 1d2fd8b but that commit just moves things around. Things should have already been missing before it.

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 22, 2018

Ok, I made the scanner aware of cpp-side errors, and found that ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER is also missing documentation.

To summarize:

Used, but missing implementation:

  1. ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK
  2. ERR_TLS_RENEGOTIATE
  3. ERR_HTTP2_SETTINGS_CANCEL

Used, but missing documentation:

  1. ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK
  2. ERR_TLS_RENEGOTIATE
  3. ERR_HTTP2_SETTINGS_CANCEL
  4. ERR_HTTP2_ERROR
  5. ERR_UNKNOWN_BUILTIN_MODULE
  6. ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER

Documented, do not seem to be present anywhere:

  1. ERR_STREAM_READ_NOT_IMPLEMENTED
  2. ERR_VALUE_OUT_OF_RANGE

@ChALkeR ChALkeR added lib / src Issues and PRs related to general changes in the lib or src directory. doc Issues and PRs related to the documentations. worker Issues and PRs related to Worker support. labels Jun 22, 2018
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 22, 2018

Full output on v10.5.0:

js: ERR_TLS_RENEGOTIATE - missing JS implementation (source)
js: ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK - missing JS implementation (source)
js: ERR_HTTP2_SETTINGS_CANCEL - missing JS implementation (source)

init: ERR_METHOD_NOT_IMPLEMENTED - failed 0 args init, but is called with "()"

impl: ERR_STREAM_READ_NOT_IMPLEMENTED - documented, missing implementation
impl: ERR_VALUE_OUT_OF_RANGE - documented, missing implementation

doc: ERR_UNKNOWN_BUILTIN_MODULE - missing documentation
doc: ERR_TLS_RENEGOTIATE - missing documentation
doc: ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK - missing documentation
doc: ERR_HTTP2_SETTINGS_CANCEL - missing documentation
doc: ERR_HTTP2_ERROR - missing documentation
doc: ERR_TRANSFERRING_EXTERNALIZED_SHAREDARRAYBUFFER - missing documentation

Test script: https://gist.github.com/ChALkeR/a592f1e79624806b383007d7c5d4a395

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 22, 2018

/cc @jasnell

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 22, 2018

PR with my test script: #21470.

@ChALkeR ChALkeR changed the title ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined ERR_TLS_RENEGOTIATE, ERR_HTTP2_SETTINGS_CANCEL are not defined Jun 24, 2018
ChALkeR added a commit to ChALkeR/io.js that referenced this issue Jun 24, 2018
ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE error codes documentation
seem to have been accidentally removed in commit
1cdb41f (pull request nodejs#15160).

This reverts that removal, restoring the documentation for those two
error codes.

Those error codes are used from lib/ folder.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

PR-URL: nodejs#21484
Refs: nodejs#21470
Refs: nodejs#21440
Refs: nodejs#15160
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this issue Jun 24, 2018
ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE error codes documentation
seem to have been accidentally removed in commit
1cdb41f (pull request #15160).

This reverts that removal, restoring the documentation for those two
error codes.

Those error codes are used from lib/ folder.

This is a part of the fixes hinted by #21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

PR-URL: #21484
Refs: #21470
Refs: #21440
Refs: #15160
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@thatshailesh
Copy link
Contributor

@ChALkeR Interested in picking this
Need help in using message text for ERR_TLS_RENEGOTIATE and ERR_HTTP2_SETTINGS_CANCEL

Is it fine to use messages below -
TLS session renegotiate failed for ERR_TLS_RENEGOTIATE
HTTP/2 pending settings has been canceled for ERR_HTTP2_SETTINGS_CANCEL

for the docs-
Attempt to regotiate tls session failed - ERR_TLS_RENEGOTIATE
Http2 session settings canceled - ERR_HTTP2_SETTINGS_CANCEL

Please let me know if there anything to add more here in docs or error message

@davisjam
Copy link
Contributor

@thatshailesh Technically speaking I don't know the appropriate messages, but here's a quick spell check:

  • TLS session renegotiation failed
  • HTTP/2 pending settings have been canceled (Though this sounds a bit funny. Is the problem here that a settings request was cancelled?)

and then

  • Attempt to renegotiate TLS session failed
  • Http2 session settings canceled: Is this spelled Http2 or HTTP2?

@thatshailesh
Copy link
Contributor

@davisjam Thanks for your answer
Actually settings frame MUST be sent by both endpoints at the start of a connection
so from what I understand is its been canceled while the session is waiting for the remote peer to acknowledge the new setting

its spelled HTTP2

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 25, 2018

@thatshailesh Actually, I am not sure how to phrase those myself and do not have anything in mind off-hand, — that's one of the reasons why I preferred to delegate it to someone. So I am not the best choice to ask about phrasing. 😃

I would say — open a pull request with some phrasing that seems fine to you, ask for review from involved people, e.g. those who introduced that API and other collaborators, and see how it goes. 😉

@thatshailesh
Copy link
Contributor

Ok Sure 👍

Working on it

targos pushed a commit to ChALkeR/io.js that referenced this issue Jun 30, 2018
This restores a broken and erroneously removed error, which was
accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice
the "INTST" vs "INST") in 921fb84
(PR nodejs#16874) and then had documentation and implementation removed under
the old name in 6e1c25c (PR nodejs#18857),
as it appeared unused.

This error code never worked or was documented under the mistyped name
ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to
ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix.

Refs: nodejs#21440
Refs: nodejs#21470
Refs: nodejs#16874
Refs: nodejs#18857
targos pushed a commit that referenced this issue Jun 30, 2018
This restores a broken and erroneously removed error, which was
accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice
the "INTST" vs "INST") in 921fb84
(PR #16874) and then had documentation and implementation removed under
the old name in 6e1c25c (PR #18857),
as it appeared unused.

This error code never worked or was documented under the mistyped name
ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to
ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix.

Refs: #21440
Refs: #21470
Refs: #16874
Refs: #18857

PR-URL: #21493
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Jul 3, 2018
This restores a broken and erroneously removed error, which was
accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice
the "INTST" vs "INST") in 921fb84
(PR #16874) and then had documentation and implementation removed under
the old name in 6e1c25c (PR #18857),
as it appeared unused.

This error code never worked or was documented under the mistyped name
ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to
ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix.

Refs: #21440
Refs: #21470
Refs: #16874
Refs: #18857

PR-URL: #21493
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
vsemozhetbyt pushed a commit that referenced this issue Jul 13, 2018
Includes implementation of tls, HTTP2 error with documentation.

PR-URL: #21564
Refs: #21440
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
targos pushed a commit that referenced this issue Jul 14, 2018
Includes implementation of tls, HTTP2 error with documentation.

PR-URL: #21564
Refs: #21440
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 23, 2018

I believe this was fixed in #21493 and #21564.

@ChALkeR ChALkeR closed this as completed Jul 23, 2018
kjin pushed a commit to kjin/node that referenced this issue Aug 23, 2018
Includes implementation of tls, HTTP2 error with documentation.

PR-URL: nodejs#21564
Refs: nodejs#21440
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
ChALkeR added a commit to ChALkeR/io.js that referenced this issue Sep 8, 2018
This adds several sanity checks for error codes.

It scans:
 * all natives (js sources),
 * doc/api/*.md documentation
 * src/node_errors.h (errors definition from the C++ side).

There is also a whitelist of manually created errors from JS side,
currently consisting of ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE.

Alsom all ERR_NAPI_ codes are whitelisted, as those are created directly
on the cpp side, without declaring them first.

The performed checks:

  1. All errors used from JS should be defined in `internal/errors` and
     present in its .codes object. Whitelist (mentioned above) applies.

  2. All errors instantiated from JS without arguments should support
     0-arguments version.

  3. All errors mentioned in doc should defined either in JS, C++, or
     in the whitelist.

  4. All errors mentioned anywhere should be documented.

  5. Documentation of error codes should be sorted, have no repeats,
     and include exactly one entry for every error code mentioned in
     the documentation, formatted as `/\n### (ERR_[A-Z0-9_]+)\n`.

  6. All doc entries for error codes should have appropriate anchors.

There is also a --report flag, which prints all the current issues and
exits without asserting, for manual inspection.

Individual fixes for those issues are landed in separate commits.

Refs: nodejs#21421
Refs: nodejs#21440
Refs: nodejs#21483
Refs: nodejs#21484
Refs: nodejs#21485
Refs: nodejs#21487
PR-URL: nodejs#21470
kjin pushed a commit to kjin/node that referenced this issue Sep 25, 2018
Includes implementation of tls, HTTP2 error with documentation.

PR-URL: nodejs#21564
Refs: nodejs#21440
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
Includes implementation of tls, HTTP2 error with documentation.

PR-URL: nodejs#21564
Refs: nodejs#21440
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
Includes implementation of tls, HTTP2 error with documentation.

Backport-PR-URL: #22850
PR-URL: #21564
Refs: #21440
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. http2 Issues or PRs related to the http2 subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants