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 171600d

Browse filesBrowse files
apapirovskiMylesBorins
apapirovski
authored and
MylesBorins
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. 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 ce40c70 commit 171600d
Copy full SHA for 171600d
Expand file treeCollapse file tree

15 files changed

+145
-122
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
@@ -880,6 +880,10 @@ The `'trailers'` event is emitted when a block of headers associated with
880880
trailing header fields is received. The listener callback is passed the
881881
[HTTP/2 Headers Object][] and flags associated with the headers.
882882

883+
Note that this event might not be emitted if `http2stream.end()` is called
884+
before trailers are received and the incoming data is not being read or
885+
listened for.
886+
883887
```js
884888
stream.on('trailers', (headers, flags) => {
885889
console.log(headers);
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+66-44Lines changed: 66 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -341,20 +341,25 @@ function onStreamClose(code) {
341341

342342
stream[kState].fd = -1;
343343
// Defer destroy we actually emit end.
344-
if (stream._readableState.endEmitted || code !== NGHTTP2_NO_ERROR) {
344+
if (!stream.readable || code !== NGHTTP2_NO_ERROR) {
345345
// If errored or ended, we can destroy immediately.
346-
stream[kMaybeDestroy](null, code);
346+
stream[kMaybeDestroy](code);
347347
} else {
348348
// Wait for end to destroy.
349349
stream.on('end', stream[kMaybeDestroy]);
350350
// Push a null so the stream can end whenever the client consumes
351351
// it completely.
352352
stream.push(null);
353-
// If the client hasn't tried to consume the stream and there is no
354-
// resume scheduled (which would indicate they would consume in the future),
355-
// then just dump the incoming data so that the stream can be destroyed.
356-
if (!stream[kState].didRead && !stream._readableState.resumeScheduled)
353+
354+
// If the user hasn't tried to consume the stream (and this is a server
355+
// session) then just dump the incoming data so that the stream can
356+
// be destroyed.
357+
if (stream[kSession][kType] === NGHTTP2_SESSION_SERVER &&
358+
!stream[kState].didRead &&
359+
stream.readableFlowing === null)
357360
stream.resume();
361+
else
362+
stream.read(0);
358363
}
359364
}
360365

@@ -379,7 +384,7 @@ function onStreamRead(nread, buf) {
379384
`${sessionName(stream[kSession][kType])}]: ending readable.`);
380385

381386
// defer this until we actually emit end
382-
if (stream._readableState.endEmitted) {
387+
if (!stream.readable) {
383388
stream[kMaybeDestroy]();
384389
} else {
385390
stream.on('end', stream[kMaybeDestroy]);
@@ -469,8 +474,7 @@ function onGoawayData(code, lastStreamID, buf) {
469474
// goaway using NGHTTP2_NO_ERROR because there was no error
470475
// condition on this side of the session that caused the
471476
// shutdown.
472-
session.destroy(new ERR_HTTP2_SESSION_ERROR(code),
473-
{ errorCode: NGHTTP2_NO_ERROR });
477+
session.destroy(new ERR_HTTP2_SESSION_ERROR(code), NGHTTP2_NO_ERROR);
474478
}
475479
}
476480

@@ -813,6 +817,21 @@ function emitClose(self, error) {
813817
self.emit('close');
814818
}
815819

820+
function finishSessionDestroy(session, error) {
821+
const socket = session[kSocket];
822+
if (!socket.destroyed)
823+
socket.destroy(error);
824+
825+
session[kProxySocket] = undefined;
826+
session[kSocket] = undefined;
827+
session[kHandle] = undefined;
828+
socket[kSession] = undefined;
829+
socket[kServer] = undefined;
830+
831+
// Finally, emit the close and error events (if necessary) on next tick.
832+
process.nextTick(emitClose, session, error);
833+
}
834+
816835
// Upon creation, the Http2Session takes ownership of the socket. The session
817836
// may not be ready to use immediately if the socket is not yet fully connected.
818837
// In that case, the Http2Session will wait for the socket to connect. Once
@@ -869,6 +888,8 @@ class Http2Session extends EventEmitter {
869888

870889
this[kState] = {
871890
flags: SESSION_FLAGS_PENDING,
891+
goawayCode: null,
892+
goawayLastStreamID: null,
872893
streams: new Map(),
873894
pendingStreams: new Set(),
874895
pendingAck: 0,
@@ -1171,25 +1192,13 @@ class Http2Session extends EventEmitter {
11711192
if (handle !== undefined)
11721193
handle.destroy(code, socket.destroyed);
11731194

1174-
// If there is no error, use setImmediate to destroy the socket on the
1195+
// If the socket is alive, use setImmediate to destroy the session on the
11751196
// next iteration of the event loop in order to give data time to transmit.
11761197
// Otherwise, destroy immediately.
1177-
if (!socket.destroyed) {
1178-
if (!error) {
1179-
setImmediate(socket.destroy.bind(socket));
1180-
} else {
1181-
socket.destroy(error);
1182-
}
1183-
}
1184-
1185-
this[kProxySocket] = undefined;
1186-
this[kSocket] = undefined;
1187-
this[kHandle] = undefined;
1188-
socket[kSession] = undefined;
1189-
socket[kServer] = undefined;
1190-
1191-
// Finally, emit the close and error events (if necessary) on next tick.
1192-
process.nextTick(emitClose, this, error);
1198+
if (!socket.destroyed)
1199+
setImmediate(finishSessionDestroy, this, error);
1200+
else
1201+
finishSessionDestroy(this, error);
11931202
}
11941203

11951204
// Closing the session will:
@@ -1441,11 +1450,8 @@ function afterDoStreamWrite(status, handle) {
14411450
}
14421451

14431452
function streamOnResume() {
1444-
if (!this.destroyed && !this.pending) {
1445-
if (!this[kState].didRead)
1446-
this[kState].didRead = true;
1453+
if (!this.destroyed)
14471454
this[kHandle].readStart();
1448-
}
14491455
}
14501456

14511457
function streamOnPause() {
@@ -1460,6 +1466,16 @@ function afterShutdown() {
14601466
stream[kMaybeDestroy]();
14611467
}
14621468

1469+
function finishSendTrailers(stream, headersList) {
1470+
stream[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
1471+
1472+
const ret = stream[kHandle].trailers(headersList);
1473+
if (ret < 0)
1474+
stream.destroy(new NghttpError(ret));
1475+
else
1476+
stream[kMaybeDestroy]();
1477+
}
1478+
14631479
function closeStream(stream, code, shouldSubmitRstStream = true) {
14641480
const state = stream[kState];
14651481
state.flags |= STREAM_FLAGS_CLOSED;
@@ -1521,6 +1537,10 @@ class Http2Stream extends Duplex {
15211537
this[kSession] = session;
15221538
session[kState].pendingStreams.add(this);
15231539

1540+
// Allow our logic for determining whether any reads have happened to
1541+
// work in all situations. This is similar to what we do in _http_incoming.
1542+
this._readableState.readingMore = true;
1543+
15241544
this[kTimeout] = null;
15251545

15261546
this[kState] = {
@@ -1531,7 +1551,6 @@ class Http2Stream extends Duplex {
15311551
trailersReady: false
15321552
};
15331553

1534-
this.on('resume', streamOnResume);
15351554
this.on('pause', streamOnPause);
15361555
}
15371556

@@ -1725,6 +1744,10 @@ class Http2Stream extends Duplex {
17251744
this.push(null);
17261745
return;
17271746
}
1747+
if (!this[kState].didRead) {
1748+
this._readableState.readingMore = false;
1749+
this[kState].didRead = true;
1750+
}
17281751
if (!this.pending) {
17291752
streamOnResume.call(this);
17301753
} else {
@@ -1773,13 +1796,8 @@ class Http2Stream extends Duplex {
17731796
throw headersList;
17741797
this[kSentTrailers] = headers;
17751798

1776-
this[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
1777-
1778-
const ret = this[kHandle].trailers(headersList);
1779-
if (ret < 0)
1780-
this.destroy(new NghttpError(ret));
1781-
else
1782-
this[kMaybeDestroy]();
1799+
// Send the trailers in setImmediate so we don't do it on nghttp2 stack.
1800+
setImmediate(finishSendTrailers, this, headersList);
17831801
}
17841802

17851803
get closed() {
@@ -1866,15 +1884,15 @@ class Http2Stream extends Duplex {
18661884
}
18671885
// The Http2Stream can be destroyed if it has closed and if the readable
18681886
// side has received the final chunk.
1869-
[kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) {
1870-
if (error || code !== NGHTTP2_NO_ERROR) {
1871-
this.destroy(error);
1887+
[kMaybeDestroy](code = NGHTTP2_NO_ERROR) {
1888+
if (code !== NGHTTP2_NO_ERROR) {
1889+
this.destroy();
18721890
return;
18731891
}
18741892

18751893
// TODO(mcollina): remove usage of _*State properties
1876-
if (this._writableState.ended && this._writableState.pendingcb === 0) {
1877-
if (this._readableState.ended && this.closed) {
1894+
if (!this.writable) {
1895+
if (!this.readable && this.closed) {
18781896
this.destroy();
18791897
return;
18801898
}
@@ -1887,7 +1905,7 @@ class Http2Stream extends Duplex {
18871905
this[kSession][kType] === NGHTTP2_SESSION_SERVER &&
18881906
!(state.flags & STREAM_FLAGS_HAS_TRAILERS) &&
18891907
!state.didRead &&
1890-
!this._readableState.resumeScheduled) {
1908+
this.readableFlowing === null) {
18911909
this.close();
18921910
}
18931911
}
@@ -2477,6 +2495,10 @@ Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout);
24772495
function socketOnError(error) {
24782496
const session = this[kSession];
24792497
if (session !== undefined) {
2498+
// We can ignore ECONNRESET after GOAWAY was received as there's nothing
2499+
// we can do and the other side is fully within its rights to do so.
2500+
if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null)
2501+
return session.destroy();
24802502
debug(`Http2Session ${sessionName(session[kType])}: socket error [` +
24812503
`${error.message}]`);
24822504
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
@@ -577,26 +577,28 @@ void Http2Session::EmitStatistics() {
577577
void Http2Session::Close(uint32_t code, bool socket_closed) {
578578
DEBUG_HTTP2SESSION(this, "closing session");
579579

580-
if (flags_ & SESSION_STATE_CLOSED)
580+
if (flags_ & SESSION_STATE_CLOSING)
581581
return;
582-
flags_ |= SESSION_STATE_CLOSED;
582+
flags_ |= SESSION_STATE_CLOSING;
583583

584584
// Stop reading on the i/o stream
585585
if (stream_ != nullptr)
586586
stream_->ReadStop();
587587

588588
// If the socket is not closed, then attempt to send a closing GOAWAY
589589
// frame. There is no guarantee that this GOAWAY will be received by
590-
// the peer but the HTTP/2 spec recommends sendinng it anyway. We'll
590+
// the peer but the HTTP/2 spec recommends sending it anyway. We'll
591591
// make a best effort.
592592
if (!socket_closed) {
593-
Http2Scope h2scope(this);
594593
DEBUG_HTTP2SESSION2(this, "terminating session with code %d", code);
595594
CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0);
595+
SendPendingData();
596596
} else if (stream_ != nullptr) {
597597
stream_->RemoveStreamListener(this);
598598
}
599599

600+
flags_ |= SESSION_STATE_CLOSED;
601+
600602
// If there are outstanding pings, those will need to be canceled, do
601603
// so on the next iteration of the event loop to avoid calling out into
602604
// javascript since this may be called during garbage collection.
@@ -1355,25 +1357,32 @@ void Http2Session::MaybeScheduleWrite() {
13551357
}
13561358
}
13571359

1360+
void Http2Session::MaybeStopReading() {
1361+
int want_read = nghttp2_session_want_read(session_);
1362+
DEBUG_HTTP2SESSION2(this, "wants read? %d", want_read);
1363+
if (want_read == 0)
1364+
stream_->ReadStop();
1365+
}
1366+
13581367
// Unset the sending state, finish up all current writes, and reset
13591368
// storage for data and metadata that was associated with these writes.
13601369
void Http2Session::ClearOutgoing(int status) {
13611370
CHECK_NE(flags_ & SESSION_STATE_SENDING, 0);
13621371

1372+
flags_ &= ~SESSION_STATE_SENDING;
1373+
13631374
if (outgoing_buffers_.size() > 0) {
13641375
outgoing_storage_.clear();
13651376

1366-
for (const nghttp2_stream_write& wr : outgoing_buffers_) {
1377+
std::vector<nghttp2_stream_write> current_outgoing_buffers_;
1378+
current_outgoing_buffers_.swap(outgoing_buffers_);
1379+
for (const nghttp2_stream_write& wr : current_outgoing_buffers_) {
13671380
WriteWrap* wrap = wr.req_wrap;
13681381
if (wrap != nullptr)
13691382
wrap->Done(status);
13701383
}
1371-
1372-
outgoing_buffers_.clear();
13731384
}
13741385

1375-
flags_ &= ~SESSION_STATE_SENDING;
1376-
13771386
// Now that we've finished sending queued data, if there are any pending
13781387
// RstStreams we should try sending again and then flush them one by one.
13791388
if (pending_rst_streams_.size() > 0) {
@@ -1484,8 +1493,7 @@ uint8_t Http2Session::SendPendingData() {
14841493
ClearOutgoing(res.err);
14851494
}
14861495

1487-
DEBUG_HTTP2SESSION2(this, "wants data in return? %d",
1488-
nghttp2_session_want_read(session_));
1496+
MaybeStopReading();
14891497

14901498
return 0;
14911499
}
@@ -1618,8 +1626,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
16181626
};
16191627
MakeCallback(env()->error_string(), arraysize(argv), argv);
16201628
} else {
1621-
DEBUG_HTTP2SESSION2(this, "processed %d bytes. wants more? %d", ret,
1622-
nghttp2_session_want_read(session_));
1629+
MaybeStopReading();
16231630
}
16241631
}
16251632

@@ -1814,6 +1821,7 @@ void Http2Stream::OnTrailers() {
18141821
HandleScope scope(isolate);
18151822
Local<Context> context = env()->context();
18161823
Context::Scope context_scope(context);
1824+
flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS;
18171825
MakeCallback(env()->ontrailers_string(), 0, nullptr);
18181826
}
18191827

@@ -1822,7 +1830,16 @@ int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) {
18221830
CHECK(!this->IsDestroyed());
18231831
Http2Scope h2scope(this);
18241832
DEBUG_HTTP2STREAM2(this, "sending %d trailers", len);
1825-
int ret = nghttp2_submit_trailer(**session_, id_, nva, len);
1833+
int ret;
1834+
// Sending an empty trailers frame poses problems in Safari, Edge & IE.
1835+
// Instead we can just send an empty data frame with NGHTTP2_FLAG_END_STREAM
1836+
// to indicate that the stream is ready to be closed.
1837+
if (len == 0) {
1838+
Http2Stream::Provider::Stream prov(this, 0);
1839+
ret = nghttp2_submit_data(**session_, NGHTTP2_FLAG_END_STREAM, id_, *prov);
1840+
} else {
1841+
ret = nghttp2_submit_trailer(**session_, id_, nva, len);
1842+
}
18261843
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
18271844
return ret;
18281845
}
@@ -2351,8 +2368,7 @@ void Http2Stream::Info(const FunctionCallbackInfo<Value>& args) {
23512368

23522369
Headers list(isolate, context, headers);
23532370
args.GetReturnValue().Set(stream->SubmitInfo(*list, list.length()));
2354-
DEBUG_HTTP2STREAM2(stream, "%d informational headers sent",
2355-
headers->Length());
2371+
DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", list.length());
23562372
}
23572373

23582374
// Submits trailing headers on the Http2Stream
@@ -2367,7 +2383,7 @@ void Http2Stream::Trailers(const FunctionCallbackInfo<Value>& args) {
23672383

23682384
Headers list(isolate, context, headers);
23692385
args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length()));
2370-
DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length());
2386+
DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", list.length());
23712387
}
23722388

23732389
// 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.