http2: fix write deadlock exposed by larger window sizes · nodejs/node@b4a7cd3 · GitHub
Skip to content

Commit b4a7cd3

Browse files
pimterryaduh95
authored andcommitted
http2: fix write deadlock exposed by larger window sizes
This removes a guard (no reads while write pending) that creates this deadlock, which was added as a security mechanism. This guard is redundant given then other existing mechanisms, and a test is added to demonstrate that. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: #65440 Backport-PR-URL: #65953 Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Tim Perry <pimterry@gmail.com>
1 parent b7e97f6 commit b4a7cd3

6 files changed

Lines changed: 327 additions & 86 deletions

File tree

‎benchmark/http2/full-duplex.js‎

Lines changed: 71 additions & 0 deletions

‎src/node_http2.cc‎

Lines changed: 13 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -970,45 +970,24 @@ ssize_t Http2Session::OnMaxFrameSizePadding(size_t frameLen,
970970
// quite expensive. This is a potential performance optimization target later.
971971
void Http2Session::ConsumeHTTP2Data() {
972972
CHECK_NOT_NULL(stream_buf_.base);
973-
CHECK_LE(stream_buf_offset_, stream_buf_.len);
974-
size_t read_len = stream_buf_.len - stream_buf_offset_;
975973

976974
// multiple side effects.
977-
Debug(this, "receiving %d bytes [wants data? %d]",
978-
read_len,
975+
Debug(this,
976+
"receiving %d bytes [wants data? %d]",
977+
stream_buf_.len,
979978
nghttp2_session_want_read(session_.get()));
980-
set_receive_paused(false);
981979
custom_recv_error_code_ = nullptr;
982980
set_receiving();
983981
ssize_t ret =
984-
nghttp2_session_mem_recv(session_.get(),
985-
reinterpret_cast<uint8_t*>(stream_buf_.base) +
986-
stream_buf_offset_,
987-
read_len);
982+
nghttp2_session_mem_recv(session_.get(),
983+
reinterpret_cast<uint8_t*>(stream_buf_.base),
984+
stream_buf_.len);
988985
set_receiving(false);
989986
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
990987
CHECK_IMPLIES(custom_recv_error_code_ != nullptr, ret < 0);
991988

992-
if (is_receive_paused()) {
993-
CHECK(is_reading_stopped());
994-
995-
CHECK_GT(ret, 0);
996-
CHECK_LE(static_cast<size_t>(ret), read_len);
997-
998-
// Mark the remainder of the data as available for later consumption.
999-
// Even if all bytes were received, a paused stream may delay the
1000-
// nghttp2_on_frame_recv_callback which may have an END_STREAM flag.
1001-
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();
1006-
goto done;
1007-
}
1008-
1009989
// We are done processing the current input chunk.
1010990
DecrementCurrentSessionMemory(stream_buf_.len);
1011-
stream_buf_offset_ = 0;
1012991
stream_buf_ab_.Reset();
1013992
stream_buf_allocation_.reset();
1014993
stream_buf_ = uv_buf_init(nullptr, 0);
@@ -1017,14 +996,6 @@ void Http2Session::ConsumeHTTP2Data() {
1017996
// not written after pending RST_STREAM frames.
1018997
MaybeFinishPendingClose();
1019998

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-
1028999
// Send any data that was queued up while processing the received data.
10291000
if (ret >= 0 && !is_destroyed()) {
10301001
SendPendingData();
@@ -1471,15 +1442,6 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
14711442
}
14721443
} while (len != 0);
14731444

1474-
// If we are currently waiting for a write operation to finish, we should
1475-
// tell nghttp2 that we want to wait before we process more input data.
1476-
if (session->is_write_in_progress()) {
1477-
CHECK(session->is_reading_stopped());
1478-
session->set_receive_paused();
1479-
Debug(session, "receive paused");
1480-
return NGHTTP2_ERR_PAUSE;
1481-
}
1482-
14831445
return 0;
14841446
}
14851447

@@ -1562,7 +1524,6 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
15621524
size_t offset = buf.base - session->stream_buf_.base;
15631525

15641526
// Verify that the data offset is inside the current read buffer.
1565-
CHECK_GE(offset, session->stream_buf_offset_);
15661527
CHECK_LE(offset, session->stream_buf_.len);
15671528
CHECK_LE(offset + buf.len, session->stream_buf_.len);
15681529

@@ -1877,11 +1838,6 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
18771838
return;
18781839
}
18791840

1880-
// If there is more incoming data queued up, consume it.
1881-
if (stream_buf_offset_ > 0) {
1882-
ConsumeHTTP2Data();
1883-
}
1884-
18851841
if (!is_write_scheduled() && !is_destroyed()) {
18861842
// Schedule a new write if nghttp2 wants to send data.
18871843
MaybeScheduleWrite();
@@ -1928,7 +1884,7 @@ void Http2Session::MaybeStopReading() {
19281884
if (is_reading_stopped() || is_closing()) return;
19291885
int want_read = nghttp2_session_want_read(session_.get());
19301886
Debug(this, "wants read? %d", want_read);
1931-
if (want_read == 0 || is_write_in_progress()) {
1887+
if (want_read == 0) {
19321888
set_reading_stopped();
19331889
stream_->ReadStop();
19341890
}
@@ -2188,7 +2144,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
21882144
Context::Scope context_scope(env()->context());
21892145
Http2Scope h2scope(this);
21902146
CHECK_NOT_NULL(stream_);
2191-
Debug(this, "receiving %d bytes, offset %d", nread, stream_buf_offset_);
2147+
Debug(this, "receiving %d bytes", nread);
21922148
std::unique_ptr<BackingStore> bs = env()->release_managed_buffer(buf_);
21932149

21942150
// Only pass data on if nread > 0
@@ -2203,40 +2159,18 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
22032159

22042160
statistics_.data_received += nread;
22052161

2206-
if (stream_buf_offset_ == 0 && static_cast<size_t>(nread) != bs->ByteLength())
2207-
[[likely]] {
2162+
// ConsumeHTTP2Data() always consumes the whole chunk, so there is never a
2163+
// partially processed buffer left over from a previous read.
2164+
DCHECK_NULL(stream_buf_.base);
2165+
2166+
if (static_cast<size_t>(nread) != bs->ByteLength()) [[likely]] {
22082167
// Shrink to the actual amount of used data.
22092168
std::unique_ptr<BackingStore> old_bs = std::move(bs);
22102169
bs = ArrayBuffer::NewBackingStore(
22112170
env()->isolate(),
22122171
nread,
22132172
BackingStoreInitializationMode::kUninitialized);
22142173
memcpy(bs->Data(), old_bs->Data(), nread);
2215-
} else {
2216-
// This is a very unlikely case, and should only happen if the ReadStart()
2217-
// call in OnStreamAfterWrite() immediately provides data. If that does
2218-
// happen, we concatenate the data we received with the already-stored
2219-
// pending input data, slicing off the already processed part.
2220-
size_t pending_len = stream_buf_.len - stream_buf_offset_;
2221-
std::unique_ptr<BackingStore> new_bs = ArrayBuffer::NewBackingStore(
2222-
env()->isolate(),
2223-
pending_len + nread,
2224-
BackingStoreInitializationMode::kUninitialized);
2225-
memcpy(static_cast<char*>(new_bs->Data()),
2226-
stream_buf_.base + stream_buf_offset_,
2227-
pending_len);
2228-
memcpy(static_cast<char*>(new_bs->Data()) + pending_len,
2229-
bs->Data(),
2230-
nread);
2231-
2232-
bs = std::move(new_bs);
2233-
nread = bs->ByteLength();
2234-
stream_buf_offset_ = 0;
2235-
stream_buf_ab_.Reset();
2236-
2237-
// We have now fully processed the stream_buf_ input chunk (by moving the
2238-
// remaining part into buf, which will be accounted for below).
2239-
DecrementCurrentSessionMemory(stream_buf_.len);
22402174
}
22412175

22422176
IncrementCurrentSessionMemory(nread);

‎src/node_http2.h‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,8 @@ constexpr int kSessionStateClosing = 0x8;
7676
constexpr int kSessionStateSending = 0x10;
7777
constexpr int kSessionStateWriteInProgress = 0x20;
7878
constexpr int kSessionStateReadingStopped = 0x40;
79-
constexpr int kSessionStateReceivePaused = 0x80;
80-
constexpr int kSessionStateReceiving = 0x100;
81-
constexpr int kSessionStateClosePending = 0x200;
79+
constexpr int kSessionStateReceiving = 0x80;
80+
constexpr int kSessionStateClosePending = 0x100;
8281

8382
// The Padding Strategy determines the method by which extra padding is
8483
// selected for HEADERS and DATA frames. These are configurable via the
@@ -669,7 +668,6 @@ class Http2Session : public AsyncWrap,
669668
IS_FLAG(sending, kSessionStateSending)
670669
IS_FLAG(write_in_progress, kSessionStateWriteInProgress)
671670
IS_FLAG(reading_stopped, kSessionStateReadingStopped)
672-
IS_FLAG(receive_paused, kSessionStateReceivePaused)
673671
IS_FLAG(receiving, kSessionStateReceiving)
674672
IS_FLAG(close_pending, kSessionStateClosePending)
675673

@@ -950,7 +948,6 @@ class Http2Session : public AsyncWrap,
950948
// will be set. stream_buf_ab_ is lazily created from stream_buf_allocation_.
951949
v8::Global<v8::ArrayBuffer> stream_buf_ab_;
952950
std::unique_ptr<v8::BackingStore> stream_buf_allocation_;
953-
size_t stream_buf_offset_ = 0;
954951
// Custom error code for errors that originated inside one of the callbacks
955952
// called by nghttp2_session_mem_recv.
956953
const char* custom_recv_error_code_ = nullptr;
Lines changed: 107 additions & 0 deletions

0 commit comments

Comments
 (0)