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

http2: fix memory leak when nghttp2 hd threshold is reached #41502

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/node_http2.cc
Expand Up @@ -1009,6 +1009,21 @@ int Http2Session::OnInvalidFrame(nghttp2_session* handle,
return 0;
}

// Remove the headers reference.
// Implicitly calls nghttp2_rcbuf_decref
void Http2Session::DecrefHeaders(const nghttp2_frame* frame) {
int32_t id = GetFrameID(frame);
BaseObjectPtr<Http2Stream> stream = FindStream(id);

if (stream && !stream->is_destroyed() && stream->headers_count() > 0) {
Debug(this, "freeing headers for stream %d", id);
stream->ClearHeaders();
CHECK_EQ(stream->headers_count(), 0);
DecrementCurrentSessionMemory(stream->current_headers_length_);
stream->current_headers_length_ = 0;
}
}

// If nghttp2 is unable to send a queued up frame, it will call this callback
// to let us know. If the failure occurred because we are in the process of
// closing down the session or stream, we go ahead and ignore it. We don't
Expand All @@ -1029,6 +1044,11 @@ int Http2Session::OnFrameNotSent(nghttp2_session* handle,
error_code == NGHTTP2_ERR_STREAM_CLOSED ||
error_code == NGHTTP2_ERR_STREAM_CLOSING ||
session->js_fields_->frame_error_listener_count == 0) {
// Nghttp2 contains header limit of 65536. When this value is exceeded the
// pipeline is stopped and we should remove the current headers reference
// to destroy the session completely.
// Further information see: https://github.com/nodejs/node/issues/35233
session->DecrefHeaders(frame);
return 0;
}

Expand Down
6 changes: 6 additions & 0 deletions src/node_http2.h
Expand Up @@ -401,6 +401,10 @@ class Http2Stream : public AsyncWrap,
size_t i = 0;
for (const auto& header : current_headers_ )
fn(header, i++);
ClearHeaders();
}

void ClearHeaders() {
current_headers_.clear();
}

Expand Down Expand Up @@ -787,6 +791,8 @@ class Http2Session : public AsyncWrap,
void HandleAltSvcFrame(const nghttp2_frame* frame);
void HandleOriginFrame(const nghttp2_frame* frame);

void DecrefHeaders(const nghttp2_frame* frame);

// nghttp2 callbacks
static int OnBeginHeadersCallback(
nghttp2_session* session,
Expand Down
46 changes: 46 additions & 0 deletions test/parallel/test-http2-options-max-headers-exceeds-nghttp2.js
@@ -0,0 +1,46 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const h2 = require('http2');

const server = h2.createServer();

server.on('stream', common.mustNotCall());
server.on('error', common.mustNotCall());

server.listen(0, common.mustCall(() => {

// Setting the maxSendHeaderBlockLength > nghttp2 threshold
// cause a 'sessionError' and no memory leak when session destroy
const options = {
maxSendHeaderBlockLength: 100000
};

const client = h2.connect(`http://localhost:${server.address().port}`,
options);
client.on('error', common.expectsError({
code: 'ERR_HTTP2_SESSION_ERROR',
name: 'Error',
message: 'Session closed with error code 9'
}));

const req = client.request({
// Greater than 65536 bytes
'test-header': 'A'.repeat(90000)
});
req.on('response', common.mustNotCall());

req.on('close', common.mustCall(() => {
client.close();
server.close();
}));

req.on('error', common.expectsError({
code: 'ERR_HTTP2_SESSION_ERROR',
name: 'Error',
message: 'Session closed with error code 9'
}));
req.end();
}));