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: fix incorrect headersTimeout measurement (alt) #32329

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -727,7 +727,8 @@ function tickOnSocket(req, socket) {
new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req),
req.maxHeaderSize || 0,
req.insecureHTTPParser === undefined ?
isLenient() : req.insecureHTTPParser);
isLenient() : req.insecureHTTPParser,
0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the response parsing by the client the headers timeout is not relevant

parser.socket = socket;
parser.outgoing = req;
req.parser = parser;
Expand Down
2 changes: 2 additions & 0 deletions lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0;
const kOnBody = HTTPParser.kOnBody | 0;
const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0;
const kOnExecute = HTTPParser.kOnExecute | 0;
const kOnTimeout = HTTPParser.kOnTimeout | 0;

const MAX_HEADER_PAIRS = 2000;

Expand Down Expand Up @@ -165,6 +166,7 @@ const parsers = new FreeList('parsers', 1000, function parsersCb() {
parser[kOnHeadersComplete] = parserOnHeadersComplete;
parser[kOnBody] = parserOnBody;
parser[kOnMessageComplete] = parserOnMessageComplete;
parser[kOnTimeout] = null;

return parser;
});
Expand Down
45 changes: 11 additions & 34 deletions lib/_http_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ const { OutgoingMessage } = require('_http_outgoing');
const {
kOutHeaders,
kNeedDrain,
nowDate,
emitStatistics
} = require('internal/http');
const {
Expand Down Expand Up @@ -142,6 +141,7 @@ const STATUS_CODES = {
};

const kOnExecute = HTTPParser.kOnExecute | 0;
const kOnTimeout = HTTPParser.kOnTimeout | 0;

class HTTPServerAsyncResource {
constructor(type, socket) {
Expand Down Expand Up @@ -426,11 +426,9 @@ function connectionListenerInternal(server, socket) {
server.maxHeaderSize || 0,
server.insecureHTTPParser === undefined ?
isLenient() : server.insecureHTTPParser,
server.headersTimeout || 0,
);
parser.socket = socket;

// We are starting to wait for our headers.
parser.parsingHeadersStart = nowDate();
socket.parser = parser;

// Propagate headers limit from server instance to parser
Expand Down Expand Up @@ -482,6 +480,9 @@ function connectionListenerInternal(server, socket) {
parser[kOnExecute] =
onParserExecute.bind(undefined, server, socket, parser, state);

parser[kOnTimeout] =
onParserTimeout.bind(undefined, server, socket);

socket._paused = false;
}

Expand Down Expand Up @@ -570,21 +571,15 @@ function socketOnData(server, socket, parser, state, d) {

function onParserExecute(server, socket, parser, state, ret) {
socket._unrefTimer();
const start = parser.parsingHeadersStart;
debug('SERVER socketOnParserExecute %d', ret);
onParserExecuteCommon(server, socket, parser, state, ret, undefined);
}

// If we have not parsed the headers, destroy the socket
// after server.headersTimeout to protect from DoS attacks.
// start === 0 means that we have parsed headers.
if (start !== 0 && nowDate() - start > server.headersTimeout) {
const serverTimeout = server.emit('timeout', socket);

if (!serverTimeout)
socket.destroy();
return;
}
function onParserTimeout(server, socket) {
const serverTimeout = server.emit('timeout', socket);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

time tracking is moved to c++ code


onParserExecuteCommon(server, socket, parser, state, ret, undefined);
if (!serverTimeout)
socket.destroy();
}

const noop = () => {};
Expand Down Expand Up @@ -720,13 +715,6 @@ function emitCloseNT(self) {
function parserOnIncoming(server, socket, state, req, keepAlive) {
resetSocketTimeout(server, socket, state);

if (server.keepAliveTimeout > 0) {
req.on('end', resetHeadersTimeoutOnReqEnd);
}

// Set to zero to communicate that we have finished parsing.
socket.parser.parsingHeadersStart = 0;

if (req.upgrade) {
req.upgrade = req.method === 'CONNECT' ||
server.listenerCount('upgrade') > 0;
Expand Down Expand Up @@ -851,17 +839,6 @@ function generateSocketListenerWrapper(originalFnName) {
};
}

function resetHeadersTimeoutOnReqEnd() {
debug('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) {
parser.parsingHeadersStart = nowDate();
}
}

module.exports = {
STATUS_CODES,
Server,
Expand Down
37 changes: 35 additions & 2 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const uint32_t kOnHeadersComplete = 1;
const uint32_t kOnBody = 2;
const uint32_t kOnMessageComplete = 3;
const uint32_t kOnExecute = 4;
const uint32_t kOnTimeout = 5;
// Any more fields than this will be flushed into JS
const size_t kMaxHeaderFieldsCount = 32;

Expand Down Expand Up @@ -181,6 +182,7 @@ class Parser : public AsyncWrap, public StreamListener {
num_fields_ = num_values_ = 0;
url_.Reset();
status_message_.Reset();
header_parsing_start_time_ = uv_hrtime();
return 0;
}

Expand Down Expand Up @@ -504,6 +506,7 @@ class Parser : public AsyncWrap, public StreamListener {
bool lenient = args[3]->IsTrue();

uint64_t max_http_header_size = 0;
uint64_t headers_timeout = 0;

CHECK(args[0]->IsInt32());
CHECK(args[1]->IsObject());
Expand All @@ -516,6 +519,11 @@ class Parser : public AsyncWrap, public StreamListener {
max_http_header_size = env->options()->max_http_header_size;
}

if (args.Length() > 4) {
CHECK(args[4]->IsInt32());
headers_timeout = args[4].As<Number>()->Value();
}

llhttp_type_t type =
static_cast<llhttp_type_t>(args[0].As<Int32>()->Value());

Expand All @@ -532,7 +540,7 @@ class Parser : public AsyncWrap, public StreamListener {

parser->set_provider_type(provider);
parser->AsyncReset(args[1].As<Object>());
parser->Init(type, max_http_header_size, lenient);
parser->Init(type, max_http_header_size, lenient, headers_timeout);
}

template <bool should_pause>
Expand Down Expand Up @@ -636,6 +644,24 @@ class Parser : public AsyncWrap, public StreamListener {
if (ret.IsEmpty())
return;

// check header parsing time
if (header_parsing_start_time_ != 0 && headers_timeout_ != 0) {
uint64_t now = uv_hrtime();
uint64_t parsing_time = (now - header_parsing_start_time_) / 1e6;

if (parsing_time > headers_timeout_) {
Local<Value> cb =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a good way to avoid having an extra callback here?

Copy link
Member

Choose a reason for hiding this comment

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

@OrKoN I don’t think so. But if it only fires in the timeout case, that should be okay.

object()->Get(env()->context(), kOnTimeout).ToLocalChecked();

if (!cb->IsFunction())
return;

MakeCallback(cb.As<Function>(), 0, nullptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I check the return value? anything else needed to mark the parsing error?

Copy link
Member

Choose a reason for hiding this comment

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

@OrKoN Since you’d be return;ing here either way, whether the call threw an exception or not, I don’t think so. You can wrap the call in USE() to make it explicit that you’re discarding the return value because of that.


return;
}
}

Local<Value> cb =
object()->Get(env()->context(), kOnExecute).ToLocalChecked();

Expand Down Expand Up @@ -779,7 +805,8 @@ class Parser : public AsyncWrap, public StreamListener {
}


void Init(llhttp_type_t type, uint64_t max_http_header_size, bool lenient) {
void Init(llhttp_type_t type, uint64_t max_http_header_size,
bool lenient, uint64_t headers_timeout) {
llhttp_init(&parser_, type, &settings);
llhttp_set_lenient(&parser_, lenient);
header_nread_ = 0;
Expand All @@ -790,6 +817,8 @@ class Parser : public AsyncWrap, public StreamListener {
have_flushed_ = false;
got_exception_ = false;
max_http_header_size_ = max_http_header_size;
header_parsing_start_time_ = 0;
headers_timeout_ = headers_timeout;
}


Expand Down Expand Up @@ -831,6 +860,8 @@ class Parser : public AsyncWrap, public StreamListener {
bool pending_pause_ = false;
uint64_t header_nread_ = 0;
uint64_t max_http_header_size_;
uint64_t headers_timeout_;
uint64_t header_parsing_start_time_ = 0;

// These are helper functions for filling `http_parser_settings`, which turn
// a member function of Parser into a C-style HTTP parser callback.
Expand Down Expand Up @@ -890,6 +921,8 @@ void InitializeHttpParser(Local<Object> target,
Integer::NewFromUnsigned(env->isolate(), kOnMessageComplete));
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnExecute"),
Integer::NewFromUnsigned(env->isolate(), kOnExecute));
t->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "kOnTimeout"),
Integer::NewFromUnsigned(env->isolate(), kOnTimeout));

Local<Array> methods = Array::New(env->isolate());
#define V(num, name, string) \
Expand Down
4 changes: 3 additions & 1 deletion test/async-hooks/test-graph.http.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@ process.on('exit', () => {
id: 'httpclientrequest:1',
triggerAsyncId: 'tcpserver:1' },
{ type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' },
{ type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' },
{ type: 'HTTPINCOMINGMESSAGE',
id: 'httpincomingmessage:1',
triggerAsyncId: 'tcp:2' },
{ type: 'Timeout',
id: 'timeout:1',
triggerAsyncId: 'httpincomingmessage:1' },
{ type: 'SHUTDOWNWRAP',
id: 'shutdown:1',
triggerAsyncId: 'tcp:2' } ]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';

const common = require('../common');
const http = require('http');
const net = require('net');
const { finished } = require('stream');

const headers =
'GET / HTTP/1.1\r\n' +
'Host: localhost\r\n' +
'Connection: keep-alive\r\n' +
'Agent: node\r\n';

const baseTimeout = 1000;

const server = http.createServer(common.mustCall((req, res) => {
req.resume();
res.writeHead(200);
res.end();
}, 2));

server.keepAliveTimeout = 10 * baseTimeout;
server.headersTimeout = baseTimeout;

server.once('timeout', common.mustNotCall((socket) => {
socket.destroy();
}));

server.listen(0, () => {
const client = net.connect(server.address().port);

// first request
client.write(headers);
client.write('\r\n');

setTimeout(() => {
// second request
client.write(headers);
// `headersTimeout` doesn't seem to fire if request
// is sent altogether.
setTimeout(() => {
client.write('\r\n');
client.end();
}, 10);
}, baseTimeout + 10);

client.resume();
finished(client, common.mustCall((err) => {
server.close();
}));
});