1berry/nodejs
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

http2: fix several serious bugs

Browse Source 
Currently http2 does not properly submit GOAWAY frames when a session
is being destroyed. It also doesn't properly handle when the other
party severs the connection after sending a GOAWAY frame, even though
it should.

Edge, IE & Safari are currently unable to handle empty TRAILERS
frames despite them being correctly to spec. Instead send an empty
DATA frame with END_STREAM flag in those situations.

Fix and adjust several flaky and/or incorrect tests.

PR-URL: https://github.com/nodejs/node/pull/20772
Fixes: https://github.com/nodejs/node/issues/20705
Fixes: https://github.com/nodejs/node/issues/20750
Fixes: https://github.com/nodejs/node/issues/20850
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit is contained in:
Anatoli Papirovski 2018-05-17 23:03:15 +04:00
parent 4f0ab76b6c
commit b11e19e8ee
No known key found for this signature in database
GPG Key ID: 614E2E1ABEB4B2C0
15 changed files with 145 additions and 122 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
doc/api/http2.md
View File

@ -880,6 +880,10 @@ The `'trailers'` event is emitted when a block of headers associated with
trailing header fields is received. The listener callback is passed the
[HTTP/2 Headers Object][] and flags associated with the headers.
Note that this event might not be emitted if `http2stream.end()` is called
before trailers are received and the incoming data is not being read or
listened for.
```js
stream.on('trailers', (headers, flags) => {
console.log(headers);

110
lib/internal/http2/core.js
View File

@ -341,20 +341,25 @@ function onStreamClose(code) {
stream[kState].fd = -1;
// Defer destroy we actually emit end.
if (stream._readableState.endEmitted || code !== NGHTTP2_NO_ERROR) {
if (!stream.readable || code !== NGHTTP2_NO_ERROR) {
// If errored or ended, we can destroy immediately.
stream[kMaybeDestroy](null, code);
stream[kMaybeDestroy](code);
} else {
// Wait for end to destroy.
stream.on('end', stream[kMaybeDestroy]);
// Push a null so the stream can end whenever the client consumes
// it completely.
stream.push(null);
// If the client hasn't tried to consume the stream and there is no
// resume scheduled (which would indicate they would consume in the future),
// then just dump the incoming data so that the stream can be destroyed.
if (!stream[kState].didRead && !stream._readableState.resumeScheduled)
// If the user hasn't tried to consume the stream (and this is a server
// session) then just dump the incoming data so that the stream can
// be destroyed.
if (stream[kSession][kType] === NGHTTP2_SESSION_SERVER &&
!stream[kState].didRead &&
stream.readableFlowing === null)
stream.resume();
else
stream.read(0);
}
}
@ -379,7 +384,7 @@ function onStreamRead(nread, buf) {
`${sessionName(stream[kSession][kType])}]: ending readable.`);
// defer this until we actually emit end
if (stream._readableState.endEmitted) {
if (!stream.readable) {
stream[kMaybeDestroy]();
} else {
stream.on('end', stream[kMaybeDestroy]);
@ -469,8 +474,7 @@ function onGoawayData(code, lastStreamID, buf) {
// goaway using NGHTTP2_NO_ERROR because there was no error
// condition on this side of the session that caused the
// shutdown.
session.destroy(new ERR_HTTP2_SESSION_ERROR(code),
{ errorCode: NGHTTP2_NO_ERROR });
session.destroy(new ERR_HTTP2_SESSION_ERROR(code), NGHTTP2_NO_ERROR);
}
}
@ -813,6 +817,21 @@ function emitClose(self, error) {
self.emit('close');
}
function finishSessionDestroy(session, error) {
const socket = session[kSocket];
if (!socket.destroyed)
socket.destroy(error);
session[kProxySocket] = undefined;
session[kSocket] = undefined;
session[kHandle] = undefined;
socket[kSession] = undefined;
socket[kServer] = undefined;
// Finally, emit the close and error events (if necessary) on next tick.
process.nextTick(emitClose, session, error);
}
// Upon creation, the Http2Session takes ownership of the socket. The session
// may not be ready to use immediately if the socket is not yet fully connected.
// In that case, the Http2Session will wait for the socket to connect. Once
@ -869,6 +888,8 @@ class Http2Session extends EventEmitter {
this[kState] = {
flags: SESSION_FLAGS_PENDING,
goawayCode: null,
goawayLastStreamID: null,
streams: new Map(),
pendingStreams: new Set(),
pendingAck: 0,
@ -1171,25 +1192,13 @@ class Http2Session extends EventEmitter {
if (handle !== undefined)
handle.destroy(code, socket.destroyed);
// If there is no error, use setImmediate to destroy the socket on the
// If the socket is alive, use setImmediate to destroy the session on the
// next iteration of the event loop in order to give data time to transmit.
// Otherwise, destroy immediately.
if (!socket.destroyed) {
if (!error) {
setImmediate(socket.destroy.bind(socket));
} else {
socket.destroy(error);
}
}
this[kProxySocket] = undefined;
this[kSocket] = undefined;
this[kHandle] = undefined;
socket[kSession] = undefined;
socket[kServer] = undefined;
// Finally, emit the close and error events (if necessary) on next tick.
process.nextTick(emitClose, this, error);
if (!socket.destroyed)
setImmediate(finishSessionDestroy, this, error);
else
finishSessionDestroy(this, error);
}
// Closing the session will:
@ -1441,12 +1450,9 @@ function afterDoStreamWrite(status, handle) {
}
function streamOnResume() {
if (!this.destroyed && !this.pending) {
if (!this[kState].didRead)
this[kState].didRead = true;
if (!this.destroyed)
this[kHandle].readStart();
}
}
function streamOnPause() {
if (!this.destroyed && !this.pending)
@ -1460,6 +1466,16 @@ function afterShutdown() {
stream[kMaybeDestroy]();
}
function finishSendTrailers(stream, headersList) {
stream[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
const ret = stream[kHandle].trailers(headersList);
if (ret < 0)
stream.destroy(new NghttpError(ret));
else
stream[kMaybeDestroy]();
}
function closeStream(stream, code, shouldSubmitRstStream = true) {
const state = stream[kState];
state.flags |= STREAM_FLAGS_CLOSED;
@ -1521,6 +1537,10 @@ class Http2Stream extends Duplex {
this[kSession] = session;
session[kState].pendingStreams.add(this);
// Allow our logic for determining whether any reads have happened to
// work in all situations. This is similar to what we do in _http_incoming.
this._readableState.readingMore = true;
this[kTimeout] = null;
this[kState] = {
@ -1531,7 +1551,6 @@ class Http2Stream extends Duplex {
trailersReady: false
};
this.on('resume', streamOnResume);
this.on('pause', streamOnPause);
}
@ -1725,6 +1744,10 @@ class Http2Stream extends Duplex {
this.push(null);
return;
}
if (!this[kState].didRead) {
this._readableState.readingMore = false;
this[kState].didRead = true;
}
if (!this.pending) {
streamOnResume.call(this);
} else {
@ -1773,13 +1796,8 @@ class Http2Stream extends Duplex {
throw headersList;
this[kSentTrailers] = headers;
this[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
const ret = this[kHandle].trailers(headersList);
if (ret < 0)
this.destroy(new NghttpError(ret));
else
this[kMaybeDestroy]();
// Send the trailers in setImmediate so we don't do it on nghttp2 stack.
setImmediate(finishSendTrailers, this, headersList);
}
get closed() {
@ -1866,15 +1884,15 @@ class Http2Stream extends Duplex {
}
// The Http2Stream can be destroyed if it has closed and if the readable
// side has received the final chunk.
[kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) {
if (error || code !== NGHTTP2_NO_ERROR) {
this.destroy(error);
[kMaybeDestroy](code = NGHTTP2_NO_ERROR) {
if (code !== NGHTTP2_NO_ERROR) {
this.destroy();
return;
}
// TODO(mcollina): remove usage of _*State properties
if (this._writableState.ended && this._writableState.pendingcb === 0) {
if (this._readableState.ended && this.closed) {
if (!this.writable) {
if (!this.readable && this.closed) {
this.destroy();
return;
}
@ -1887,7 +1905,7 @@ class Http2Stream extends Duplex {
this[kSession][kType] === NGHTTP2_SESSION_SERVER &&
!(state.flags & STREAM_FLAGS_HAS_TRAILERS) &&
!state.didRead &&
!this._readableState.resumeScheduled) {
this.readableFlowing === null) {
this.close();
}
}
@ -2477,6 +2495,10 @@ Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout);
function socketOnError(error) {
const session = this[kSession];
if (session !== undefined) {
// We can ignore ECONNRESET after GOAWAY was received as there's nothing
// we can do and the other side is fully within its rights to do so.
if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null)
return session.destroy();
debug(`Http2Session ${sessionName(session[kType])}: socket error [` +
`${error.message}]`);
session.destroy(error);

50
src/node_http2.cc
View File

@ -577,9 +577,9 @@ void Http2Session::EmitStatistics() {
void Http2Session::Close(uint32_t code, bool socket_closed) {
DEBUG_HTTP2SESSION(this, "closing session");
if (flags_ & SESSION_STATE_CLOSED)
if (flags_ & SESSION_STATE_CLOSING)
return;
flags_ |= SESSION_STATE_CLOSED;
flags_ |= SESSION_STATE_CLOSING;
// Stop reading on the i/o stream
if (stream_ != nullptr)
@ -587,16 +587,18 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
// If the socket is not closed, then attempt to send a closing GOAWAY
// frame. There is no guarantee that this GOAWAY will be received by
// the peer but the HTTP/2 spec recommends sendinng it anyway. We'll
// the peer but the HTTP/2 spec recommends sending it anyway. We'll
// make a best effort.
if (!socket_closed) {
Http2Scope h2scope(this);
DEBUG_HTTP2SESSION2(this, "terminating session with code %d", code);
CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0);
SendPendingData();
} else if (stream_ != nullptr) {
stream_->RemoveStreamListener(this);
}
flags_ |= SESSION_STATE_CLOSED;
// If there are outstanding pings, those will need to be canceled, do
// so on the next iteration of the event loop to avoid calling out into
// javascript since this may be called during garbage collection.
@ -1355,25 +1357,32 @@ void Http2Session::MaybeScheduleWrite() {
}
}
void Http2Session::MaybeStopReading() {
int want_read = nghttp2_session_want_read(session_);
DEBUG_HTTP2SESSION2(this, "wants read? %d", want_read);
if (want_read == 0)
stream_->ReadStop();
}
// Unset the sending state, finish up all current writes, and reset
// storage for data and metadata that was associated with these writes.
void Http2Session::ClearOutgoing(int status) {
CHECK_NE(flags_ & SESSION_STATE_SENDING, 0);
flags_ &= ~SESSION_STATE_SENDING;
if (outgoing_buffers_.size() > 0) {
outgoing_storage_.clear();
for (const nghttp2_stream_write& wr : outgoing_buffers_) {
std::vector<nghttp2_stream_write> current_outgoing_buffers_;
current_outgoing_buffers_.swap(outgoing_buffers_);
for (const nghttp2_stream_write& wr : current_outgoing_buffers_) {
WriteWrap* wrap = wr.req_wrap;
if (wrap != nullptr)
wrap->Done(status);
}
outgoing_buffers_.clear();
}
flags_ &= ~SESSION_STATE_SENDING;
// Now that we've finished sending queued data, if there are any pending
// RstStreams we should try sending again and then flush them one by one.
if (pending_rst_streams_.size() > 0) {
@ -1484,8 +1493,7 @@ uint8_t Http2Session::SendPendingData() {
ClearOutgoing(res.err);
}
DEBUG_HTTP2SESSION2(this, "wants data in return? %d",
nghttp2_session_want_read(session_));
MaybeStopReading();
return 0;
}
@ -1618,8 +1626,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
};
MakeCallback(env()->error_string(), arraysize(argv), argv);
} else {
DEBUG_HTTP2SESSION2(this, "processed %d bytes. wants more? %d", ret,
nghttp2_session_want_read(session_));
MaybeStopReading();
}
}
@ -1814,6 +1821,7 @@ void Http2Stream::OnTrailers() {
HandleScope scope(isolate);
Local<Context> context = env()->context();
Context::Scope context_scope(context);
flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS;
MakeCallback(env()->ontrailers_string(), 0, nullptr);
}
@ -1822,7 +1830,16 @@ int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) {
CHECK(!this->IsDestroyed());
Http2Scope h2scope(this);
DEBUG_HTTP2STREAM2(this, "sending %d trailers", len);
int ret = nghttp2_submit_trailer(**session_, id_, nva, len);
int ret;
// Sending an empty trailers frame poses problems in Safari, Edge & IE.
// Instead we can just send an empty data frame with NGHTTP2_FLAG_END_STREAM
// to indicate that the stream is ready to be closed.
if (len == 0) {
Http2Stream::Provider::Stream prov(this, 0);
ret = nghttp2_submit_data(**session_, NGHTTP2_FLAG_END_STREAM, id_, *prov);
} else {
ret = nghttp2_submit_trailer(**session_, id_, nva, len);
}
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
return ret;
}
@ -2351,8 +2368,7 @@ void Http2Stream::Info(const FunctionCallbackInfo<Value>& args) {
Headers list(isolate, context, headers);
args.GetReturnValue().Set(stream->SubmitInfo(*list, list.length()));
DEBUG_HTTP2STREAM2(stream, "%d informational headers sent",
headers->Length());
DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", list.length());
}
// Submits trailing headers on the Http2Stream
@ -2367,7 +2383,7 @@ void Http2Stream::Trailers(const FunctionCallbackInfo<Value>& args) {
Headers list(isolate, context, headers);
args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length()));
DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length());
DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", list.length());
}
// Grab the numeric id of the Http2Stream

6
src/node_http2.h
View File

@ -433,7 +433,8 @@ enum session_state_flags {
SESSION_STATE_HAS_SCOPE = 0x1,
SESSION_STATE_WRITE_SCHEDULED = 0x2,
SESSION_STATE_CLOSED = 0x4,
SESSION_STATE_SENDING = 0x8,
SESSION_STATE_CLOSING = 0x8,
SESSION_STATE_SENDING = 0x10,
};
// This allows for 4 default-sized frames with their frame headers
@ -827,6 +828,9 @@ class Http2Session : public AsyncWrap, public StreamListener {
// Schedule a write if nghttp2 indicates it wants to write to the socket.
void MaybeScheduleWrite();
// Stop reading if nghttp2 doesn't want to anymore.
void MaybeStopReading();
// Returns pointer to the stream, or nullptr if stream does not exist
inline Http2Stream* FindStream(int32_t id);

6
test/parallel/parallel.status
View File

@ -8,12 +8,6 @@ prefix parallel
# Postmortem debugging data is prone to accidental removal during V8 updates.
test-postmortem-metadata: PASS,FLAKY
# http2 has a few bugs that make these tests flaky and that are currently worked
# on.
test-http2-client-upload-reject: PASS,FLAKY
test-http2-pipe: PASS,FLAKY
test-http2-client-upload: PASS,FLAKY
[$system==win32]
[$system==linux]

8
test/parallel/test-http2-client-destroy.js
View File

@ -109,9 +109,6 @@ const Countdown = require('../common/countdown');
server.listen(0, common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
// On some platforms (e.g. windows), an ECONNRESET may occur at this
// point -- or it may not. Do not make this a mustCall
client.on('error', () => {});
client.on('close', () => {
server.close();
@ -119,10 +116,7 @@ const Countdown = require('../common/countdown');
client.destroy();
});
const req = client.request();
// On some platforms (e.g. windows), an ECONNRESET may occur at this
// point -- or it may not. Do not make this a mustCall
req.on('error', () => {});
client.request();
}));
}

5
test/parallel/test-http2-no-wanttrailers-listener.js
View File

@ -3,7 +3,6 @@
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const h2 = require('http2');
const server = h2.createServer();
@ -25,9 +24,7 @@ server.on('listening', common.mustCall(function() {
const client = h2.connect(`http://localhost:${this.address().port}`);
const req = client.request();
req.resume();
req.on('trailers', common.mustCall((headers) => {
assert.strictEqual(Object.keys(headers).length, 0);
}));
req.on('trailers', common.mustNotCall());
req.on('close', common.mustCall(() => {
server.close();
client.close();

19
test/parallel/test-http2-server-close-callback.js
View File

@ -4,21 +4,24 @@ const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const Countdown = require('../common/countdown');
const http2 = require('http2');
const server = http2.createServer();
let session;
const countdown = new Countdown(2, () => {
server.close(common.mustCall());
session.destroy();
});
server.listen(0, common.mustCall(() => {
const client = http2.connect(`http://localhost:${server.address().port}`);
client.on('error', (err) => {
if (err.code !== 'ECONNRESET')
throw err;
});
client.on('connect', common.mustCall(() => countdown.dec()));
}));
server.on('session', common.mustCall((s) => {
setImmediate(() => {
server.close(common.mustCall());
s.destroy();
});
session = s;
countdown.dec();
}));

13
test/parallel/test-http2-server-sessionerror.js
View File

@ -35,14 +35,17 @@ server.on('session', common.mustCall((session) => {
server.listen(0, common.mustCall(() => {
const url = `http://localhost:${server.address().port}`;
http2.connect(url)
// An ECONNRESET error may occur depending on the platform (due largely
// to differences in the timing of socket closing). Do not wrap this in
// a common must call.
.on('error', () => {})
.on('error', common.expectsError({
code: 'ERR_HTTP2_SESSION_ERROR',
message: 'Session closed with error code 2',
}))
.on('close', () => {
server.removeAllListeners('error');
http2.connect(url)
.on('error', () => {})
.on('error', common.expectsError({
code: 'ERR_HTTP2_SESSION_ERROR',
message: 'Session closed with error code 2',
}))
.on('close', () => server.close());
});
}));

6
test/parallel/test-http2-server-shutdown-options-errors.js
View File

@ -55,13 +55,7 @@ server.listen(
0,
common.mustCall(() => {
const client = http2.connect(`http://localhost:${server.address().port}`);
// On certain operating systems, an ECONNRESET may occur. We do not need
// to test for it here. Do not make this a mustCall
client.on('error', () => {});
const req = client.request();
// On certain operating systems, an ECONNRESET may occur. We do not need
// to test for it here. Do not make this a mustCall
req.on('error', () => {});
req.resume();
req.on('close', common.mustCall(() => {
client.close();

10
test/parallel/test-http2-server-socket-destroy.js
View File

@ -41,14 +41,20 @@ server.on('listening', common.mustCall(() => {
// The client may have an ECONNRESET error here depending on the operating
// system, due mainly to differences in the timing of socket closing. Do
// not wrap this in a common mustCall.
client.on('error', () => {});
client.on('error', (err) => {
if (err.code !== 'ECONNRESET')
throw err;
});
client.on('close', common.mustCall());
const req = client.request({ ':method': 'POST' });
// The client may have an ECONNRESET error here depending on the operating
// system, due mainly to differences in the timing of socket closing. Do
// not wrap this in a common mustCall.
req.on('error', () => {});
req.on('error', (err) => {
if (err.code !== 'ECONNRESET')
throw err;
});
req.on('aborted', common.mustCall());
req.resume();

8
test/parallel/test-http2-server-stream-session-destroy.js
View File

@ -44,16 +44,8 @@ server.on('stream', common.mustCall((stream) => {
server.listen(0, common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
client.on('error', (err) => {
if (err.code !== 'ECONNRESET')
throw err;
});
const req = client.request();
req.resume();
req.on('end', common.mustCall());
req.on('close', common.mustCall(() => server.close(common.mustCall())));
req.on('error', (err) => {
if (err.code !== 'ECONNRESET')
throw err;
});
}));

10
test/parallel/test-http2-server-timeout.js
View File

@ -6,10 +6,10 @@ if (!common.hasCrypto)
const http2 = require('http2');
const server = http2.createServer();
server.setTimeout(common.platformTimeout(1));
server.setTimeout(common.platformTimeout(50));
const onServerTimeout = common.mustCall((session) => {
session.close(() => session.destroy());
session.close();
});
server.on('stream', common.mustNotCall());
@ -18,14 +18,8 @@ server.once('timeout', onServerTimeout);
server.listen(0, common.mustCall(() => {
const url = `http://localhost:${server.address().port}`;
const client = http2.connect(url);
// Because of the timeout, an ECONNRESET error may or may not happen here.
// Keep this as a non-op and do not use common.mustCall()
client.on('error', () => {});
client.on('close', common.mustCall(() => {
const client2 = http2.connect(url);
// Because of the timeout, an ECONNRESET error may or may not happen here.
// Keep this as a non-op and do not use common.mustCall()
client2.on('error', () => {});
client2.on('close', common.mustCall(() => server.close()));
}));
}));

7
test/parallel/test-http2-too-many-settings.js
View File

@ -29,9 +29,10 @@ function doTest(session) {
server.listen(0, common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
// On some operating systems, an ECONNRESET error may be emitted.
// On others it won't be. Do not make this a mustCall
client.on('error', () => {});
client.on('error', common.expectsError({
code: 'ERR_HTTP2_SESSION_ERROR',
message: 'Session closed with error code 2',
}));
client.on('close', common.mustCall(() => server.close()));
}));
}

3
test/parallel/test-http2-trailers.js
View File

@ -18,6 +18,7 @@ server.on('stream', common.mustCall(onStream));
function onStream(stream, headers, flags) {
stream.on('trailers', common.mustCall((headers) => {
assert.strictEqual(headers[trailerKey], trailerValue);
stream.end(body);
}));
stream.respond({
'content-type': 'text/html',
@ -41,8 +42,6 @@ function onStream(stream, headers, flags) {
type: Error
}
);
stream.end(body);
}
server.listen(0);