Skip to content

Commit 0e32ee2

Browse files
sankalpsthakurmcollina
authored andcommitted
http2: fix onread assert when destroying session from stream handler
When session.destroy() runs from a 'stream' handler, MakeCallback drains nextTick while nghttp2 is still inside mem_recv. Close is deferred for that window (see #64166), so later HEADERS in the same buffer created C++ streams without a JS wrapper or onread, and DATA delivery aborted with Assertion failed: onread->IsFunction(). - Reject new streams while the session is closing - Destroy the C++ handle if on_headers runs after JS destroy - Drop DATA when onread is not installed (defensive) Fixes: #64850 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65116 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent e5778f7 commit 0e32ee2

3 files changed

Lines changed: 78 additions & 1 deletion

File tree

lib/internal/http2/core.js

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -366,8 +366,15 @@ function emit(self, ...args) {
366366
// the block of headers on.
367367
function onSessionHeaders(handle, id, cat, flags, headers, sensitiveHeaders) {
368368
const session = this[kOwner];
369-
if (session.destroyed)
369+
// Session may have been destroyed mid-receive (e.g. session.destroy() from a
370+
// 'stream' handler drained via nextTick inside MakeCallback while nghttp2 is
371+
// still walking the receive buffer). Tear down the C++ stream so subsequent
372+
// DATA frames do not call CallJSOnreadMethod with a missing onread.
373+
if (session.destroyed) {
374+
handle.rstStream(NGHTTP2_REFUSED_STREAM);
375+
handle.destroy();
370376
return;
377+
}
371378

372379
const type = session[kType];
373380
session[kUpdateTimer]();

src/node_http2.cc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1063,6 +1063,16 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle,
10631063
// The common case is that we're creating a new stream. The less likely
10641064
// case is that we're receiving a set of trailers
10651065
if (!stream) [[likely]] {
1066+
// Close() may be deferred while mem_recv is in progress (see
1067+
// Http2Session::Close). A 'stream' handler that calls session.destroy()
1068+
// runs via nextTick from MakeCallback during that window, so later
1069+
// HEADERS in the same receive buffer must not create a C++ stream
1070+
// whose JS wrapper (and onread) is never installed.
1071+
if (session->is_closing()) {
1072+
nghttp2_submit_rst_stream(
1073+
session->session(), NGHTTP2_FLAG_NONE, id, NGHTTP2_REFUSED_STREAM);
1074+
return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
1075+
}
10661076
if (!session->CanAddStream() ||
10671077
Http2Stream::New(session, id, frame->headers.cat) == nullptr)
10681078
[[unlikely]] {
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
const http2 = require('http2');
8+
9+
// Regression test for https://github.com/nodejs/node/issues/64850
10+
//
11+
// Destroying the session from a 'stream' handler runs (via nextTick drained
12+
// from MakeCallback) while nghttp2 is still inside mem_recv. Close is deferred
13+
// for that window; later HEADERS/DATA in the same buffer must not abort with
14+
// Assertion failed: onread->IsFunction().
15+
16+
const STREAMS = 8;
17+
const BODY = Buffer.alloc(2048, 'a');
18+
const ROUNDS = 40;
19+
20+
const server = http2.createServer({
21+
settings: { maxConcurrentStreams: 4 },
22+
});
23+
24+
server.on('session', (session) => session.on('error', () => {}));
25+
26+
server.on('stream', (stream) => {
27+
stream.on('error', () => {});
28+
stream.session.destroy();
29+
});
30+
31+
server.listen(0, '127.0.0.1', common.mustCall(() => {
32+
const port = server.address().port;
33+
const origin = `http://127.0.0.1:${port}`;
34+
let remaining = ROUNDS;
35+
36+
const round = () => {
37+
if (remaining-- <= 0) {
38+
server.close();
39+
return;
40+
}
41+
42+
const session = http2.connect(origin);
43+
session.on('error', () => {});
44+
session.on('close', () => setImmediate(round));
45+
46+
session.on('connect', () => {
47+
for (let i = 0; i < STREAMS; i++) {
48+
const stream = session.request({
49+
':path': `/${i}`,
50+
':method': 'POST',
51+
});
52+
stream.on('error', () => {});
53+
stream.resume();
54+
stream.end(BODY);
55+
}
56+
});
57+
};
58+
59+
round();
60+
}));

0 commit comments

Comments
 (0)