GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — http2: avoid uaf while receiving and sending rst_stream · nodejs/node@563cab1

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • src
    • node_http2.cc
    • node_http2.h
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+97-6Lines changed: 97 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -815,6 +815,21 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
815815
return;
816816
set_closing();
817817

818+
// Do not flush GOAWAY from inside nghttp2_session_mem_recv() callbacks.
819+
// ConsumeHTTP2Data() finishes the close once mem_recv returns.
820+
if (is_receiving()) {
821+
set_close_pending();
822+
pending_close_code_ = code;
823+
pending_close_socket_closed_ = socket_closed;
824+
return;
825+
}
826+
827+
FinishClose(code, socket_closed);
828+
}
829+
830+
void Http2Session::FinishClose(uint32_t code, bool socket_closed) {
831+
CHECK(is_closing());
832+
818833
// Stop reading on the i/o stream
819834
if (stream_ != nullptr) {
820835
set_reading_stopped();
@@ -864,6 +879,12 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
864879
EmitStatistics();
865880
}
866881

882+
void Http2Session::MaybeFinishPendingClose() {
883+
if (!is_close_pending() || is_destroyed()) return;
884+
set_close_pending(false);
885+
FinishClose(pending_close_code_, pending_close_socket_closed_);
886+
}
887+
867888
// Locates an existing known stream by ID. nghttp2 has a similar method
868889
// but this is faster and does not fail if the stream is not found.
869890
BaseObjectPtr<Http2Stream> Http2Session::FindStream(int32_t id) {
@@ -958,11 +979,13 @@ void Http2Session::ConsumeHTTP2Data() {
958979
nghttp2_session_want_read(session_.get()));
959980
set_receive_paused(false);
960981
custom_recv_error_code_ = nullptr;
982+
set_receiving();
961983
ssize_t ret =
962984
nghttp2_session_mem_recv(session_.get(),
963985
reinterpret_cast<uint8_t*>(stream_buf_.base) +
964986
stream_buf_offset_,
965987
read_len);
988+
set_receiving(false);
966989
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
967990
CHECK_IMPLIES(custom_recv_error_code_ != nullptr, ret < 0);
968991

@@ -976,6 +999,10 @@ void Http2Session::ConsumeHTTP2Data() {
976999
// Even if all bytes were received, a paused stream may delay the
9771000
// nghttp2_on_frame_recv_callback which may have an END_STREAM flag.
9781001
stream_buf_offset_ += ret;
1002+
// Still complete a Close() deferred during mem_recv; do not fall through
1003+
// to SendPendingData() here (paused receives historically skip that flush
1004+
// because a write may already be in progress).
1005+
MaybeFinishPendingClose();
9791006
goto done;
9801007
}
9811008

@@ -986,12 +1013,23 @@ void Http2Session::ConsumeHTTP2Data() {
9861013
stream_buf_allocation_.reset();
9871014
stream_buf_ = uv_buf_init(nullptr, 0);
9881015

1016+
// Finish a Close() deferred during mem_recv before flushing, so GOAWAY is
1017+
// not written after pending RST_STREAM frames.
1018+
MaybeFinishPendingClose();
1019+
1020+
done:
1021+
// Finish a Close() deferred above before flushing, so GOAWAY is not written
1022+
// after pending RST_STREAM frames.
1023+
if (is_close_pending() && !is_destroyed()) {
1024+
set_close_pending(false);
1025+
FinishClose(pending_close_code_, pending_close_socket_closed_);
1026+
}
1027+
9891028
// Send any data that was queued up while processing the received data.
9901029
if (ret >= 0 && !is_destroyed()) {
9911030
SendPendingData();
9921031
}
9931032

994-
done:
9951033
if (ret < 0) [[unlikely]] {
9961034
Isolate* isolate = env()->isolate();
9971035
Debug(this,
@@ -1405,6 +1443,9 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
14051443
len -= avail;
14061444
stream->EmitRead(avail, buf);
14071445

1446+
// JS may have destroyed the stream from inside onread; stop delivering.
1447+
if (stream->is_destroyed()) break;
1448+
14081449
// If the stream owner (e.g. the JS Http2Stream) wants more data, just
14091450
// tell nghttp2 that all data has been consumed. Otherwise, defer until
14101451
// more data is being requested.
@@ -1962,6 +2003,7 @@ uint8_t Http2Session::SendPendingData() {
19622003
// SendPendingData should not be called recursively.
19632004
if (is_sending())
19642005
return 1;
2006+
19652007
// This is cleared by ClearOutgoing().
19662008
set_sending();
19672009

@@ -2372,10 +2414,48 @@ void Http2Stream::Destroy() {
23722414
// Do nothing if this stream instance is already destroyed
23732415
if (is_destroyed())
23742416
return;
2375-
if (session_->has_pending_rststream(id_))
2376-
FlushRstStream();
2417+
2418+
// Session may already be gone if destroy was deferred across a session
2419+
// teardown.
2420+
if (!session_) {
2421+
set_destroyed();
2422+
Detach();
2423+
return;
2424+
}
2425+
2426+
// Mark destroyed immediately so OnDataChunkReceived stops EmitRead into an
2427+
// already-destroyed JS stream (which would treat the byte count as errno).
23772428
set_destroyed();
23782429

2430+
// While mem_recv is active, do not FlushRstStream or RemoveStream yet:
2431+
// - FlushRstStream would close the nghttp2 stream before queued response
2432+
// DATA can be mem_send'd after receive returns.
2433+
// - RemoveStream would make OnSendData/Provider::OnRead fail to FindStream.
2434+
// Pending RSTs stay in pending_rst_streams_ and are flushed from
2435+
// ClearOutgoing after the post-receive SendPendingData.
2436+
if (session_->is_receiving()) {
2437+
BaseObjectPtr<Http2Stream> strong_ref{this};
2438+
env()->SetImmediate(
2439+
[this, strong_ref](Environment*) { CompleteDestroyCleanup(); });
2440+
return;
2441+
}
2442+
2443+
if (session_->has_pending_rststream(id_)) FlushRstStream();
2444+
2445+
CompleteDestroyCleanup();
2446+
}
2447+
2448+
void Http2Stream::CompleteDestroyCleanup() {
2449+
if (!session_) {
2450+
Detach();
2451+
return;
2452+
}
2453+
2454+
// Destroy() always set_destroyed() before scheduling or calling this.
2455+
CHECK(is_destroyed());
2456+
2457+
if (session_->has_pending_rststream(id_)) FlushRstStream();
2458+
23792459
Debug(this, "destroying stream");
23802460

23812461
// Wait until the start of the next loop to delete because there
@@ -2412,7 +2492,6 @@ void Http2Stream::Destroy() {
24122492
EmitStatistics();
24132493
}
24142494

2415-
24162495
// Initiates a response on the Http2Stream using data provided via the
24172496
// StreamBase Streams API.
24182497
int Http2Stream::SubmitResponse(const Http2Headers& headers, int options) {
@@ -2521,6 +2600,18 @@ void Http2Stream::SubmitRstStream(const uint32_t code) {
25212600
return code == NGHTTP2_CANCEL;
25222601
};
25232602

2603+
// Do not call `nghttp2_session_mem_send()` while nghttp2 is processing
2604+
// incoming data. Sending may close the stream and free nghttp2 state
2605+
// that is still in use by `nghttp2_session_mem_recv()`.
2606+
if (session_->is_receiving() && available_outbound_length_ == 0) {
2607+
if (is_stream_cancel(code)) {
2608+
session_->AddPendingRstStream(id_);
2609+
return;
2610+
}
2611+
FlushRstStream();
2612+
return;
2613+
}
2614+
25242615
// If RST_STREAM frame is received with error code NGHTTP2_CANCEL,
25252616
// add it to the pending list and don't force purge the data. It is
25262617
// to avoids the double free error due to unwanted behavior of nghttp2.
@@ -2556,8 +2647,8 @@ void Http2Stream::SubmitRstStream(const uint32_t code) {
25562647
}
25572648

25582649
void Http2Stream::FlushRstStream() {
2559-
if (is_destroyed())
2560-
return;
2650+
if (!session_) return;
2651+
session_->RemovePendingRstStream(id_);
25612652
Http2Scope h2scope(this);
25622653
CHECK_EQ(nghttp2_submit_rst_stream(
25632654
session_->session(),
Collapse file

‎src/node_http2.h‎

Copy file name to clipboardExpand all lines: src/node_http2.h
+18Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ constexpr int kSessionStateSending = 0x10;
7676
constexpr int kSessionStateWriteInProgress = 0x20;
7777
constexpr int kSessionStateReadingStopped = 0x40;
7878
constexpr int kSessionStateReceivePaused = 0x80;
79+
constexpr int kSessionStateReceiving = 0x100;
80+
constexpr int kSessionStateClosePending = 0x200;
7981

8082
// The Padding Strategy determines the method by which extra padding is
8183
// selected for HEADERS and DATA frames. These are configurable via the
@@ -331,6 +333,10 @@ class Http2Stream : public AsyncWrap,
331333
// Destroy this stream instance and free all held memory.
332334
void Destroy();
333335

336+
// Completes Destroy() after set_destroyed(); may run deferred until after
337+
// nghttp2_session_mem_recv() returns.
338+
void CompleteDestroyCleanup();
339+
334340
bool is_destroyed() const {
335341
return flags_ & kStreamStateDestroyed;
336342
}
@@ -659,6 +665,8 @@ class Http2Session : public AsyncWrap,
659665
IS_FLAG(write_in_progress, kSessionStateWriteInProgress)
660666
IS_FLAG(reading_stopped, kSessionStateReadingStopped)
661667
IS_FLAG(receive_paused, kSessionStateReceivePaused)
668+
IS_FLAG(receiving, kSessionStateReceiving)
669+
IS_FLAG(close_pending, kSessionStateClosePending)
662670

663671
#undef IS_FLAG
664672

@@ -702,6 +710,10 @@ class Http2Session : public AsyncWrap,
702710
std::ranges::find(pending_rst_streams_, stream_id);
703711
}
704712

713+
void RemovePendingRstStream(int32_t stream_id) {
714+
std::erase(pending_rst_streams_, stream_id);
715+
}
716+
705717
// Handle reads/writes from the underlying network transport.
706718
uv_buf_t OnStreamAlloc(size_t suggested_size) override;
707719
void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override;
@@ -951,6 +963,10 @@ class Http2Session : public AsyncWrap,
951963
std::vector<uint8_t> outgoing_storage_;
952964
size_t outgoing_length_ = 0;
953965
std::vector<int32_t> pending_rst_streams_;
966+
// Saved arguments for Close() deferred while nghttp2_session_mem_recv()
967+
// callbacks are active.
968+
uint32_t pending_close_code_ = NGHTTP2_NO_ERROR;
969+
bool pending_close_socket_closed_ = false;
954970
// Count streams that have been rejected while being opened. Exceeding a fixed
955971
// limit will result in the session being destroyed, as an indication of a
956972
// misbehaving peer. This counter is reset once new streams are being
@@ -965,6 +981,8 @@ class Http2Session : public AsyncWrap,
965981

966982
void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length);
967983
void ClearOutgoing(int status);
984+
void FinishClose(uint32_t code, bool socket_closed);
985+
void MaybeFinishPendingClose();
968986

969987
void MaybeNotifyGracefulCloseComplete();
970988

0 commit comments

Comments
0 (0)
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.