Skip to content

Commit a947ae4

Browse files
mcollinaaduh95
authored andcommitted
http2: retain streams during pending writes
Native file responses create write requests without a JS reference to the Http2Stream wrapper. Resetting a stream while its socket write is pending could let GC delete the stream before WriteWrap::OnDone(). Keep a strong stream reference with each outgoing DATA buffer until the underlying write completes. Assisted-by: codex:gpt-5.6-sol Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs-private/node-private#1040 Refs: https://hackerone.com/reports/4150891 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com> CVE-ID: CVE-2026-123874
1 parent e766c3d commit a947ae4

3 files changed

Lines changed: 106 additions & 5 deletions

File tree

‎src/node_http2.cc‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2184,15 +2184,19 @@ int Http2Session::OnSendData(
21842184
if (write.buf.len <= length) {
21852185
// This write does not suffice by itself, so we can consume it completely.
21862186
length -= write.buf.len;
2187+
write.stream = stream;
21872188
session->PushOutgoingBuffer(std::move(write));
21882189
stream->queue_.pop();
21892190
continue;
21902191
}
21912192

21922193
// Slice off `length` bytes of the first write in the queue. Retain the
21932194
// originating request until the slice is no longer used by the socket.
2194-
session->PushOutgoingBuffer(NgHttp2StreamWrite{
2195-
nullptr, write.buffer_owner, uv_buf_init(write.buf.base, length)});
2195+
session->PushOutgoingBuffer(
2196+
NgHttp2StreamWrite{nullptr,
2197+
write.buffer_owner,
2198+
stream,
2199+
uv_buf_init(write.buf.base, length)});
21962200
write.buf.base += length;
21972201
write.buf.len -= length;
21982202
break;
@@ -2290,8 +2294,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
22902294

22912295
bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) {
22922296
for (const NgHttp2StreamWrite& wr : outgoing_buffers_) {
2293-
if (wr.req_wrap && WriteWrap::FromObject(wr.req_wrap)->stream() == stream)
2294-
return true;
2297+
if (wr.stream.get() == stream) return true;
22952298
}
22962299
return false;
22972300
}
@@ -3621,6 +3624,7 @@ void NgHttp2StreamWrite::MemoryInfo(MemoryTracker* tracker) const {
36213624
if (req_wrap)
36223625
tracker->TrackField("req_wrap", req_wrap);
36233626
if (buffer_owner) tracker->TrackField("buffer_owner", buffer_owner);
3627+
if (stream) tracker->TrackField("stream", stream);
36243628
tracker->TrackField("buf", buf);
36253629
}
36263630

‎src/node_http2.h‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,11 +163,14 @@ struct Http2RcBufferPointerTraits {
163163
using Http2Headers = NgHeaders<Http2HeadersTraits>;
164164
using Http2RcBufferPointer = NgRcBufPointer<Http2RcBufferPointerTraits>;
165165

166+
class Http2Stream;
167+
166168
struct NgHttp2StreamWrite : public MemoryRetainer {
167169
// Completed once all buffers from the request have been consumed.
168170
BaseObjectPtr<AsyncWrap> req_wrap;
169171
// Retains the request's buffers without completing it.
170172
BaseObjectPtr<AsyncWrap> buffer_owner;
173+
BaseObjectPtr<Http2Stream> stream;
171174
uv_buf_t buf;
172175

173176
inline explicit NgHttp2StreamWrite(uv_buf_t buf_) : buf(buf_) {}
@@ -177,6 +180,14 @@ struct NgHttp2StreamWrite : public MemoryRetainer {
177180
: req_wrap(std::move(req_wrap)),
178181
buffer_owner(std::move(buffer_owner)),
179182
buf(buf_) {}
183+
inline NgHttp2StreamWrite(BaseObjectPtr<AsyncWrap> req_wrap,
184+
BaseObjectPtr<AsyncWrap> buffer_owner,
185+
BaseObjectPtr<Http2Stream> stream,
186+
uv_buf_t buf_)
187+
: req_wrap(std::move(req_wrap)),
188+
buffer_owner(std::move(buffer_owner)),
189+
stream(std::move(stream)),
190+
buf(buf_) {}
180191

181192
void MemoryInfo(MemoryTracker* tracker) const override;
182193
SET_MEMORY_INFO_NAME(NgHttp2StreamWrite)
@@ -189,7 +200,6 @@ typedef uint32_t(*get_setting)(nghttp2_session* session,
189200
class Http2Ping;
190201
class Http2Session;
191202
class Http2Settings;
192-
class Http2Stream;
193203
class Origins;
194204

195205
// This scope should be present when any call into nghttp2 that may schedule
Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
// Flags: --expose-gc
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const assert = require('assert');
9+
const fs = require('fs');
10+
const http2 = require('http2');
11+
const { duplexPair } = require('stream');
12+
const { setImmediate } = require('timers/promises');
13+
const tmpdir = require('../common/tmpdir');
14+
15+
tmpdir.refresh();
16+
const filename = tmpdir.resolve('http2-file');
17+
fs.writeFileSync(filename, Buffer.alloc(1024 * 1024));
18+
19+
async function test(useFD) {
20+
const [clientSide, serverSide] = duplexPair();
21+
let reset = false;
22+
let pendingWrite;
23+
const originalWrite = serverSide._write;
24+
serverSide._write = common.mustCallAtLeast(function(buf, enc, cb) {
25+
originalWrite.call(this, buf, enc, common.mustCall(async () => {
26+
// Deliver the data, but allow the client to reset the stream before
27+
// completing the corresponding native write.
28+
await setImmediate();
29+
if (reset) {
30+
assert.strictEqual(pendingWrite, undefined);
31+
pendingWrite = cb;
32+
} else {
33+
cb();
34+
}
35+
}));
36+
});
37+
38+
const server = http2.createServer();
39+
const fd = useFD ? fs.openSync(filename, 'r') : undefined;
40+
let streamClosed;
41+
const closed = new Promise((resolve) => { streamClosed = resolve; });
42+
server.on('stream', common.mustCall((stream) => {
43+
stream.on('close', common.mustCall(streamClosed));
44+
if (useFD)
45+
stream.respondWithFD(fd);
46+
else
47+
stream.respondWithFile(filename);
48+
}));
49+
server.emit('connection', serverSide);
50+
51+
const client = http2.connect('http://localhost', {
52+
createConnection: () => clientSide,
53+
settings: { initialWindowSize: 1024 * 1024 },
54+
});
55+
client.on('connect', common.mustCall(() => {
56+
client.setLocalWindowSize(1024 * 1024);
57+
}));
58+
const req = client.request();
59+
req.once('data', common.mustCall(() => {
60+
reset = true;
61+
req.close(http2.constants.NGHTTP2_CANCEL);
62+
}));
63+
req.resume();
64+
65+
await closed;
66+
// Let deferred native stream cleanup and unpipe callbacks run, then collect
67+
// the wrapper while its file write is still awaiting completion.
68+
for (let i = 0; i < 10; i++) {
69+
await setImmediate();
70+
global.gc();
71+
}
72+
assert.strictEqual(typeof pendingWrite, 'function');
73+
reset = false;
74+
pendingWrite();
75+
pendingWrite = undefined;
76+
await setImmediate();
77+
78+
client.destroy();
79+
server.close();
80+
if (useFD)
81+
fs.closeSync(fd);
82+
}
83+
84+
(async () => {
85+
await test(false);
86+
await test(true);
87+
})().then(common.mustCall());

0 commit comments

Comments
 (0)