Skip to content

Commit

Permalink
http2: force through RST_STREAM in destroy
Browse files Browse the repository at this point in the history
If still needed, force through RST_STREAM in Http2Stream#destroy
calls, so that nghttp2 can wrap up properly and doesn't continue
trying to read & write data to the stream.

PR-URL: #21016
Fixes: #21008
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
apapirovski authored and addaleax committed Jun 1, 2018
1 parent 2a9912c commit d4787cf
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 5 deletions.
15 changes: 10 additions & 5 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ function onStreamClose(code) {
`${sessionName(stream[kSession][kType])}]: closed with code ${code}`);

if (!stream.closed)
closeStream(stream, code, false);
closeStream(stream, code, kNoRstStream);

stream[kState].fd = -1;
// Defer destroy we actually emit end.
Expand Down Expand Up @@ -1476,7 +1476,11 @@ function finishSendTrailers(stream, headersList) {
stream[kMaybeDestroy]();
}

function closeStream(stream, code, shouldSubmitRstStream = true) {
const kNoRstStream = 0;
const kSubmitRstStream = 1;
const kForceRstStream = 2;

function closeStream(stream, code, rstStreamStatus = kSubmitRstStream) {
const state = stream[kState];
state.flags |= STREAM_FLAGS_CLOSED;
state.rstCode = code;
Expand All @@ -1499,9 +1503,10 @@ function closeStream(stream, code, shouldSubmitRstStream = true) {
stream.end();
}

if (shouldSubmitRstStream) {
if (rstStreamStatus !== kNoRstStream) {
const finishFn = finishCloseStream.bind(stream, code);
if (!ending || finished || code !== NGHTTP2_NO_ERROR)
if (!ending || finished || code !== NGHTTP2_NO_ERROR ||
rstStreamStatus === kForceRstStream)
finishFn();
else
stream.once('finish', finishFn);
Expand Down Expand Up @@ -1852,7 +1857,7 @@ class Http2Stream extends Duplex {
const hasHandle = handle !== undefined;

if (!this.closed)
closeStream(this, code, hasHandle);
closeStream(this, code, hasHandle ? kForceRstStream : kNoRstStream);
this.push(null);

if (hasHandle) {
Expand Down
2 changes: 2 additions & 0 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1762,6 +1762,8 @@ void Http2Stream::Destroy() {
// Do nothing if this stream instance is already destroyed
if (IsDestroyed())
return;
if (session_->HasPendingRstStream(id_))
FlushRstStream();
flags_ |= NGHTTP2_STREAM_FLAG_DESTROYED;

Debug(this, "destroying stream");
Expand Down
7 changes: 7 additions & 0 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "stream_base-inl.h"
#include "string_bytes.h"

#include <algorithm>
#include <queue>

namespace node {
Expand Down Expand Up @@ -803,6 +804,12 @@ class Http2Session : public AsyncWrap, public StreamListener {
pending_rst_streams_.emplace_back(stream_id);
}

inline bool HasPendingRstStream(int32_t stream_id) {
return pending_rst_streams_.end() != std::find(pending_rst_streams_.begin(),
pending_rst_streams_.end(),
stream_id);
}

// Handle reads/writes from the underlying network transport.
void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override;
void OnStreamAfterWrite(WriteWrap* w, int status) override;
Expand Down
40 changes: 40 additions & 0 deletions test/parallel/test-http2-large-write-destroy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
'use strict';
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const fixtures = require('../common/fixtures');
const http2 = require('http2');

// This test will result in a crash due to a missed CHECK in C++ or
// a straight-up segfault if the C++ doesn't send RST_STREAM through
// properly when calling destroy.

const content = Buffer.alloc(60000, 0x44);

const server = http2.createSecureServer({
key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem')
});
server.on('stream', common.mustCall((stream) => {
stream.respond({
'Content-Type': 'application/octet-stream',
'Content-Length': (content.length.toString() * 2),
'Vary': 'Accept-Encoding'
}, { waitForTrailers: true });

stream.write(content);
stream.destroy();
}));

server.listen(0, common.mustCall(() => {
const client = http2.connect(`https://localhost:${server.address().port}`,
{ rejectUnauthorized: false });

const req = client.request({ ':path': '/' });
req.end();

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

0 comments on commit d4787cf

Please sign in to comment.