Skip to content

Commit c152449

Browse files
addaleaxBethGriggs
authored andcommitted
http2: pause input processing if sending output
If we are waiting for the ability to send more output, we should not process more input. This commit a) makes us send output earlier, during processing of input, if we accumulate a lot and b) allows interrupting the call into nghttp2 that processes input data and resuming it at a later time, if we do find ourselves in a position where we are waiting to be able to send more output. This is part of mitigating CVE-2019-9511/CVE-2019-9517. Backport-PR-URL: #29123 PR-URL: #29122 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent 0ce699c commit c152449

File tree

3 files changed

+140
-44
lines changed

3 files changed

+140
-44
lines changed

src/node_http2.cc

+89-42
Original file line numberDiff line numberDiff line change
@@ -869,31 +869,52 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen,
869869
// various callback functions. Each of these will typically result in a call
870870
// out to JavaScript so this particular function is rather hot and can be
871871
// quite expensive. This is a potential performance optimization target later.
872-
ssize_t Http2Session::Write(const uv_buf_t* bufs, size_t nbufs) {
873-
size_t total = 0;
874-
// Note that nghttp2_session_mem_recv is a synchronous operation that
875-
// will trigger a number of other callbacks. Those will, in turn have
872+
ssize_t Http2Session::ConsumeHTTP2Data() {
873+
CHECK_NOT_NULL(stream_buf_.base);
874+
CHECK_LT(stream_buf_offset_, stream_buf_.len);
875+
size_t read_len = stream_buf_.len - stream_buf_offset_;
876+
876877
// multiple side effects.
877-
for (size_t n = 0; n < nbufs; n++) {
878-
Debug(this, "receiving %d bytes [wants data? %d]",
879-
bufs[n].len,
880-
nghttp2_session_want_read(session_));
881-
ssize_t ret =
882-
nghttp2_session_mem_recv(session_,
883-
reinterpret_cast<uint8_t*>(bufs[n].base),
884-
bufs[n].len);
885-
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
886-
887-
if (ret < 0)
888-
return ret;
878+
Debug(this, "receiving %d bytes [wants data? %d]",
879+
read_len,
880+
nghttp2_session_want_read(session_));
881+
flags_ &= ~SESSION_STATE_NGHTTP2_RECV_PAUSED;
882+
ssize_t ret =
883+
nghttp2_session_mem_recv(session_,
884+
reinterpret_cast<uint8_t*>(stream_buf_.base) +
885+
stream_buf_offset_,
886+
read_len);
887+
CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
889888

890-
total += ret;
889+
if (flags_ & SESSION_STATE_NGHTTP2_RECV_PAUSED) {
890+
CHECK_NE(flags_ & SESSION_STATE_READING_STOPPED, 0);
891+
892+
CHECK_GT(ret, 0);
893+
CHECK_LE(static_cast<size_t>(ret), read_len);
894+
895+
if (static_cast<size_t>(ret) < read_len) {
896+
// Mark the remainder of the data as available for later consumption.
897+
stream_buf_offset_ += ret;
898+
return ret;
899+
}
891900
}
901+
902+
// We are done processing the current input chunk.
903+
DecrementCurrentSessionMemory(stream_buf_.len);
904+
stream_buf_offset_ = 0;
905+
stream_buf_ab_.Reset();
906+
free(stream_buf_allocation_.base);
907+
stream_buf_allocation_ = uv_buf_init(nullptr, 0);
908+
stream_buf_ = uv_buf_init(nullptr, 0);
909+
910+
if (ret < 0)
911+
return ret;
912+
892913
// Send any data that was queued up while processing the received data.
893914
if (!IsDestroyed()) {
894915
SendPendingData();
895916
}
896-
return total;
917+
return ret;
897918
}
898919

899920

@@ -1196,8 +1217,18 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
11961217
nghttp2_session_consume_stream(handle, id, avail);
11971218
else
11981219
stream->inbound_consumed_data_while_paused_ += avail;
1220+
1221+
// If we have a gathered a lot of data for output, try sending it now.
1222+
if (session->outgoing_length_ > 4096) session->SendPendingData();
11991223
} while (len != 0);
12001224

1225+
// If we are currently waiting for a write operation to finish, we should
1226+
// tell nghttp2 that we want to wait before we process more input data.
1227+
if (session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS) {
1228+
session->flags_ |= SESSION_STATE_NGHTTP2_RECV_PAUSED;
1229+
return NGHTTP2_ERR_PAUSE;
1230+
}
1231+
12011232
return 0;
12021233
}
12031234

@@ -1289,6 +1320,7 @@ void Http2StreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
12891320
size_t offset = buf.base - session->stream_buf_.base;
12901321

12911322
// Verify that the data offset is inside the current read buffer.
1323+
CHECK_GE(offset, session->stream_buf_offset_);
12921324
CHECK_LE(offset, session->stream_buf_.len);
12931325
CHECK_LE(offset + buf.len, session->stream_buf_.len);
12941326

@@ -1586,6 +1618,11 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
15861618
stream_->ReadStart();
15871619
}
15881620

1621+
// If there is more incoming data queued up, consume it.
1622+
if (stream_buf_offset_ > 0) {
1623+
ConsumeHTTP2Data();
1624+
}
1625+
15891626
if (!(flags_ & SESSION_STATE_WRITE_SCHEDULED)) {
15901627
// Schedule a new write if nghttp2 wants to send data.
15911628
MaybeScheduleWrite();
@@ -1643,6 +1680,7 @@ void Http2Session::ClearOutgoing(int status) {
16431680

16441681
if (outgoing_buffers_.size() > 0) {
16451682
outgoing_storage_.clear();
1683+
outgoing_length_ = 0;
16461684

16471685
std::vector<nghttp2_stream_write> current_outgoing_buffers_;
16481686
current_outgoing_buffers_.swap(outgoing_buffers_);
@@ -1669,6 +1707,11 @@ void Http2Session::ClearOutgoing(int status) {
16691707
}
16701708
}
16711709

1710+
void Http2Session::PushOutgoingBuffer(nghttp2_stream_write&& write) {
1711+
outgoing_length_ += write.buf.len;
1712+
outgoing_buffers_.emplace_back(std::move(write));
1713+
}
1714+
16721715
// Queue a given block of data for sending. This always creates a copy,
16731716
// so it is used for the cases in which nghttp2 requests sending of a
16741717
// small chunk of data.
@@ -1681,7 +1724,7 @@ void Http2Session::CopyDataIntoOutgoing(const uint8_t* src, size_t src_length) {
16811724
// of the outgoing_buffers_ vector may invalidate the pointer.
16821725
// The correct base pointers will be set later, before writing to the
16831726
// underlying socket.
1684-
outgoing_buffers_.emplace_back(nghttp2_stream_write {
1727+
PushOutgoingBuffer(nghttp2_stream_write {
16851728
uv_buf_init(nullptr, src_length)
16861729
});
16871730
}
@@ -1804,13 +1847,13 @@ int Http2Session::OnSendData(
18041847
if (write.buf.len <= length) {
18051848
// This write does not suffice by itself, so we can consume it completely.
18061849
length -= write.buf.len;
1807-
session->outgoing_buffers_.emplace_back(std::move(write));
1850+
session->PushOutgoingBuffer(std::move(write));
18081851
stream->queue_.pop();
18091852
continue;
18101853
}
18111854

18121855
// Slice off `length` bytes of the first write in the queue.
1813-
session->outgoing_buffers_.emplace_back(nghttp2_stream_write {
1856+
session->PushOutgoingBuffer(nghttp2_stream_write {
18141857
uv_buf_init(write.buf.base, length)
18151858
});
18161859
write.buf.base += length;
@@ -1820,7 +1863,7 @@ int Http2Session::OnSendData(
18201863

18211864
if (frame->data.padlen > 0) {
18221865
// Send padding if that was requested.
1823-
session->outgoing_buffers_.emplace_back(nghttp2_stream_write {
1866+
session->PushOutgoingBuffer(nghttp2_stream_write {
18241867
uv_buf_init(const_cast<char*>(zero_bytes_256), frame->data.padlen - 1)
18251868
});
18261869
}
@@ -1853,8 +1896,6 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
18531896
Http2Scope h2scope(this);
18541897
CHECK_NOT_NULL(stream_);
18551898
Debug(this, "receiving %d bytes", nread);
1856-
CHECK_EQ(stream_buf_allocation_.base, nullptr);
1857-
CHECK(stream_buf_ab_.IsEmpty());
18581899

18591900
// Only pass data on if nread > 0
18601901
if (nread <= 0) {
@@ -1865,26 +1906,33 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
18651906
return;
18661907
}
18671908

1868-
// Shrink to the actual amount of used data.
18691909
uv_buf_t buf = buf_;
1870-
buf.base = Realloc(buf.base, nread);
18711910

1872-
IncrementCurrentSessionMemory(nread);
1873-
OnScopeLeave on_scope_leave([&]() {
1874-
// Once finished handling this write, reset the stream buffer.
1875-
// The memory has either been free()d or was handed over to V8.
1876-
// We use `nread` instead of `buf.size()` here, because the buffer is
1877-
// cleared as part of the `.ToArrayBuffer()` call below.
1878-
DecrementCurrentSessionMemory(nread);
1911+
statistics_.data_received += nread;
1912+
1913+
if (UNLIKELY(stream_buf_offset_ > 0)) {
1914+
// This is a very unlikely case, and should only happen if the ReadStart()
1915+
// call in OnStreamAfterWrite() immediately provides data. If that does
1916+
// happen, we concatenate the data we received with the already-stored
1917+
// pending input data, slicing off the already processed part.
1918+
char* new_buf = Malloc(stream_buf_.len - stream_buf_offset_ + nread);
1919+
memcpy(new_buf,
1920+
stream_buf_.base + stream_buf_offset_,
1921+
stream_buf_.len - stream_buf_offset_);
1922+
memcpy(new_buf + stream_buf_.len - stream_buf_offset_,
1923+
buf.base,
1924+
nread);
1925+
free(buf.base);
1926+
nread = stream_buf_.len - stream_buf_offset_ + nread;
1927+
buf = uv_buf_init(new_buf, nread);
1928+
stream_buf_offset_ = 0;
18791929
stream_buf_ab_.Reset();
1880-
free(stream_buf_allocation_.base);
1881-
stream_buf_allocation_ = uv_buf_init(nullptr, 0);
1882-
stream_buf_ = uv_buf_init(nullptr, 0);
1883-
});
1930+
DecrementCurrentSessionMemory(stream_buf_offset_);
1931+
}
18841932

1885-
// Make sure that there was no read previously active.
1886-
CHECK_NULL(stream_buf_.base);
1887-
CHECK_EQ(stream_buf_.len, 0);
1933+
// Shrink to the actual amount of used data.
1934+
buf.base = Realloc(buf.base, nread);
1935+
IncrementCurrentSessionMemory(nread);
18881936

18891937
// Remember the current buffer, so that OnDataChunkReceived knows the
18901938
// offset of a DATA frame's data into the socket read buffer.
@@ -1903,8 +1951,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) {
19031951
// to copy memory.
19041952
stream_buf_allocation_ = buf;
19051953

1906-
statistics_.data_received += nread;
1907-
ssize_t ret = Write(&stream_buf_, 1);
1954+
ssize_t ret = ConsumeHTTP2Data();
19081955

19091956
if (UNLIKELY(ret < 0)) {
19101957
Debug(this, "fatal error receiving data: %d", ret);

src/node_http2.h

+12-2
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,7 @@ enum session_state_flags {
336336
SESSION_STATE_SENDING = 0x10,
337337
SESSION_STATE_WRITE_IN_PROGRESS = 0x20,
338338
SESSION_STATE_READING_STOPPED = 0x40,
339+
SESSION_STATE_NGHTTP2_RECV_PAUSED = 0x80
339340
};
340341

341342
typedef uint32_t(*get_setting)(nghttp2_session* session,
@@ -775,14 +776,15 @@ class Http2Session : public AsyncWrap, public StreamListener {
775776
// Indicates whether there currently exist outgoing buffers for this stream.
776777
bool HasWritesOnSocketForStream(Http2Stream* stream);
777778

778-
// Write data to the session
779-
ssize_t Write(const uv_buf_t* bufs, size_t nbufs);
779+
// Write data from stream_buf_ to the session
780+
ssize_t ConsumeHTTP2Data();
780781

781782
void MemoryInfo(MemoryTracker* tracker) const override {
782783
tracker->TrackField("streams", streams_);
783784
tracker->TrackField("outstanding_pings", outstanding_pings_);
784785
tracker->TrackField("outstanding_settings", outstanding_settings_);
785786
tracker->TrackField("outgoing_buffers", outgoing_buffers_);
787+
tracker->TrackFieldWithSize("stream_buf", stream_buf_.len);
786788
tracker->TrackFieldWithSize("outgoing_storage", outgoing_storage_.size());
787789
tracker->TrackFieldWithSize("pending_rst_streams",
788790
pending_rst_streams_.size() * sizeof(int32_t));
@@ -845,6 +847,9 @@ class Http2Session : public AsyncWrap, public StreamListener {
845847
}
846848

847849
void DecrementCurrentSessionMemory(uint64_t amount) {
850+
#ifdef DEBUG
851+
CHECK_LE(amount, current_session_memory_);
852+
#endif
848853
current_session_memory_ -= amount;
849854
}
850855

@@ -1007,8 +1012,11 @@ class Http2Session : public AsyncWrap, public StreamListener {
10071012
uint32_t chunks_sent_since_last_write_ = 0;
10081013

10091014
uv_buf_t stream_buf_ = uv_buf_init(nullptr, 0);
1015+
// When processing input data, either stream_buf_ab_ or stream_buf_allocation_
1016+
// will be set. stream_buf_ab_ is lazily created from stream_buf_allocation_.
10101017
v8::Global<v8::ArrayBuffer> stream_buf_ab_;
10111018
uv_buf_t stream_buf_allocation_ = uv_buf_init(nullptr, 0);
1019+
size_t stream_buf_offset_ = 0;
10121020

10131021
size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
10141022
std::queue<Http2Ping*> outstanding_pings_;
@@ -1018,6 +1026,7 @@ class Http2Session : public AsyncWrap, public StreamListener {
10181026

10191027
std::vector<nghttp2_stream_write> outgoing_buffers_;
10201028
std::vector<uint8_t> outgoing_storage_;
1029+
size_t outgoing_length_ = 0;
10211030
std::vector<int32_t> pending_rst_streams_;
10221031
// Count streams that have been rejected while being opened. Exceeding a fixed
10231032
// limit will result in the session being destroyed, as an indication of a
@@ -1027,6 +1036,7 @@ class Http2Session : public AsyncWrap, public StreamListener {
10271036
// Also use the invalid frame count as a measure for rejecting input frames.
10281037
int32_t invalid_frame_count_ = 0;
10291038

1039+
void PushOutgoingBuffer(nghttp2_stream_write&& write);
10301040
void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length);
10311041
void ClearOutgoing(int status);
10321042

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (!common.hasCrypto)
4+
common.skip('missing crypto');
5+
6+
const fixtures = require('../common/fixtures');
7+
const assert = require('assert');
8+
const http2 = require('http2');
9+
10+
const content = fixtures.readSync('person-large.jpg');
11+
12+
const server = http2.createServer({
13+
maxSessionMemory: 1000
14+
});
15+
server.on('stream', (stream, headers) => {
16+
stream.respond({
17+
'content-type': 'image/jpeg',
18+
':status': 200
19+
});
20+
stream.end(content);
21+
});
22+
server.unref();
23+
24+
server.listen(0, common.mustCall(() => {
25+
const client = http2.connect(`http://localhost:${server.address().port}/`);
26+
27+
let finished = 0;
28+
for (let i = 0; i < 100; i++) {
29+
const req = client.request({ ':path': '/' }).end();
30+
const chunks = [];
31+
req.on('data', (chunk) => {
32+
chunks.push(chunk);
33+
});
34+
req.on('end', common.mustCall(() => {
35+
assert.deepStrictEqual(Buffer.concat(chunks), content);
36+
if (++finished === 100) client.close();
37+
}));
38+
}
39+
}));

0 commit comments

Comments
 (0)