Skip to content

Commit

Permalink
http2: fix memory leak when headers are not emitted
Browse files Browse the repository at this point in the history
When headers are not emitted to JS, e.g. because of an error
before that could happen, we currently still have the vector of
previously received headers lying around, each one holding
a reference count of 1.

To fix the resulting memory leak, release them in the `Http2Stream`
destructor.

Also, clear the vector of headers once they have been emitted –
there’s not need to keep it around, wasting memory.

Backport-PR-URL: #22850
PR-URL: #21373
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
  • Loading branch information
addaleax authored and BethGriggs committed Oct 16, 2018
1 parent 0f3e650 commit e9e4f43
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 11 deletions.
13 changes: 8 additions & 5 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1137,8 +1137,7 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
if (stream->IsDestroyed())
return;

nghttp2_header* headers = stream->headers();
size_t count = stream->headers_count();
std::vector<nghttp2_header> headers(stream->move_headers());

Local<String> name_str;
Local<String> value_str;
Expand All @@ -1155,9 +1154,9 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
// this way for performance reasons (it's faster to generate and pass an
// array than it is to generate and pass the object).
size_t n = 0;
while (count > 0) {
while (n < headers.size()) {
size_t j = 0;
while (count > 0 && j < arraysize(argv) / 2) {
while (n < headers.size() && j < arraysize(argv) / 2) {
nghttp2_header item = headers[n++];
// The header name and value are passed as external one-byte strings
name_str =
Expand All @@ -1166,7 +1165,6 @@ inline void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) {
ExternalHeader::New<false>(env(), item.value).ToLocalChecked();
argv[j * 2] = name_str;
argv[j * 2 + 1] = value_str;
count--;
j++;
}
// For performance, we pass name and value pairs to array.protototype.push
Expand Down Expand Up @@ -1782,6 +1780,11 @@ Http2Stream::Http2Stream(


Http2Stream::~Http2Stream() {
for (nghttp2_header& header : current_headers_) {
nghttp2_rcbuf_decref(header.name);
nghttp2_rcbuf_decref(header.value);
}

if (session_ == nullptr)
return;
DEBUG_HTTP2STREAM(this, "tearing down stream");
Expand Down
8 changes: 2 additions & 6 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -653,18 +653,14 @@ class Http2Stream : public AsyncWrap,
nghttp2_rcbuf* value,
uint8_t flags);

inline nghttp2_header* headers() {
return current_headers_.data();
inline std::vector<nghttp2_header> move_headers() {
return std::move(current_headers_);
}

inline nghttp2_headers_category headers_category() const {
return current_headers_category_;
}

inline size_t headers_count() const {
return current_headers_.size();
}

void StartHeaders(nghttp2_headers_category category);

// Required for StreamBase
Expand Down

0 comments on commit e9e4f43

Please sign in to comment.