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

http2: throw better error when accessing unbound socket proxy #22486

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 23, 2018

Fixes the bug in the sense that a better error message is provided. Generally speaking, once the connection has been closed, users should not continue to the use the socket proxy object. So, throw whenever they try to.

Fixes: #22268

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added dont-land-on-v6.x errors Issues and PRs related to JavaScript errors originated in Node.js core. http2 Issues or PRs related to the http2 subsystem. labels Aug 23, 2018
@szmarczak
Copy link
Member

There's another ways to expose the original socket:

session.socket.ref()
session.socket.unref()
session.socket.setEncoding()
session.socket.setKeepAlive()
session.socket.setNoDelay()

Should these be fixed?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I am not sure if this is the ideal solution but the code looks good to me.

}, {
code: 'ERR_HTTP2_SOCKET_UNBOUND'
});
}));
Copy link
Member

Choose a reason for hiding this comment

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

What is expected to happen sync?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2018

Should these be fixed?

Yeah, I'll take another look.

@jasnell
Copy link
Member Author

jasnell commented Aug 24, 2018

Added additional tests for the ref(), unref(), setEncoding(), setKeepAlive(), and setNoDelay() functions.

@jasnell
Copy link
Member Author

jasnell commented Aug 24, 2018

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 24, 2018
@Trott
Copy link
Member

Trott commented Aug 25, 2018

@Trott
Copy link
Member

Trott commented Aug 25, 2018

Theoretically semver-major (I think?) if it doesn't end up in the same release as the "HTTP/2 is stable!" thing, so let's get this thing landed.

I personally don't know that I would have chosen an HTTP2 error for this (and others) instead of one of the existing SOCKET errors, but maybe more specific is good and I make bad decisions. 🙃

@Trott
Copy link
Member

Trott commented Aug 25, 2018

@nodejs/http2

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

jasnell added a commit that referenced this pull request Aug 27, 2018
Fixes: #22268

PR-URL: #22486
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Aug 27, 2018

Landed in dd03706

@jasnell jasnell closed this Aug 27, 2018
addaleax pushed a commit that referenced this pull request Aug 27, 2018
Fixes: #22268

PR-URL: #22486
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@szmarczak
Copy link
Member

@jasnell I think you missed something :P

You can't do session.socket.destroy() because it'll throw ERR_HTTP2_NO_SOCKET_MANIPULATION, but you can do session.socket.ref().destroy(). As I said, these functions expose the original socket:

session.socket.ref()
session.socket.unref()
session.socket.setEncoding()
session.socket.setKeepAlive()
session.socket.setNoDelay()

So ProxySocket should look like:

  get(session, prop) {
    switch (prop) {
      case 'setTimeout':
        return session.setTimeout.bind(session);
      case 'destroy':
      case 'emit':
      case 'end':
      case 'pause':
      case 'read':
      case 'resume':
      case 'write':
      case 'setEncoding':
      case 'setKeepAlive':
      case 'setNoDelay':
        throw new ERR_HTTP2_NO_SOCKET_MANIPULATION();
      case 'ref':
      case 'unref':
        return socket[prop].bind(session.socket);
      default:
        const socket = session[kSocket];
        if (socket === undefined)
          throw new ERR_HTTP2_SOCKET_UNBOUND();
        const value = socket[prop];
        return typeof value === 'function' ? value.bind(socket) : value;
    }
  }
...
  set(session, prop, value) {
    switch (prop) {
      case 'setTimeout':
        session.setTimeout = value;
        return true;
      case 'destroy':
      case 'emit':
      case 'end':
      case 'pause':
      case 'read':
      case 'resume':
      case 'write':
      case 'setEncoding':
      case 'setKeepAlive':
      case 'setNoDelay':
      case 'ref':
      case 'unref':
        throw new ERR_HTTP2_NO_SOCKET_MANIPULATION();
      default:
        const socket = session[kSocket];
        if (socket === undefined)
          throw new ERR_HTTP2_SOCKET_UNBOUND();
        socket[prop] = value;
        return true;
    }

@mcollina
Copy link
Member

@szmarczak good spot! would you like to open a PR to fix that?

@szmarczak
Copy link
Member

Yeah, sure.

@szmarczak
Copy link
Member

@mcollina Here it is: #22650

targos pushed a commit that referenced this pull request Sep 3, 2018
Fixes: #22268

PR-URL: #22486
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
mcollina pushed a commit that referenced this pull request Sep 5, 2018
Refs: #22486

PR-URL: #22650
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 5, 2018
Refs: #22486

PR-URL: #22650
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Fixes: #22268

PR-URL: #22486
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
Refs: #22486

PR-URL: #22650
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 3, 2018
Fixes: nodejs#22268

PR-URL: nodejs#22486
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 3, 2018
Refs: nodejs#22486

PR-URL: nodejs#22650
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Fixes: nodejs#22268

PR-URL: nodejs#22486
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Refs: nodejs#22486

PR-URL: nodejs#22650
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Fixes: #22268

Backport-PR-URL: #22850
PR-URL: #22486
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Refs: #22486

Backport-PR-URL: #22850
PR-URL: #22650
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessing socket after HTTP2 session gets destroyed throws an error
7 participants