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

http: check for existance in resetHeadersTimeoutOnReqEnd #26402

Closed
wants to merge 1 commit into from

Conversation

@mcollina
Copy link
Member

mcollina commented Mar 2, 2019

socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26366

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
@mcollina mcollina requested review from addaleax and rvagg Mar 2, 2019
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 2, 2019

cc @nodejs/http

@nodejs/lts @nodejs/release we would likely have to backport this down to 6 for safety, given that we do not know how this condition is triggered.

@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 2, 2019

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Mar 2, 2019

socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26357

Shouldn't that be #26366? (Commit message too.)

@mcollina mcollina force-pushed the mcollina:fix-null-check branch from fedaa10 to 979c71c Mar 2, 2019
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 2, 2019

@richardlau good spot! Fixed.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 2, 2019

Optional typo fix for commit title: s/existance/existence/

@cjihrig
cjihrig approved these changes Mar 2, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 2, 2019

Here's a test that reproduces the error in #26366 in current master.

'use strict';

require('../common');

const http = require('http');

const server = http.createServer((req, res) => {
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.write('okay', () => { delete res.socket.parser });
  res.end();
});

server.listen(1337, '127.0.0.1');

const req = http.request({
  port: 1337,
  host: '127.0.0.1',
  method: 'GET',
});

req.end();
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 2, 2019

Is it worth adding the code in the previous comment (or something like it) as a test?

@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 2, 2019

I think so. However it’s not clear if we are doing it in core or not, or it is just user specific (somehow).

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 2, 2019

By the way, #26404 is basically the same thing but on the client end rather than the server end.

@@ -755,7 +755,7 @@ function resetHeadersTimeoutOnReqEnd() {
const parser = this.socket.parser;
// Parser can be null if the socket was destroyed
// in that case, there is nothing to do.
if (parser !== null) {
if (parser) {

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 2, 2019

Member

Changing to parser != null would work also and be a bit safer

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Mar 3, 2019

I think we need to understand why this happens or we could mask a bug instead of fixing it. It would be great if @bjjenson or @jasine could provide a test case to reproduce the issue.

@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 3, 2019

The overall problem with supporting a “delete” case is that it could trigger the vulnerability we are trying to protect against.

socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26366
@mcollina mcollina force-pushed the mcollina:fix-null-check branch from 979c71c to 0c3621d Mar 5, 2019
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 6, 2019

@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 6, 2019

Landed in 3c83f93

@mcollina mcollina closed this Mar 6, 2019
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Mar 6, 2019

@nodejs/lts this should be backported asap to all lines.

gireeshpunathil pushed a commit to gireeshpunathil/node that referenced this pull request Mar 6, 2019
socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: nodejs#26366

PR-URL: nodejs#26402
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Mar 6, 2019

@nodejs/lts this should be backported asap to all lines.

Probably too late for 11.11.0, but ping @BridgeAR.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 6, 2019

@richardlau I would rather pull that into the release afterwards.

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Mar 7, 2019

I've finally found the root issue behind #26366 or better in https://github.com/eggjs/egg-socket.io.

The problem is that our request.socket is replaced by egg-socket.io with the socket.io Socket. See https://github.com/eggjs/egg-socket.io/blob/9e7f71d835930d3a63c69635488737066f779661/lib/connectionMiddlewareInit.js#L8-L9.

There is nothing wrong with this fix but the problem is in egg-socket.io and may arise again.

I think the regression test added here does not make much sense.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: nodejs#26366

PR-URL: nodejs#26402
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BridgeAR added a commit that referenced this pull request Mar 14, 2019
socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26366

PR-URL: #26402
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs added a commit that referenced this pull request Apr 16, 2019
socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26366

PR-URL: #26402
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BethGriggs BethGriggs referenced this pull request May 1, 2019
BethGriggs added a commit that referenced this pull request Sep 19, 2019
socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26366

PR-URL: #26402
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@BethGriggs BethGriggs referenced this pull request Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.