Commit 854dba6
http2: stop reading from socket if writes are in progress
If a write to the underlying socket finishes asynchronously, that
means that we cannot write any more data at that point without waiting
for it to finish. If this happens, we should also not be producing any
more input.
This is part of mitigating CVE-2019-9511/CVE-2019-9517.
Backport-PR-URL: #29124
PR-URL: #29122
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>1 parent a319168 commit 854dba6
2 files changed
Lines changed: 20 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1585,9 +1585,18 @@ void Http2Session::OnStreamAfterWriteImpl(WriteWrap* w, int status, void* ctx) { | |||
| 1585 | 1585 | Http2Session* session = static_cast<Http2Session*>(ctx); | |
| 1586 | 1586 | DEBUG_HTTP2SESSION2(session, "write finished with status %d", status); | |
| 1587 | 1587 | ||
| 1588 | + CHECK_NE(session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); | ||
| 1589 | + session->flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; | ||
| 1590 | + | ||
| 1588 | 1591 | // Inform all pending writes about their completion. | |
| 1589 | 1592 | session->ClearOutgoing(status); | |
| 1590 | 1593 | ||
| 1594 | + if ((session->flags_ & SESSION_STATE_READING_STOPPED) && | ||
| 1595 | + nghttp2_session_want_read(session->session_)) { | ||
| 1596 | + session->flags_ &= ~SESSION_STATE_READING_STOPPED; | ||
| 1597 | + session->stream_->ReadStart(); | ||
| 1598 | + } | ||
| 1599 | + | ||
| 1591 | 1600 | if (!(session->flags_ & SESSION_STATE_WRITE_SCHEDULED)) { | |
| 1592 | 1601 | // Schedule a new write if nghttp2 wants to send data. | |
| 1593 | 1602 | session->MaybeScheduleWrite(); | |
@@ -1627,10 +1636,13 @@ void Http2Session::MaybeScheduleWrite() { | |||
| 1627 | 1636 | } | |
| 1628 | 1637 | ||
| 1629 | 1638 | void Http2Session::MaybeStopReading() { | |
| 1639 | + if (flags_ & SESSION_STATE_READING_STOPPED) return; | ||
| 1630 | 1640 | int want_read = nghttp2_session_want_read(session_); | |
| 1631 | 1641 | DEBUG_HTTP2SESSION2(this, "wants read? %d", want_read); | |
| 1632 | - if (want_read == 0) | ||
| 1642 | + if (want_read == 0 || (flags_ & SESSION_STATE_WRITE_IN_PROGRESS)) { | ||
| 1643 | + flags_ |= SESSION_STATE_READING_STOPPED; | ||
| 1633 | 1644 | stream_->ReadStop(); | |
| 1645 | + } | ||
| 1634 | 1646 | } | |
| 1635 | 1647 | ||
| 1636 | 1648 | // Unset the sending state, finish up all current writes, and reset | |
@@ -1757,6 +1769,8 @@ uint8_t Http2Session::SendPendingData() { | |||
| 1757 | 1769 | ||
| 1758 | 1770 | chunks_sent_since_last_write_++; | |
| 1759 | 1771 | ||
| 1772 | + CHECK_EQ(flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); | ||
| 1773 | + | ||
| 1760 | 1774 | // DoTryWrite may modify both the buffer list start itself and the | |
| 1761 | 1775 | // base pointers/length of the individual buffers. | |
| 1762 | 1776 | uv_buf_t* writebufs = *bufs; | |
@@ -1766,8 +1780,11 @@ uint8_t Http2Session::SendPendingData() { | |||
| 1766 | 1780 | return 0; | |
| 1767 | 1781 | } | |
| 1768 | 1782 | ||
| 1783 | + flags_ |= SESSION_STATE_WRITE_IN_PROGRESS; | ||
| 1784 | + | ||
| 1769 | 1785 | WriteWrap* req = AllocateSend(); | |
| 1770 | 1786 | if (stream_->DoWrite(req, writebufs, count, nullptr) != 0) { | |
| 1787 | + flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; | ||
| 1771 | 1788 | req->Dispose(); | |
| 1772 | 1789 | } | |
| 1773 | 1790 | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -384,6 +384,8 @@ enum session_state_flags { | |||
| 384 | 384 | SESSION_STATE_CLOSED = 0x4, | |
| 385 | 385 | SESSION_STATE_CLOSING = 0x8, | |
| 386 | 386 | SESSION_STATE_SENDING = 0x10, | |
| 387 | + SESSION_STATE_WRITE_IN_PROGRESS = 0x20, | ||
| 388 | + SESSION_STATE_READING_STOPPED = 0x40, | ||
| 387 | 389 | }; | |
| 388 | 390 | ||
| 389 | 391 | // This allows for 4 default-sized frames with their frame headers | |
0 commit comments