From 4f4c8ab3b4cea246d2ece6ca006fe280241d84a4 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 3 Feb 2016 17:28:48 -0800 Subject: [PATCH] deps: update http-parser to version 2.6.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit includes parsing improvements to ensure closer HTTP spec conformance PR-URL: https://github.com/nodejs/node-private/pull/26 Reviewed-By: Rod Vagg Reviewed-By: Сковорода Никита Андреевич Reviewed-By: Ben Noordhuis --- deps/http_parser/Makefile | 4 +- deps/http_parser/http_parser.c | 32 +++- deps/http_parser/http_parser.h | 12 +- deps/http_parser/test.c | 161 ++++++++++++++++++ ...ient-reject-chunked-with-content-length.js | 28 +++ .../test-http-client-reject-cr-no-lf.js | 27 +++ .../test-http-double-content-length.js | 33 ++++ ...test-http-response-multi-content-length.js | 52 ++++++ .../test-http-response-multiheaders.js | 8 +- .../test-http-server-multiheaders2.js | 6 +- ...rver-reject-chunked-with-content-length.js | 31 ++++ .../test-http-server-reject-cr-no-lf.js | 33 ++++ 12 files changed, 409 insertions(+), 18 deletions(-) create mode 100644 test/parallel/test-http-client-reject-chunked-with-content-length.js create mode 100644 test/parallel/test-http-client-reject-cr-no-lf.js create mode 100644 test/parallel/test-http-double-content-length.js create mode 100644 test/parallel/test-http-response-multi-content-length.js create mode 100644 test/parallel/test-http-server-reject-chunked-with-content-length.js create mode 100644 test/parallel/test-http-server-reject-cr-no-lf.js diff --git a/deps/http_parser/Makefile b/deps/http_parser/Makefile index 33c8ba0239..b3e0ff4ae0 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.0.dylib +SONAME ?= libhttp_parser.2.6.1.dylib SOEXT ?= dylib else ifeq (wine,$(PLATFORM)) CC = winegcc BINEXT = .exe.so HELPER = wine else -SONAME ?= libhttp_parser.so.2.6.0 +SONAME ?= libhttp_parser.so.2.6.1 SOEXT ?= so endif diff --git a/deps/http_parser/http_parser.c b/deps/http_parser/http_parser.c index b3037d7e3d..33d29bb175 100644 --- a/deps/http_parser/http_parser.c +++ b/deps/http_parser/http_parser.c @@ -435,6 +435,12 @@ enum http_host_state (IS_ALPHANUM(c) || (c) == '.' || (c) == '-' || (c) == '_') #endif +/** + * Verify that a char is a valid visible (printable) US-ASCII + * character or %x80-FF + **/ +#define IS_HEADER_CHAR(ch) \ + (ch == CR || ch == LF || ch == 9 || (ch > 31 && ch != 127)) #define start_state (parser->type == HTTP_REQUEST ? s_start_req : s_start_res) @@ -639,7 +645,8 @@ size_t http_parser_execute (http_parser *parser, const char *body_mark = 0; const char *status_mark = 0; enum state p_state = (enum state) parser->state; - + const unsigned int lenient = parser->lenient_http_headers; + /* We're in an error state. Don't bother doing anything. */ if (HTTP_PARSER_ERRNO(parser) != HPE_OK) { return 0; @@ -1408,7 +1415,12 @@ reexecute: || c != CONTENT_LENGTH[parser->index]) { parser->header_state = h_general; } else if (parser->index == sizeof(CONTENT_LENGTH)-2) { + if (parser->flags & F_CONTENTLENGTH) { + SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH); + goto error; + } parser->header_state = h_content_length; + parser->flags |= F_CONTENTLENGTH; } break; @@ -1560,6 +1572,11 @@ reexecute: REEXECUTE(); } + if (!lenient && !IS_HEADER_CHAR(ch)) { + SET_ERRNO(HPE_INVALID_HEADER_TOKEN); + goto error; + } + c = LOWER(ch); switch (h_state) { @@ -1727,7 +1744,10 @@ reexecute: case s_header_almost_done: { - STRICT_CHECK(ch != LF); + if (UNLIKELY(ch != LF)) { + SET_ERRNO(HPE_LF_EXPECTED); + goto error; + } UPDATE_STATE(s_header_value_lws); break; @@ -1811,6 +1831,14 @@ reexecute: REEXECUTE(); } + /* Cannot use chunked encoding and a content-length header together + per the HTTP specification. */ + if ((parser->flags & F_CHUNKED) && + (parser->flags & F_CONTENTLENGTH)) { + SET_ERRNO(HPE_UNEXPECTED_CONTENT_LENGTH); + goto error; + } + UPDATE_STATE(s_headers_done); /* Set this here so that on_headers_complete() callbacks can see it */ diff --git a/deps/http_parser/http_parser.h b/deps/http_parser/http_parser.h index 34bebf3d36..e33c0620a1 100644 --- a/deps/http_parser/http_parser.h +++ b/deps/http_parser/http_parser.h @@ -27,7 +27,7 @@ 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 0 +#define HTTP_PARSER_VERSION_PATCH 1 #include #if defined(_WIN32) && !defined(__MINGW32__) && \ @@ -148,6 +148,7 @@ enum flags , F_TRAILING = 1 << 4 , F_UPGRADE = 1 << 5 , F_SKIPBODY = 1 << 6 + , F_CONTENTLENGTH = 1 << 7 }; @@ -190,6 +191,8 @@ enum flags XX(INVALID_HEADER_TOKEN, "invalid character in header") \ XX(INVALID_CONTENT_LENGTH, \ "invalid character in content-length header") \ + XX(UNEXPECTED_CONTENT_LENGTH, \ + "unexpected content-length header") \ XX(INVALID_CHUNK_SIZE, \ "invalid character in chunk size header") \ XX(INVALID_CONSTANT, "invalid constant string") \ @@ -214,10 +217,11 @@ enum http_errno { struct http_parser { /** PRIVATE **/ unsigned int type : 2; /* enum http_parser_type */ - unsigned int flags : 7; /* F_* values from 'flags' enum; semi-public */ + unsigned int flags : 8; /* F_* values from 'flags' enum; semi-public */ unsigned int state : 7; /* enum state from http_parser.c */ - unsigned int header_state : 8; /* enum header_state from http_parser.c */ - unsigned int index : 8; /* index into current matcher */ + unsigned int header_state : 7; /* enum header_state from http_parser.c */ + unsigned int index : 7; /* index into current matcher */ + unsigned int lenient_http_headers : 1; uint32_t nread; /* # bytes read in various scenarios */ uint64_t content_length; /* # bytes in body (0 if no Content-Length header) */ diff --git a/deps/http_parser/test.c b/deps/http_parser/test.c index c63b8f0b0b..87345bb0dc 100644 --- a/deps/http_parser/test.c +++ b/deps/http_parser/test.c @@ -3270,6 +3270,155 @@ test_simple (const char *buf, enum http_errno err_expected) } } +void +test_invalid_header_content (int req, const char* str) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = str; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_INVALID_HEADER_TOKEN); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in invalid header content test ***\n"); + abort(); +} + +void +test_invalid_header_field_content_error (int req) +{ + test_invalid_header_content(req, "Foo: F\01ailure"); + test_invalid_header_content(req, "Foo: B\02ar"); +} + +void +test_invalid_header_field (int req, const char* str) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = str; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_INVALID_HEADER_TOKEN); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in invalid header token test ***\n"); + abort(); +} + +void +test_invalid_header_field_token_error (int req) +{ + test_invalid_header_field(req, "Fo@: Failure"); + test_invalid_header_field(req, "Foo\01\test: Bar"); +} + +void +test_double_content_length_error (int req) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = "Content-Length: 0\r\nContent-Length: 1\r\n\r\n"; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_MULTIPLE_CONTENT_LENGTH); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in double content-length test ***\n"); + abort(); +} + +void +test_chunked_content_length_error (int req) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = "Transfer-Encoding: chunked\r\nContent-Length: 1\r\n\r\n"; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_CHUNKED_WITH_CONTENT_LENGTH); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in chunked content-length test ***\n"); + abort(); +} + +void +test_header_cr_no_lf_error (int req) +{ + http_parser parser; + http_parser_init(&parser, req ? HTTP_REQUEST : HTTP_RESPONSE); + size_t parsed; + const char *buf; + buf = req ? + "GET / HTTP/1.1\r\n" : + "HTTP/1.1 200 OK\r\n"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = "Foo: 1\rBar: 1\r\n\r\n"; + size_t buflen = strlen(buf); + + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_LF_EXPECTED); + return; + } + + fprintf(stderr, + "\n*** Error expected but none in header whitespace test ***\n"); + abort(); +} + void test_header_overflow_error (int req) { @@ -3696,6 +3845,18 @@ main (void) test_header_content_length_overflow_error(); test_chunk_content_length_overflow_error(); + //// HEADER FIELD CONDITIONS + test_double_content_length_error(HTTP_REQUEST); + test_chunked_content_length_error(HTTP_REQUEST); + test_header_cr_no_lf_error(HTTP_REQUEST); + test_invalid_header_field_token_error(HTTP_REQUEST); + test_invalid_header_field_content_error(HTTP_REQUEST); + test_double_content_length_error(HTTP_RESPONSE); + test_chunked_content_length_error(HTTP_RESPONSE); + test_header_cr_no_lf_error(HTTP_RESPONSE); + test_invalid_header_field_token_error(HTTP_RESPONSE); + test_invalid_header_field_content_error(HTTP_RESPONSE); + //// RESPONSES for (i = 0; i < response_count; i++) { diff --git a/test/parallel/test-http-client-reject-chunked-with-content-length.js b/test/parallel/test-http-client-reject-chunked-with-content-length.js new file mode 100644 index 0000000000..a6639c9069 --- /dev/null +++ b/test/parallel/test-http-client-reject-chunked-with-content-length.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +const reqstr = 'HTTP/1.1 200 OK\r\n' + + 'Content-Length: 1\r\n' + + 'Transfer-Encoding: chunked\r\n\r\n'; + +const server = net.createServer((socket) => { + socket.write(reqstr); +}); + +server.listen(common.PORT, () => { + // The callback should not be called because the server is sending + // both a Content-Length header and a Transfer-Encoding: chunked + // header, which is a violation of the HTTP spec. + const req = http.get({port:common.PORT}, (res) => { + assert.fail(null, null, 'callback should not be called'); + }); + req.on('error', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + server.close(); + })); +}); diff --git a/test/parallel/test-http-client-reject-cr-no-lf.js b/test/parallel/test-http-client-reject-cr-no-lf.js new file mode 100644 index 0000000000..b60220cbb6 --- /dev/null +++ b/test/parallel/test-http-client-reject-cr-no-lf.js @@ -0,0 +1,27 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +const reqstr = 'HTTP/1.1 200 OK\r\n' + + 'Foo: Bar\r' + + 'Content-Length: 1\r\n\r\n'; + +const server = net.createServer((socket) => { + socket.write(reqstr); +}); + +server.listen(common.PORT, () => { + // The callback should not be called because the server is sending a + // header field that ends only in \r with no following \n + const req = http.get({port:common.PORT}, (res) => { + assert.fail(null, null, 'callback should not be called'); + }); + req.on('error', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_LF_EXPECTED'); + server.close(); + })); +}); diff --git a/test/parallel/test-http-double-content-length.js b/test/parallel/test-http-double-content-length.js new file mode 100644 index 0000000000..c35f18321c --- /dev/null +++ b/test/parallel/test-http-double-content-length.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); + +// The callback should never be invoked because the server +// should respond with a 400 Client Error when a double +// Content-Length header is received. +const server = http.createServer((req, res) => { + assert(false, 'callback should not have been invoked'); + res.end(); +}); +server.on('clientError', common.mustCall((err, socket) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + socket.destroy(); +})); + +server.listen(common.PORT, () => { + const req = http.get({ + port: common.PORT, + // Send two content-length header values. + headers: {'Content-Length': [1, 2]}}, + (res) => { + assert.fail(null, null, 'an error should have occurred'); + server.close(); + } + ); + req.on('error', common.mustCall(() => { + server.close(); + })); +}); diff --git a/test/parallel/test-http-response-multi-content-length.js b/test/parallel/test-http-response-multi-content-length.js new file mode 100644 index 0000000000..4b0f2c11e3 --- /dev/null +++ b/test/parallel/test-http-response-multi-content-length.js @@ -0,0 +1,52 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); + +const MAX_COUNT = 2; + +const server = http.createServer((req, res) => { + const num = req.headers['x-num']; + // TODO(@jasnell) At some point this should be refactored as the API + // should not be allowing users to set multiple content-length values + // in the first place. + switch (num) { + case '1': + res.setHeader('content-length', [2, 1]); + break; + case '2': + res.writeHead(200, {'content-length': [1, 2]}); + break; + default: + assert.fail(null, null, 'should never get here'); + } + res.end('ok'); +}); + +var count = 0; + +server.listen(common.PORT, common.mustCall(() => { + for (let n = 1; n <= MAX_COUNT ; n++) { + // This runs twice, the first time, the server will use + // setHeader, the second time it uses writeHead. In either + // case, the error handler must be called because the client + // is not allowed to accept multiple content-length headers. + http.get( + {port:common.PORT, headers:{'x-num': n}}, + (res) => { + assert(false, 'client allowed multiple content-length headers.'); + } + ).on('error', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + count++; + if (count === MAX_COUNT) + server.close(); + })); + } +})); + +process.on('exit', () => { + assert.equal(count, MAX_COUNT); +}); diff --git a/test/parallel/test-http-response-multiheaders.js b/test/parallel/test-http-response-multiheaders.js index 83171bb475..46265053ad 100644 --- a/test/parallel/test-http-response-multiheaders.js +++ b/test/parallel/test-http-response-multiheaders.js @@ -5,8 +5,9 @@ const http = require('http'); const assert = require('assert'); // Test that certain response header fields do not repeat. -// 'content-length' should also be in this list, but it needs -// a numeric value, so it's tested slightly differently. +// 'content-length' should also be in this list but it is +// handled differently because multiple content-lengths are +// an error (see test-http-response-multi-content-length.js). const norepeat = [ 'content-type', 'user-agent', @@ -30,14 +31,12 @@ const norepeat = [ const server = http.createServer(function(req, res) { var num = req.headers['x-num']; if (num == 1) { - res.setHeader('content-length', [1, 2]); for (const name of norepeat) { res.setHeader(name, ['A', 'B']); } res.setHeader('X-A', ['A', 'B']); } else if (num == 2) { const headers = {}; - headers['content-length'] = [1, 2]; for (const name of norepeat) { headers[name] = ['A', 'B']; } @@ -60,7 +59,6 @@ server.listen(common.PORT, common.mustCall(function() { {port:common.PORT, headers:{'x-num': n}}, common.mustCall(function(res) { if (++count === 2) server.close(); - assert.equal(res.headers['content-length'], 1); for (const name of norepeat) { assert.equal(res.headers[name], 'A'); } diff --git a/test/parallel/test-http-server-multiheaders2.js b/test/parallel/test-http-server-multiheaders2.js index 438e8ba759..bf54af3465 100644 --- a/test/parallel/test-http-server-multiheaders2.js +++ b/test/parallel/test-http-server-multiheaders2.js @@ -57,7 +57,6 @@ var srv = http.createServer(function(req, res) { assert.equal(req.headers[header.toLowerCase()], 'foo, bar', 'header parsed incorrectly: ' + header); }); - assert.equal(req.headers['content-length'], 0); res.writeHead(200, {'Content-Type' : 'text/plain'}); res.end('EOF'); @@ -75,10 +74,7 @@ var headers = [] .concat(multipleAllowed.map(makeHeader('foo'))) .concat(multipleForbidden.map(makeHeader('foo'))) .concat(multipleAllowed.map(makeHeader('bar'))) - .concat(multipleForbidden.map(makeHeader('bar'))) - // content-length is a special case since node.js - // is dropping connetions with non-numeric headers - .concat([['content-length', 0], ['content-length', 123]]); + .concat(multipleForbidden.map(makeHeader('bar'))); srv.listen(common.PORT, function() { http.get({ diff --git a/test/parallel/test-http-server-reject-chunked-with-content-length.js b/test/parallel/test-http-server-reject-chunked-with-content-length.js new file mode 100644 index 0000000000..42cc1e83eb --- /dev/null +++ b/test/parallel/test-http-server-reject-chunked-with-content-length.js @@ -0,0 +1,31 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const net = require('net'); +const assert = require('assert'); + +const reqstr = 'POST / HTTP/1.1\r\n' + + 'Content-Length: 1\r\n' + + 'Transfer-Encoding: chunked\r\n\r\n'; + +const server = http.createServer((req, res) => { + assert.fail(null, null, 'callback should not be invoked'); +}); +server.on('clientError', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_UNEXPECTED_CONTENT_LENGTH'); + server.close(); +})); +server.listen(common.PORT, () => { + const client = net.connect({port: common.PORT}, () => { + client.write(reqstr); + client.end(); + }); + client.on('data', (data) => { + // Should not get to this point because the server should simply + // close the connection without returning any data. + assert.fail(null, null, 'no data should be returned by the server'); + }); + client.on('end', common.mustCall(() => {})); +}); diff --git a/test/parallel/test-http-server-reject-cr-no-lf.js b/test/parallel/test-http-server-reject-cr-no-lf.js new file mode 100644 index 0000000000..fbb89f0ff3 --- /dev/null +++ b/test/parallel/test-http-server-reject-cr-no-lf.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common'); +const net = require('net'); +const http = require('http'); +const assert = require('assert'); + +const str = 'GET / HTTP/1.1\r\n' + + 'Dummy: Header\r' + + 'Content-Length: 1\r\n' + + '\r\n'; + + +const server = http.createServer((req, res) => { + assert.fail(null, null, 'this should not be called'); +}); +server.on('clientError', common.mustCall((err) => { + assert(/^Parse Error/.test(err.message)); + assert.equal(err.code, 'HPE_LF_EXPECTED'); + server.close(); +})); +server.listen(common.PORT, () => { + const client = net.connect({port:common.PORT}, () => { + client.on('data', (chunk) => { + assert.fail(null, null, 'this should not be called'); + }); + client.on('end', common.mustCall(() => { + server.close(); + })); + client.write(str); + client.end(); + }); +});