Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 0a6672f

Browse filesBrowse files
apapirovskiBethGriggs
apapirovski
authored and
BethGriggs
committed
http2: fix several serious bugs
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. Backport-PR-URL: #22850 PR-URL: #20772 Fixes: #20705 Fixes: #20750 Fixes: #20850 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 11a63dd commit 0a6672f
Copy full SHA for 0a6672f
Expand file treeCollapse file tree

14 files changed

+145
-115
lines changed
Open diff view settings
Collapse file

‎doc/api/http2.md‎

Copy file name to clipboardExpand all lines: doc/api/http2.md
+4Lines changed: 4 additions & 0 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -884,6 +884,10 @@ The `'trailers'` event is emitted when a block of headers associated with
884884
trailing header fields is received. The listener callback is passed the
885885
[HTTP/2 Headers Object][] and flags associated with the headers.
886886

887+
Note that this event might not be emitted if `http2stream.end()` is called
888+
before trailers are received and the incoming data is not being read or
889+
listened for.
890+
887891
```js
888892
stream.on('trailers', (headers, flags) => {
889893
console.log(headers);
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+66-43Lines changed: 66 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -292,20 +292,25 @@ function onStreamClose(code) {
292292
tryClose(stream[kState].fd);
293293

294294
// Defer destroy we actually emit end.
295-
if (stream._readableState.endEmitted || code !== NGHTTP2_NO_ERROR) {
295+
if (!stream.readable || code !== NGHTTP2_NO_ERROR) {
296296
// If errored or ended, we can destroy immediately.
297-
stream[kMaybeDestroy](null, code);
297+
stream[kMaybeDestroy](code);
298298
} else {
299299
// Wait for end to destroy.
300300
stream.on('end', stream[kMaybeDestroy]);
301301
// Push a null so the stream can end whenever the client consumes
302302
// it completely.
303303
stream.push(null);
304-
// If the client hasn't tried to consume the stream and there is no
305-
// resume scheduled (which would indicate they would consume in the future),
306-
// then just dump the incoming data so that the stream can be destroyed.
307-
if (!stream[kState].didRead && !stream._readableState.resumeScheduled)
304+
305+
// If the user hasn't tried to consume the stream (and this is a server
306+
// session) then just dump the incoming data so that the stream can
307+
// be destroyed.
308+
if (stream[kSession][kType] === NGHTTP2_SESSION_SERVER &&
309+
!stream[kState].didRead &&
310+
stream._readableState.flowing === null)
308311
stream.resume();
312+
else
313+
stream.read(0);
309314
}
310315
}
311316

@@ -330,7 +335,7 @@ function onStreamRead(nread, buf) {
330335
`${sessionName(stream[kSession][kType])}]: ending readable.`);
331336

332337
// defer this until we actually emit end
333-
if (stream._readableState.endEmitted) {
338+
if (!stream.readable) {
334339
stream[kMaybeDestroy]();
335340
} else {
336341
stream.on('end', stream[kMaybeDestroy]);
@@ -421,7 +426,7 @@ function onGoawayData(code, lastStreamID, buf) {
421426
// condition on this side of the session that caused the
422427
// shutdown.
423428
session.destroy(new errors.Error('ERR_HTTP2_SESSION_ERROR', code),
424-
{ errorCode: NGHTTP2_NO_ERROR });
429+
NGHTTP2_NO_ERROR);
425430
}
426431
}
427432

@@ -772,6 +777,21 @@ function emitClose(self, error) {
772777
self.emit('close');
773778
}
774779

780+
function finishSessionDestroy(session, error) {
781+
const socket = session[kSocket];
782+
if (!socket.destroyed)
783+
socket.destroy(error);
784+
785+
session[kProxySocket] = undefined;
786+
session[kSocket] = undefined;
787+
session[kHandle] = undefined;
788+
socket[kSession] = undefined;
789+
socket[kServer] = undefined;
790+
791+
// Finally, emit the close and error events (if necessary) on next tick.
792+
process.nextTick(emitClose, session, error);
793+
}
794+
775795
// Upon creation, the Http2Session takes ownership of the socket. The session
776796
// may not be ready to use immediately if the socket is not yet fully connected.
777797
// In that case, the Http2Session will wait for the socket to connect. Once
@@ -828,6 +848,8 @@ class Http2Session extends EventEmitter {
828848

829849
this[kState] = {
830850
flags: SESSION_FLAGS_PENDING,
851+
goawayCode: null,
852+
goawayLastStreamID: null,
831853
streams: new Map(),
832854
pendingStreams: new Set(),
833855
pendingAck: 0,
@@ -1130,25 +1152,13 @@ class Http2Session extends EventEmitter {
11301152
if (handle !== undefined)
11311153
handle.destroy(code, socket.destroyed);
11321154

1133-
// If there is no error, use setImmediate to destroy the socket on the
1155+
// If the socket is alive, use setImmediate to destroy the session on the
11341156
// next iteration of the event loop in order to give data time to transmit.
11351157
// Otherwise, destroy immediately.
1136-
if (!socket.destroyed) {
1137-
if (!error) {
1138-
setImmediate(socket.destroy.bind(socket));
1139-
} else {
1140-
socket.destroy(error);
1141-
}
1142-
}
1143-
1144-
this[kProxySocket] = undefined;
1145-
this[kSocket] = undefined;
1146-
this[kHandle] = undefined;
1147-
socket[kSession] = undefined;
1148-
socket[kServer] = undefined;
1149-
1150-
// Finally, emit the close and error events (if necessary) on next tick.
1151-
process.nextTick(emitClose, this, error);
1158+
if (!socket.destroyed)
1159+
setImmediate(finishSessionDestroy, this, error);
1160+
else
1161+
finishSessionDestroy(this, error);
11521162
}
11531163

11541164
// Closing the session will:
@@ -1422,11 +1432,8 @@ function afterDoStreamWrite(status, handle, req) {
14221432
}
14231433

14241434
function streamOnResume() {
1425-
if (!this.destroyed && !this.pending) {
1426-
if (!this[kState].didRead)
1427-
this[kState].didRead = true;
1435+
if (!this.destroyed)
14281436
this[kHandle].readStart();
1429-
}
14301437
}
14311438

14321439
function streamOnPause() {
@@ -1441,6 +1448,16 @@ function afterShutdown() {
14411448
stream[kMaybeDestroy]();
14421449
}
14431450

1451+
function finishSendTrailers(stream, headersList) {
1452+
stream[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
1453+
1454+
const ret = stream[kHandle].trailers(headersList);
1455+
if (ret < 0)
1456+
stream.destroy(new NghttpError(ret));
1457+
else
1458+
stream[kMaybeDestroy]();
1459+
}
1460+
14441461
function closeStream(stream, code, shouldSubmitRstStream = true) {
14451462
const state = stream[kState];
14461463
state.flags |= STREAM_FLAGS_CLOSED;
@@ -1502,6 +1519,10 @@ class Http2Stream extends Duplex {
15021519
this[kSession] = session;
15031520
session[kState].pendingStreams.add(this);
15041521

1522+
// Allow our logic for determining whether any reads have happened to
1523+
// work in all situations. This is similar to what we do in _http_incoming.
1524+
this._readableState.readingMore = true;
1525+
15051526
this[kState] = {
15061527
didRead: false,
15071528
flags: STREAM_FLAGS_PENDING,
@@ -1510,7 +1531,6 @@ class Http2Stream extends Duplex {
15101531
trailersReady: false
15111532
};
15121533

1513-
this.on('resume', streamOnResume);
15141534
this.on('pause', streamOnPause);
15151535
}
15161536

@@ -1717,6 +1737,10 @@ class Http2Stream extends Duplex {
17171737
this.push(null);
17181738
return;
17191739
}
1740+
if (!this[kState].didRead) {
1741+
this._readableState.readingMore = false;
1742+
this[kState].didRead = true;
1743+
}
17201744
if (!this.pending) {
17211745
streamOnResume.call(this);
17221746
} else {
@@ -1765,13 +1789,8 @@ class Http2Stream extends Duplex {
17651789
throw headersList;
17661790
this[kSentTrailers] = headers;
17671791

1768-
this[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
1769-
1770-
const ret = this[kHandle].trailers(headersList);
1771-
if (ret < 0)
1772-
this.destroy(new NghttpError(ret));
1773-
else
1774-
this[kMaybeDestroy]();
1792+
// Send the trailers in setImmediate so we don't do it on nghttp2 stack.
1793+
setImmediate(finishSendTrailers, this, headersList);
17751794
}
17761795

17771796
get closed() {
@@ -1861,15 +1880,15 @@ class Http2Stream extends Duplex {
18611880
}
18621881
// The Http2Stream can be destroyed if it has closed and if the readable
18631882
// side has received the final chunk.
1864-
[kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) {
1865-
if (error || code !== NGHTTP2_NO_ERROR) {
1866-
this.destroy(error);
1883+
[kMaybeDestroy](code = NGHTTP2_NO_ERROR) {
1884+
if (code !== NGHTTP2_NO_ERROR) {
1885+
this.destroy();
18671886
return;
18681887
}
18691888

18701889
// TODO(mcollina): remove usage of _*State properties
1871-
if (this._writableState.ended && this._writableState.pendingcb === 0) {
1872-
if (this._readableState.ended && this.closed) {
1890+
if (!this.writable) {
1891+
if (!this.readable && this.closed) {
18731892
this.destroy();
18741893
return;
18751894
}
@@ -1882,7 +1901,7 @@ class Http2Stream extends Duplex {
18821901
this[kSession][kType] === NGHTTP2_SESSION_SERVER &&
18831902
!(state.flags & STREAM_FLAGS_HAS_TRAILERS) &&
18841903
!state.didRead &&
1885-
!this._readableState.resumeScheduled) {
1904+
this._readableState.flowing === null) {
18861905
this.close();
18871906
}
18881907
}
@@ -2445,6 +2464,10 @@ Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout);
24452464
function socketOnError(error) {
24462465
const session = this[kSession];
24472466
if (session !== undefined) {
2467+
// We can ignore ECONNRESET after GOAWAY was received as there's nothing
2468+
// we can do and the other side is fully within its rights to do so.
2469+
if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null)
2470+
return session.destroy();
24482471
debug(`Http2Session ${sessionName(session[kType])}: socket error [` +
24492472
`${error.message}]`);
24502473
session.destroy(error);
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+33-17Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -631,26 +631,28 @@ inline void Http2Session::EmitStatistics() {
631631
void Http2Session::Close(uint32_t code, bool socket_closed) {
632632
DEBUG_HTTP2SESSION(this, "closing session");
633633

634-
if (flags_ & SESSION_STATE_CLOSED)
634+
if (flags_ & SESSION_STATE_CLOSING)
635635
return;
636-
flags_ |= SESSION_STATE_CLOSED;
636+
flags_ |= SESSION_STATE_CLOSING;
637637

638638
// Stop reading on the i/o stream
639639
if (stream_ != nullptr)
640640
stream_->ReadStop();
641641

642642
// If the socket is not closed, then attempt to send a closing GOAWAY
643643
// frame. There is no guarantee that this GOAWAY will be received by
644-
// the peer but the HTTP/2 spec recommends sendinng it anyway. We'll
644+
// the peer but the HTTP/2 spec recommends sending it anyway. We'll
645645
// make a best effort.
646646
if (!socket_closed) {
647-
Http2Scope h2scope(this);
648647
DEBUG_HTTP2SESSION2(this, "terminating session with code %d", code);
649648
CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0);
649+
SendPendingData();
650650
} else {
651651
Unconsume();
652652
}
653653

654+
flags_ |= SESSION_STATE_CLOSED;
655+
654656
// If there are outstanding pings, those will need to be canceled, do
655657
// so on the next iteration of the event loop to avoid calling out into
656658
// javascript since this may be called during garbage collection.
@@ -1387,25 +1389,32 @@ void Http2Session::MaybeScheduleWrite() {
13871389
}
13881390
}
13891391

1392+
void Http2Session::MaybeStopReading() {
1393+
int want_read = nghttp2_session_want_read(session_);
1394+
DEBUG_HTTP2SESSION2(this, "wants read? %d", want_read);
1395+
if (want_read == 0)
1396+
stream_->ReadStop();
1397+
}
1398+
13901399
// Unset the sending state, finish up all current writes, and reset
13911400
// storage for data and metadata that was associated with these writes.
13921401
void Http2Session::ClearOutgoing(int status) {
13931402
CHECK_NE(flags_ & SESSION_STATE_SENDING, 0);
13941403

1404+
flags_ &= ~SESSION_STATE_SENDING;
1405+
13951406
if (outgoing_buffers_.size() > 0) {
13961407
outgoing_storage_.clear();
13971408

1398-
for (const nghttp2_stream_write& wr : outgoing_buffers_) {
1409+
std::vector<nghttp2_stream_write> current_outgoing_buffers_;
1410+
current_outgoing_buffers_.swap(outgoing_buffers_);
1411+
for (const nghttp2_stream_write& wr : current_outgoing_buffers_) {
13991412
WriteWrap* wrap = wr.req_wrap;
14001413
if (wrap != nullptr)
14011414
wrap->Done(status);
14021415
}
1403-
1404-
outgoing_buffers_.clear();
14051416
}
14061417

1407-
flags_ &= ~SESSION_STATE_SENDING;
1408-
14091418
// Now that we've finished sending queued data, if there are any pending
14101419
// RstStreams we should try sending again and then flush them one by one.
14111420
if (pending_rst_streams_.size() > 0) {
@@ -1525,8 +1534,7 @@ uint8_t Http2Session::SendPendingData() {
15251534
req->Dispose();
15261535
}
15271536

1528-
DEBUG_HTTP2SESSION2(this, "wants data in return? %d",
1529-
nghttp2_session_want_read(session_));
1537+
MaybeStopReading();
15301538

15311539
return 0;
15321540
}
@@ -1691,8 +1699,7 @@ void Http2Session::OnStreamReadImpl(ssize_t nread,
16911699
};
16921700
session->MakeCallback(env->error_string(), arraysize(argv), argv);
16931701
} else {
1694-
DEBUG_HTTP2SESSION2(session, "processed %d bytes. wants more? %d", ret,
1695-
nghttp2_session_want_read(**session));
1702+
session->MaybeStopReading();
16961703
}
16971704
}
16981705

@@ -1928,6 +1935,7 @@ void Http2Stream::OnTrailers() {
19281935
HandleScope scope(isolate);
19291936
Local<Context> context = env()->context();
19301937
Context::Scope context_scope(context);
1938+
flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS;
19311939
MakeCallback(env()->ontrailers_string(), 0, nullptr);
19321940
}
19331941

@@ -1936,7 +1944,16 @@ int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) {
19361944
CHECK(!this->IsDestroyed());
19371945
Http2Scope h2scope(this);
19381946
DEBUG_HTTP2STREAM2(this, "sending %d trailers", len);
1939-
int ret = nghttp2_submit_trailer(**session_, id_, nva, len);
1947+
int ret;
1948+
// Sending an empty trailers frame poses problems in Safari, Edge & IE.
1949+
// Instead we can just send an empty data frame with NGHTTP2_FLAG_END_STREAM
1950+
// to indicate that the stream is ready to be closed.
1951+
if (len == 0) {
1952+
Http2Stream::Provider::Stream prov(this, 0);
1953+
ret = nghttp2_submit_data(**session_, NGHTTP2_FLAG_END_STREAM, id_, *prov);
1954+
} else {
1955+
ret = nghttp2_submit_trailer(**session_, id_, nva, len);
1956+
}
19401957
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
19411958
return ret;
19421959
}
@@ -2562,8 +2579,7 @@ void Http2Stream::Info(const FunctionCallbackInfo<Value>& args) {
25622579

25632580
Headers list(isolate, context, headers);
25642581
args.GetReturnValue().Set(stream->SubmitInfo(*list, list.length()));
2565-
DEBUG_HTTP2STREAM2(stream, "%d informational headers sent",
2566-
headers->Length());
2582+
DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", list.length());
25672583
}
25682584

25692585
// Submits trailing headers on the Http2Stream
@@ -2578,7 +2594,7 @@ void Http2Stream::Trailers(const FunctionCallbackInfo<Value>& args) {
25782594

25792595
Headers list(isolate, context, headers);
25802596
args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length()));
2581-
DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length());
2597+
DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", list.length());
25822598
}
25832599

25842600
// Grab the numeric id of the Http2Stream

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.