From aebdf6fc4ab86af7090ba25ca0024e754182c05e Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Tue, 19 Apr 2016 10:47:55 -0400 Subject: [PATCH 1/3] deps: update to http-parser 2.7.0 Adds `2` as a return value of `on_headers_complete`, this mode will be used to fix handling responses to `CONNECT` requests. See: https://github.com/nodejs/node/pull/6198 --- deps/http_parser/Makefile | 4 +- deps/http_parser/http_parser.c | 3 + deps/http_parser/http_parser.h | 9 ++- deps/http_parser/test.c | 103 +++++++++++++++++++++++++++------ 4 files changed, 98 insertions(+), 21 deletions(-) diff --git a/deps/http_parser/Makefile b/deps/http_parser/Makefile index 970bdc42635ae1..5f4eb2252f241b 100644 --- a/deps/http_parser/Makefile +++ b/deps/http_parser/Makefile @@ -22,14 +22,14 @@ PLATFORM ?= $(shell sh -c 'uname -s | tr "[A-Z]" "[a-z]"') HELPER ?= BINEXT ?= ifeq (darwin,$(PLATFORM)) -SONAME ?= libhttp_parser.2.6.2.dylib +SONAME ?= libhttp_parser.2.7.0.dylib SOEXT ?= dylib else ifeq (wine,$(PLATFORM)) CC = winegcc BINEXT = .exe.so HELPER = wine else -SONAME ?= libhttp_parser.so.2.6.2 +SONAME ?= libhttp_parser.so.2.7.0 SOEXT ?= so endif diff --git a/deps/http_parser/http_parser.c b/deps/http_parser/http_parser.c index d51a2e7bcdf30a..719617549d0cb1 100644 --- a/deps/http_parser/http_parser.c +++ b/deps/http_parser/http_parser.c @@ -1812,6 +1812,9 @@ size_t http_parser_execute (http_parser *parser, case 0: break; + case 2: + parser->upgrade = 1; + case 1: parser->flags |= F_SKIPBODY; break; diff --git a/deps/http_parser/http_parser.h b/deps/http_parser/http_parser.h index 0cee4cc85b3892..105ae510a8ab57 100644 --- a/deps/http_parser/http_parser.h +++ b/deps/http_parser/http_parser.h @@ -26,8 +26,8 @@ extern "C" { /* Also update SONAME in the Makefile whenever you change these. */ #define HTTP_PARSER_VERSION_MAJOR 2 -#define HTTP_PARSER_VERSION_MINOR 6 -#define HTTP_PARSER_VERSION_PATCH 2 +#define HTTP_PARSER_VERSION_MINOR 7 +#define HTTP_PARSER_VERSION_PATCH 0 #include #if defined(_WIN32) && !defined(__MINGW32__) && \ @@ -77,6 +77,11 @@ typedef struct http_parser_settings http_parser_settings; * HEAD request which may contain 'Content-Length' or 'Transfer-Encoding: * chunked' headers that indicate the presence of a body. * + * Returning `2` from on_headers_complete will tell parser that it should not + * expect neither a body nor any futher responses on this connection. This is + * useful for handling responses to a CONNECT request which may not contain + * `Upgrade` or `Connection: upgrade` headers. + * * http_data_cb does not return data chunks. It will be called arbitrarily * many times for each string. E.G. you might get 10 callbacks for "on_url" * each providing just a few characters more data. diff --git a/deps/http_parser/test.c b/deps/http_parser/test.c index 7b01dc346b63d9..456a78add05e68 100644 --- a/deps/http_parser/test.c +++ b/deps/http_parser/test.c @@ -2173,6 +2173,20 @@ pause_chunk_complete_cb (http_parser *p) return chunk_complete_cb(p); } +int +connect_headers_complete_cb (http_parser *p) +{ + headers_complete_cb(p); + return 1; +} + +int +connect_message_complete_cb (http_parser *p) +{ + messages[num_messages].should_keep_alive = http_should_keep_alive(parser); + return message_complete_cb(p); +} + static http_parser_settings settings_pause = {.on_message_begin = pause_message_begin_cb ,.on_header_field = pause_header_field_cb @@ -2212,6 +2226,19 @@ static http_parser_settings settings_count_body = ,.on_chunk_complete = chunk_complete_cb }; +static http_parser_settings settings_connect = + {.on_message_begin = message_begin_cb + ,.on_header_field = header_field_cb + ,.on_header_value = header_value_cb + ,.on_url = request_url_cb + ,.on_status = response_status_cb + ,.on_body = dontcall_body_cb + ,.on_headers_complete = connect_headers_complete_cb + ,.on_message_complete = connect_message_complete_cb + ,.on_chunk_header = chunk_header_cb + ,.on_chunk_complete = chunk_complete_cb + }; + static http_parser_settings settings_null = {.on_message_begin = 0 ,.on_header_field = 0 @@ -2275,6 +2302,14 @@ size_t parse_pause (const char *buf, size_t len) return nparsed; } +size_t parse_connect (const char *buf, size_t len) +{ + size_t nparsed; + currently_parsing_eof = (len == 0); + nparsed = http_parser_execute(parser, &settings_connect, buf, len); + return nparsed; +} + static inline int check_str_eq (const struct message *m, const char *prop, @@ -2331,7 +2366,7 @@ do { \ } while(0) int -message_eq (int index, const struct message *expected) +message_eq (int index, int connect, const struct message *expected) { int i; struct message *m = &messages[index]; @@ -2346,8 +2381,10 @@ message_eq (int index, const struct message *expected) MESSAGE_CHECK_STR_EQ(expected, m, response_status); } - MESSAGE_CHECK_NUM_EQ(expected, m, should_keep_alive); - MESSAGE_CHECK_NUM_EQ(expected, m, message_complete_on_eof); + if (!connect) { + MESSAGE_CHECK_NUM_EQ(expected, m, should_keep_alive); + MESSAGE_CHECK_NUM_EQ(expected, m, message_complete_on_eof); + } assert(m->message_begin_cb_called); assert(m->headers_complete_cb_called); @@ -2385,16 +2422,22 @@ message_eq (int index, const struct message *expected) MESSAGE_CHECK_NUM_EQ(expected, m, port); } - if (expected->body_size) { + if (connect) { + check_num_eq(m, "body_size", 0, m->body_size); + } else if (expected->body_size) { MESSAGE_CHECK_NUM_EQ(expected, m, body_size); } else { MESSAGE_CHECK_STR_EQ(expected, m, body); } - assert(m->num_chunks == m->num_chunks_complete); - MESSAGE_CHECK_NUM_EQ(expected, m, num_chunks_complete); - for (i = 0; i < m->num_chunks && i < MAX_CHUNKS; i++) { - MESSAGE_CHECK_NUM_EQ(expected, m, chunk_lengths[i]); + if (connect) { + check_num_eq(m, "num_chunks_complete", 0, m->num_chunks_complete); + } else { + assert(m->num_chunks == m->num_chunks_complete); + MESSAGE_CHECK_NUM_EQ(expected, m, num_chunks_complete); + for (i = 0; i < m->num_chunks && i < MAX_CHUNKS; i++) { + MESSAGE_CHECK_NUM_EQ(expected, m, chunk_lengths[i]); + } } MESSAGE_CHECK_NUM_EQ(expected, m, num_headers); @@ -3201,7 +3244,7 @@ test_message (const struct message *message) abort(); } - if(!message_eq(0, message)) abort(); + if(!message_eq(0, 0, message)) abort(); parser_free(); } @@ -3238,7 +3281,7 @@ test_message_count_body (const struct message *message) abort(); } - if(!message_eq(0, message)) abort(); + if(!message_eq(0, 0, message)) abort(); parser_free(); } @@ -3589,9 +3632,9 @@ test_multiple3 (const struct message *r1, const struct message *r2, const struct abort(); } - if (!message_eq(0, r1)) abort(); - if (message_count > 1 && !message_eq(1, r2)) abort(); - if (message_count > 2 && !message_eq(2, r3)) abort(); + if (!message_eq(0, 0, r1)) abort(); + if (message_count > 1 && !message_eq(1, 0, r2)) abort(); + if (message_count > 2 && !message_eq(2, 0, r3)) abort(); parser_free(); } @@ -3687,17 +3730,17 @@ test_scan (const struct message *r1, const struct message *r2, const struct mess goto error; } - if (!message_eq(0, r1)) { + if (!message_eq(0, 0, r1)) { fprintf(stderr, "\n\nError matching messages[0] in test_scan.\n"); goto error; } - if (message_count > 1 && !message_eq(1, r2)) { + if (message_count > 1 && !message_eq(1, 0, r2)) { fprintf(stderr, "\n\nError matching messages[1] in test_scan.\n"); goto error; } - if (message_count > 2 && !message_eq(2, r3)) { + if (message_count > 2 && !message_eq(2, 0, r3)) { fprintf(stderr, "\n\nError matching messages[2] in test_scan.\n"); goto error; } @@ -3796,7 +3839,29 @@ test_message_pause (const struct message *msg) abort(); } - if(!message_eq(0, msg)) abort(); + if(!message_eq(0, 0, msg)) abort(); + + parser_free(); +} + +/* Verify that body and next message won't be parsed in responses to CONNECT */ +void +test_message_connect (const struct message *msg) +{ + char *buf = (char*) msg->raw; + size_t buflen = strlen(msg->raw); + size_t nread; + + parser_init(msg->type); + + nread = parse_connect(buf, buflen); + + if (num_messages != 1) { + printf("\n*** num_messages != 1 after testing '%s' ***\n\n", msg->name); + abort(); + } + + if(!message_eq(0, 1, msg)) abort(); parser_free(); } @@ -3867,6 +3932,10 @@ main (void) test_message_pause(&responses[i]); } + for (i = 0; i < response_count; i++) { + test_message_connect(&responses[i]); + } + for (i = 0; i < response_count; i++) { if (!responses[i].should_keep_alive) continue; for (j = 0; j < response_count; j++) { From e4116920d58f6bca093f1c4270b33f8d838231a4 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Tue, 19 Apr 2016 10:51:05 -0400 Subject: [PATCH 2/3] http: skip body and next message of CONNECT res When handling a response to `CONNECT` request - skip message body and do not attempt to parse the next message. `CONNECT` requests are used in similar sense to HTTP Upgrade. Fix: https://github.com/nodejs/node/pull/6198 --- lib/_http_client.js | 2 +- lib/_http_common.js | 7 +++++-- src/node_http_parser.cc | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index bc294263c7f881..68eb125e0e10e9 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -432,7 +432,7 @@ function parserOnIncomingClient(res, shouldKeepAlive) { // Responses to CONNECT request is handled as Upgrade. if (req.method === 'CONNECT') { res.upgrade = true; - return true; // skip body + return 2; // skip body, and the rest } // Responses to HEAD requests are crazy. diff --git a/lib/_http_common.js b/lib/_http_common.js index 7c9fac5c6b7671..583c5d610a5ed9 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -96,7 +96,7 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, parser.incoming.upgrade = upgrade; - var skipBody = false; // response to HEAD or CONNECT + var skipBody = 0; // response to HEAD or CONNECT if (!upgrade) { // For upgraded connections and CONNECT method request, we'll emit this @@ -105,7 +105,10 @@ function parserOnHeadersComplete(versionMajor, versionMinor, headers, method, skipBody = parser.onIncoming(parser.incoming, shouldKeepAlive); } - return skipBody; + if (typeof skipBody !== 'number') + return skipBody ? 1 : 0; + else + return skipBody; } // XXX This is a mess. diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 7d30cfd99ee19b..d1a130d7eead04 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -300,7 +300,7 @@ class Parser : public AsyncWrap { return -1; } - return head_response->IsTrue() ? 1 : 0; + return head_response->IntegerValue(); } From 3b07864a4b19933f10d542263b439a07953ac592 Mon Sep 17 00:00:00 2001 From: Josh Leder Date: Tue, 19 Apr 2016 10:50:20 -0400 Subject: [PATCH 3/3] test: add test for responses to HTTP CONNECT req See: https://github.com/nodejs/node/pull/6198 --- test/parallel/test-http-connect-req-res.js | 72 ++++++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 test/parallel/test-http-connect-req-res.js diff --git a/test/parallel/test-http-connect-req-res.js b/test/parallel/test-http-connect-req-res.js new file mode 100644 index 00000000000000..bb5056fce49a24 --- /dev/null +++ b/test/parallel/test-http-connect-req-res.js @@ -0,0 +1,72 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +const server = http.createServer(function(req, res) { + assert(false); +}); +server.on('connect', common.mustCall(function(req, socket, firstBodyChunk) { + assert.equal(req.method, 'CONNECT'); + assert.equal(req.url, 'example.com:443'); + console.error('Server got CONNECT request'); + + // It is legal for the server to send some data intended for the client + // along with the CONNECT response + socket.write( + 'HTTP/1.1 200 Connection established\r\n' + + 'Date: Tue, 15 Nov 1994 08:12:31 GMT\r\n' + + '\r\n' + + 'Head' + ); + + var data = firstBodyChunk.toString(); + socket.on('data', function(buf) { + data += buf.toString(); + }); + socket.on('end', function() { + socket.end(data); + }); +})); +server.listen(common.PORT, common.mustCall(function() { + const req = http.request({ + port: common.PORT, + method: 'CONNECT', + path: 'example.com:443' + }, function(res) { + assert(false); + }); + + req.on('close', common.mustCall(function() { })); + + req.on('connect', common.mustCall(function(res, socket, firstBodyChunk) { + console.error('Client got CONNECT request'); + + // Make sure this request got removed from the pool. + const name = 'localhost:' + common.PORT; + assert(!http.globalAgent.sockets.hasOwnProperty(name)); + assert(!http.globalAgent.requests.hasOwnProperty(name)); + + // Make sure this socket has detached. + assert(!socket.ondata); + assert(!socket.onend); + assert.equal(socket.listeners('connect').length, 0); + assert.equal(socket.listeners('data').length, 0); + + var data = firstBodyChunk.toString(); + + // test that the firstBodyChunk was not parsed as HTTP + assert.equal(data, 'Head'); + + socket.on('data', function(buf) { + data += buf.toString(); + }); + socket.on('end', function() { + assert.equal(data, 'HeadRequestEnd'); + server.close(); + }); + socket.end('End'); + })); + + req.end('Request'); +}));