From 47509324f4b7b87245e4059a06262daa3c89dfc9 Mon Sep 17 00:00:00 2001 From: esgor <100363036+Eusgor@users.noreply.github.com> Date: Fri, 17 Jul 2026 18:39:42 +0600 Subject: [PATCH 1/2] http2: avoid uaf while receiving and sending rst_stream Mark the session as receiving around nghttp2_session_mem_recv() and defer RST_STREAM handling while receive is in progress. This prevents closing a stream while nghttp2 still processes it and avoids heap-use-after-free in nghttp2_session_mem_recv2(). Fixes: https://github.com/nodejs/node/issues/64113 Signed-off-by: Evgeniy Gorbanev PR-URL: https://github.com/nodejs/node/pull/64166 Reviewed-By: Matteo Collina Reviewed-By: Tim Perry Reviewed-By: Rafael Gonzaga (cherry picked from commit 46de80de88c6ab0fc1fa27aa4d51804c7e989da4) --- src/node_http2.cc | 108 +++++++++++++++++++++++++++++++++++++++++++--- src/node_http2.h | 18 ++++++++ 2 files changed, 120 insertions(+), 6 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 5dfc425230e6..08aa544b3df5 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -815,6 +815,21 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { return; set_closing(); + // Do not flush GOAWAY from inside nghttp2_session_mem_recv() callbacks. + // ConsumeHTTP2Data() finishes the close once mem_recv returns. + if (is_receiving()) { + set_close_pending(); + pending_close_code_ = code; + pending_close_socket_closed_ = socket_closed; + return; + } + + FinishClose(code, socket_closed); +} + +void Http2Session::FinishClose(uint32_t code, bool socket_closed) { + CHECK(is_closing()); + // Stop reading on the i/o stream if (stream_ != nullptr) { set_reading_stopped(); @@ -864,6 +879,12 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { EmitStatistics(); } +void Http2Session::MaybeFinishPendingClose() { + if (!is_close_pending() || is_destroyed()) return; + set_close_pending(false); + FinishClose(pending_close_code_, pending_close_socket_closed_); +} + // Locates an existing known stream by ID. nghttp2 has a similar method // but this is faster and does not fail if the stream is not found. BaseObjectPtr Http2Session::FindStream(int32_t id) { @@ -958,11 +979,13 @@ void Http2Session::ConsumeHTTP2Data() { nghttp2_session_want_read(session_.get())); set_receive_paused(false); custom_recv_error_code_ = nullptr; + set_receiving(); ssize_t ret = nghttp2_session_mem_recv(session_.get(), reinterpret_cast(stream_buf_.base) + stream_buf_offset_, read_len); + set_receiving(false); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); CHECK_IMPLIES(custom_recv_error_code_ != nullptr, ret < 0); @@ -976,6 +999,10 @@ void Http2Session::ConsumeHTTP2Data() { // Even if all bytes were received, a paused stream may delay the // nghttp2_on_frame_recv_callback which may have an END_STREAM flag. stream_buf_offset_ += ret; + // Still complete a Close() deferred during mem_recv; do not fall through + // to SendPendingData() here (paused receives historically skip that flush + // because a write may already be in progress). + MaybeFinishPendingClose(); goto done; } @@ -986,12 +1013,23 @@ void Http2Session::ConsumeHTTP2Data() { stream_buf_allocation_.reset(); stream_buf_ = uv_buf_init(nullptr, 0); + // Finish a Close() deferred during mem_recv before flushing, so GOAWAY is + // not written after pending RST_STREAM frames. + MaybeFinishPendingClose(); + +done: + // Finish a Close() deferred above before flushing, so GOAWAY is not written + // after pending RST_STREAM frames. + if (is_close_pending() && !is_destroyed()) { + set_close_pending(false); + FinishClose(pending_close_code_, pending_close_socket_closed_); + } + // Send any data that was queued up while processing the received data. if (ret >= 0 && !is_destroyed()) { SendPendingData(); } -done: if (ret < 0) [[unlikely]] { Isolate* isolate = env()->isolate(); Debug(this, @@ -1405,6 +1443,9 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, len -= avail; stream->EmitRead(avail, buf); + // JS may have destroyed the stream from inside onread; stop delivering. + if (stream->is_destroyed()) break; + // If the stream owner (e.g. the JS Http2Stream) wants more data, just // tell nghttp2 that all data has been consumed. Otherwise, defer until // more data is being requested. @@ -1962,6 +2003,12 @@ uint8_t Http2Session::SendPendingData() { // SendPendingData should not be called recursively. if (is_sending()) return 1; + + // Do not call `nghttp2_session_mem_send()` while nghttp2 is processing + // incoming data. Sending may close the stream and free nghttp2 state + // that is still in use by `nghttp2_session_mem_recv()`. + if (is_receiving()) return 1; + // This is cleared by ClearOutgoing(). set_sending(); @@ -2372,10 +2419,48 @@ void Http2Stream::Destroy() { // Do nothing if this stream instance is already destroyed if (is_destroyed()) return; - if (session_->has_pending_rststream(id_)) - FlushRstStream(); + + // Session may already be gone if destroy was deferred across a session + // teardown. + if (!session_) { + set_destroyed(); + Detach(); + return; + } + + // Mark destroyed immediately so OnDataChunkReceived stops EmitRead into an + // already-destroyed JS stream (which would treat the byte count as errno). set_destroyed(); + // While mem_recv is active, do not FlushRstStream or RemoveStream yet: + // - FlushRstStream would close the nghttp2 stream before queued response + // DATA can be mem_send'd after receive returns. + // - RemoveStream would make OnSendData/Provider::OnRead fail to FindStream. + // Pending RSTs stay in pending_rst_streams_ and are flushed from + // ClearOutgoing after the post-receive SendPendingData. + if (session_->is_receiving()) { + BaseObjectPtr strong_ref{this}; + env()->SetImmediate( + [this, strong_ref](Environment*) { CompleteDestroyCleanup(); }); + return; + } + + if (session_->has_pending_rststream(id_)) FlushRstStream(); + + CompleteDestroyCleanup(); +} + +void Http2Stream::CompleteDestroyCleanup() { + if (!session_) { + Detach(); + return; + } + + // Destroy() always set_destroyed() before scheduling or calling this. + CHECK(is_destroyed()); + + if (session_->has_pending_rststream(id_)) FlushRstStream(); + Debug(this, "destroying stream"); // Wait until the start of the next loop to delete because there @@ -2412,7 +2497,6 @@ void Http2Stream::Destroy() { EmitStatistics(); } - // Initiates a response on the Http2Stream using data provided via the // StreamBase Streams API. int Http2Stream::SubmitResponse(const Http2Headers& headers, int options) { @@ -2521,6 +2605,18 @@ void Http2Stream::SubmitRstStream(const uint32_t code) { return code == NGHTTP2_CANCEL; }; + // Do not call `nghttp2_session_mem_send()` while nghttp2 is processing + // incoming data. Sending may close the stream and free nghttp2 state + // that is still in use by `nghttp2_session_mem_recv()`. + if (session_->is_receiving() && available_outbound_length_ == 0) { + if (is_stream_cancel(code)) { + session_->AddPendingRstStream(id_); + return; + } + FlushRstStream(); + return; + } + // If RST_STREAM frame is received with error code NGHTTP2_CANCEL, // add it to the pending list and don't force purge the data. It is // to avoids the double free error due to unwanted behavior of nghttp2. @@ -2556,8 +2652,8 @@ void Http2Stream::SubmitRstStream(const uint32_t code) { } void Http2Stream::FlushRstStream() { - if (is_destroyed()) - return; + if (!session_) return; + session_->RemovePendingRstStream(id_); Http2Scope h2scope(this); CHECK_EQ(nghttp2_submit_rst_stream( session_->session(), diff --git a/src/node_http2.h b/src/node_http2.h index 0bb2b0cb2891..f321bf0c1601 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -76,6 +76,8 @@ constexpr int kSessionStateSending = 0x10; constexpr int kSessionStateWriteInProgress = 0x20; constexpr int kSessionStateReadingStopped = 0x40; constexpr int kSessionStateReceivePaused = 0x80; +constexpr int kSessionStateReceiving = 0x100; +constexpr int kSessionStateClosePending = 0x200; // The Padding Strategy determines the method by which extra padding is // selected for HEADERS and DATA frames. These are configurable via the @@ -331,6 +333,10 @@ class Http2Stream : public AsyncWrap, // Destroy this stream instance and free all held memory. void Destroy(); + // Completes Destroy() after set_destroyed(); may run deferred until after + // nghttp2_session_mem_recv() returns. + void CompleteDestroyCleanup(); + bool is_destroyed() const { return flags_ & kStreamStateDestroyed; } @@ -659,6 +665,8 @@ class Http2Session : public AsyncWrap, IS_FLAG(write_in_progress, kSessionStateWriteInProgress) IS_FLAG(reading_stopped, kSessionStateReadingStopped) IS_FLAG(receive_paused, kSessionStateReceivePaused) + IS_FLAG(receiving, kSessionStateReceiving) + IS_FLAG(close_pending, kSessionStateClosePending) #undef IS_FLAG @@ -702,6 +710,10 @@ class Http2Session : public AsyncWrap, std::ranges::find(pending_rst_streams_, stream_id); } + void RemovePendingRstStream(int32_t stream_id) { + std::erase(pending_rst_streams_, stream_id); + } + // Handle reads/writes from the underlying network transport. uv_buf_t OnStreamAlloc(size_t suggested_size) override; void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override; @@ -951,6 +963,10 @@ class Http2Session : public AsyncWrap, std::vector outgoing_storage_; size_t outgoing_length_ = 0; std::vector pending_rst_streams_; + // Saved arguments for Close() deferred while nghttp2_session_mem_recv() + // callbacks are active. + uint32_t pending_close_code_ = NGHTTP2_NO_ERROR; + bool pending_close_socket_closed_ = false; // Count streams that have been rejected while being opened. Exceeding a fixed // limit will result in the session being destroyed, as an indication of a // misbehaving peer. This counter is reset once new streams are being @@ -965,6 +981,8 @@ class Http2Session : public AsyncWrap, void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); + void FinishClose(uint32_t code, bool socket_closed); + void MaybeFinishPendingClose(); void MaybeNotifyGracefulCloseComplete(); From e9d433fc44396b33378527cc3e9ad103fd165172 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 6 Aug 2026 15:01:53 +0200 Subject: [PATCH 2/2] http2: adapt receive deferral for v24 Node.js v24 does not include the later stream lifecycle changes that the original fix relies on. Preserve its reset ordering, avoid JavaScript callbacks after a deferred session close, and let destroyed streams finish without requesting trailers. Signed-off-by: Matteo Collina (cherry picked from commit 687a46df60e6211a6d0fbdcf3513125299440ae3) --- src/node_http2.cc | 34 ++++++++++++--- ...st-http2-session-destroy-during-receive.js | 41 +++++++++++++++++++ 2 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-http2-session-destroy-during-receive.js diff --git a/src/node_http2.cc b/src/node_http2.cc index 08aa544b3df5..b6bbb1a4edc3 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1074,6 +1074,11 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, int32_t id = GetFrameID(frame); Debug(session, "beginning headers for stream %d", id); + // Close() can be called by JavaScript from an earlier receive callback. + // Do not create streams that can no longer be exposed to JavaScript. + if (session->is_close_pending()) + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; + BaseObjectPtr stream = session->FindStream(id); // The common case is that we're creating a new stream. The less likely // case is that we're receiving a set of trailers @@ -1139,6 +1144,12 @@ int Http2Session::OnFrameReceive(nghttp2_session* handle, session->statistics_.frame_count++; Debug(session, "complete frame received: type: %d", frame->hd.type); + + // JavaScript may have closed the session from an earlier receive callback. + // FinishClose() runs after nghttp2_session_mem_recv() returns. + if (session->is_close_pending()) + return 0; + switch (frame->hd.type) { case NGHTTP2_DATA: return session->HandleDataFrame(frame); @@ -1405,6 +1416,12 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, if (len == 0) return 0; + // Close() can be called by JavaScript from an earlier receive callback. + // Ignore the rest of the buffered DATA because its stream may never have + // been exposed to JavaScript and therefore has no onread callback. + if (session->is_close_pending()) + return 0; + // Notify nghttp2 that we've consumed a chunk of data on the connection // so that it can send a WINDOW_UPDATE frame. This is a critical part of // the flow control process in http2 @@ -2608,12 +2625,17 @@ void Http2Stream::SubmitRstStream(const uint32_t code) { // Do not call `nghttp2_session_mem_send()` while nghttp2 is processing // incoming data. Sending may close the stream and free nghttp2 state // that is still in use by `nghttp2_session_mem_recv()`. - if (session_->is_receiving() && available_outbound_length_ == 0) { - if (is_stream_cancel(code)) { + if (session_->is_receiving()) { + // These resets must be submitted before the current callback returns. + // In particular, nghttp2 otherwise replaces ENHANCE_YOUR_CALM with + // INTERNAL_ERROR when OnHeaderCallback returns a temporal failure. + if (code == NGHTTP2_ENHANCE_YOUR_CALM || + code == NGHTTP2_REFUSED_STREAM) { + FlushRstStream(); + } else { + // Let queued DATA, including END_STREAM, be serialized before the reset. session_->AddPendingRstStream(id_); - return; } - FlushRstStream(); return; } @@ -2869,7 +2891,9 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, if (stream->available_outbound_length_ == 0 && !stream->is_writable()) { Debug(session, "no more data for stream %d", id); *flags |= NGHTTP2_DATA_FLAG_EOF; - if (stream->has_trailers()) { + // A deferred Destroy() cannot call back into JavaScript for trailers. + // Let the DATA frame end the stream instead. + if (stream->has_trailers() && !stream->is_destroyed()) { *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; stream->OnTrailers(); } diff --git a/test/parallel/test-http2-session-destroy-during-receive.js b/test/parallel/test-http2-session-destroy-during-receive.js new file mode 100644 index 000000000000..238d511fd83c --- /dev/null +++ b/test/parallel/test-http2-session-destroy-during-receive.js @@ -0,0 +1,41 @@ +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const fixtures = require('../common/fixtures'); +const http2 = require('http2'); + +// Regression test for closing a session while nghttp2 is processing several +// streams from the same input buffer. No stream created after the close can be +// exposed to JavaScript, so delivering its DATA would call a missing onread. +const server = http2.createSecureServer({ + key: fixtures.readKey('agent2-key.pem'), + cert: fixtures.readKey('agent2-cert.pem') +}); + +server.on('stream', common.mustCallAtLeast((stream) => { + stream.on('error', () => {}); + stream.session.destroy(); +}, 1)); + +server.listen(0, common.mustCall(() => { + const client = http2.connect(`https://localhost:${server.address().port}`, { + rejectUnauthorized: false + }); + client.on('error', () => {}); + client.on('close', common.mustCall(() => server.close())); + + client.on('remoteSettings', common.mustCall(() => { + for (let i = 0; i < 8; i++) { + const stream = client.request({ + ':method': 'POST', + ':path': `/${i}` + }); + stream.on('error', () => {}); + stream.resume(); + stream.end(Buffer.alloc(512)); + } + })); +}));