http: incoming parser set to null twice causes crash #4948

Closed
wants to merge 2 commits into
from

Projects

None yet

4 participants

@hnry
hnry commented Mar 7, 2013

The error:

http.js:423
  this.socket.parser.incoming = null;
                              ^
TypeError: Cannot set property 'incoming' of null
    at IncomingMessage._dump (http.js:423:31)
    at ServerResponse.<anonymous> (http.js:1979:13)
    at ServerResponse.EventEmitter.emit (events.js:91:17)
    at ServerResponse.OutgoingMessage._finish (http.js:963:8)
    at ServerResponse.OutgoingMessage.end (http.js:946:10)
    at IncomingMessage.<anonymous> (###### user code #####)
    at IncomingMessage.EventEmitter.emit (events.js:116:20)
    at _stream_readable.js:852:12
    at process._tickCallback (node.js:403:13)
@hnry
hnry commented Mar 7, 2013

it looks like freeParser already sets it to null

Would just checking if it's already been freed/set to null be ok?

@hnry
hnry commented Mar 8, 2013

By the way this is how to reproduce:

var http = require('http');
http.createServer(function(serverReq, serverRes){
  http.request({hostname: 'google.com', port: 80}, function(res) {
    res.on('data', function(data) { }); // this is required to reproduce
    serverRes.end(); // this is required this to reproduce
  }).end();
}).listen(8080);

Send a request to the server listening on 8080 and close the connection early.

The two required pieces of line to reproduce I'm guessing one is calling freeParser() and the other is calling _dump(). So this.socket.parser gets set to null twice.

@bnoordhuis
Member

Can you add a regression test in test/simple or test/internet? (One that preferably doesn't need external services.)

Also, please see https://github.com/joyent/node/blob/master/CONTRIBUTING.md - esp. the bits about the CLA and the commit log.

@hnry hnry http: check if incoming parser has already been freed
Fix #4948

This adds a check before setting the incoming parser
to null. Under certain circumstances it'll already be set to
null by freeParser().

Otherwise this will cause node to crash as it tries to set
null on something that is already null.
73380f5
@tjfontaine tjfontaine commented on an outdated diff Mar 8, 2013
test/internet/test-regress-GH-4948.js
+
+// https://github.com/joyent/node/issues/4948
+
+var http = require('http');
+
+http.createServer(function(serverReq, serverRes){
+
+ http.request({hostname: 'google.com', port: 80}, function(res) {
+ // required, just needs to be in the client response somewhere
+ serverRes.end();
+
+ // required for test to fail
+ res.on('data', function(data) {});
+ }).end();
+
+}).listen(8080);
@tjfontaine
tjfontaine Mar 8, 2013

this should be common.PORT

@tjfontaine tjfontaine commented on an outdated diff Mar 8, 2013
test/internet/test-regress-GH-4948.js
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+// https://github.com/joyent/node/issues/4948
+
+var http = require('http');
+
+http.createServer(function(serverReq, serverRes){
+
+ http.request({hostname: 'google.com', port: 80}, function(res) {
@tjfontaine
tjfontaine Mar 8, 2013

Does this test really need to contact an external site?

@hnry
hnry commented Mar 8, 2013

No it doesn't need to contact an external site, it can make a request to the created server. I just thought it would help clarify use case.

I can make the changes, but I noticed a lot of tests related to http are failing because of this bug. Should I include the test still?

[Update] Actually I'm going to include it, most of the tests that are failing due to this bug do not fail when running with node v0.9.12 (only v0.9.13-pre), my test fails for both

@isaacs isaacs added a commit that closed this pull request Mar 8, 2013
@hnry hnry http: check if incoming parser has already been freed
Fix #4948

This adds a check before setting the incoming parser
to null. Under certain circumstances it'll already be set to
null by freeParser().

Otherwise this will cause node to crash as it tries to set
null on something that is already null.
9f4c3b0
@isaacs isaacs closed this in 9f4c3b0 Mar 8, 2013
@isaacs
Collaborator
isaacs commented Mar 8, 2013

Thanks! Landed on 9f4c3b0.

@isaacs
Collaborator
isaacs commented Mar 8, 2013

Aaaannnd reverted on 632b7d8. My bad.

This test doesn't pass with the commit, and also makes a bunch of other tests fail.

@isaacs isaacs reopened this Mar 8, 2013
@isaacs isaacs commented on an outdated diff Mar 8, 2013
test/simple/test-regress-GH-4948.js
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+// https://github.com/joyent/node/issues/4948
+
+var common = require('../common');
+var http = require('http');
+
+http.createServer(function(serverReq, serverRes){
@isaacs
isaacs Mar 8, 2013 collaborator

This server listens, but never calls server.close() so the test script will stay open forever and time out.

@isaacs isaacs commented on an outdated diff Mar 8, 2013
test/simple/test-regress-GH-4948.js
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+// https://github.com/joyent/node/issues/4948
+
+var common = require('../common');
+var http = require('http');
+
+http.createServer(function(serverReq, serverRes){
+
+ http.request({hostname: 'localhost', port: common.PORT}, function(res) {
@isaacs
isaacs Mar 8, 2013 collaborator

How is this supposed to work? It gets the initial request, then makes a request to itself, which triggers another request to itself...

@hnry
hnry commented Mar 9, 2013

Sorry for the oversight

@hnry hnry regression test for #4948
this test crashes as it'll try to set the incoming parser
to null twice
2ba2b7d
@isaacs isaacs added a commit that referenced this pull request Mar 9, 2013
@hnry hnry http: check if incoming parser has already been freed
Fix #4948

This adds a check before setting the incoming parser
to null. Under certain circumstances it'll already be set to
null by freeParser().

Otherwise this will cause node to crash as it tries to set
null on something that is already null.
5757ce4
@isaacs
Collaborator
isaacs commented Mar 9, 2013

Landed in v0.10 on 5757ce4. Thanks!

@isaacs isaacs closed this Mar 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment