Harden QuicStream::flow_controller_
This CL unblocks merge. Calling QuicheOptional::operator-> when there is no value present is undefined behavior. Chrome's implementation of QuicheOptional is more strict than the C++ standard library used in google3, and in debug builds it asserts when operator-> is used even though there is no value present. This is causing tests to fail and blocking merge.
This CL protects every single call to QuicStream::flow_controller_::operator-> with a check that verifies that a value was present. In almost all cases, it adds a QUIC_BUG to prove that this never happens in practice. The only case that does not have a QUIC_BUG is inside OnClose because this seems to be leveraged by multiple of our tests (QuicSessionTestClient.InvalidStreamFlowControlWindowInHandshake, QuicSessionTestServer.DontCallOnWriteBlockedForDisconnectedConnection, QuicSpdySessionTestClient.BadStreamFramePendingStream). We've confirmed that this CL fixes the merge issue.
Avoid undefined behavior, not flag protected
PiperOrigin-RevId: 311355214
Change-Id: I0e7d8c0c97815e1ed6f19509f0e64dfda8a32e5c
diff --git a/quic/core/quic_stream.cc b/quic/core/quic_stream.cc
index 7355aea..40a67bb 100644
--- a/quic/core/quic_stream.cc
+++ b/quic/core/quic_stream.cc
@@ -4,6 +4,7 @@
#include "net/third_party/quiche/src/quic/core/quic_stream.h"
+#include <limits>
#include <string>
#include "net/third_party/quiche/src/quic/core/quic_error_codes.h"
@@ -461,7 +462,10 @@
MaybeIncreaseHighestReceivedOffset(frame.offset + frame_payload_size)) {
// As the highest received offset has changed, check to see if this is a
// violation of flow control.
- if (flow_controller_->FlowControlViolation() ||
+ QUIC_BUG_IF(!flow_controller_.has_value())
+ << ENDPOINT << "OnStreamFrame called on stream without flow control";
+ if ((flow_controller_.has_value() &&
+ flow_controller_->FlowControlViolation()) ||
connection_flow_controller_->FlowControlViolation()) {
OnUnrecoverableError(QUIC_FLOW_CONTROL_RECEIVED_TOO_MUCH_DATA,
"Flow control violation after increasing offset");
@@ -525,7 +529,10 @@
}
MaybeIncreaseHighestReceivedOffset(frame.byte_offset);
- if (flow_controller_->FlowControlViolation() ||
+ QUIC_BUG_IF(!flow_controller_.has_value())
+ << ENDPOINT << "OnStreamReset called on stream without flow control";
+ if ((flow_controller_.has_value() &&
+ flow_controller_->FlowControlViolation()) ||
connection_flow_controller_->FlowControlViolation()) {
OnUnrecoverableError(QUIC_FLOW_CONTROL_RECEIVED_TOO_MUCH_DATA,
"Flow control violation after increasing offset");
@@ -678,6 +685,11 @@
}
void QuicStream::MaybeSendBlocked() {
+ if (!flow_controller_.has_value()) {
+ QUIC_BUG << ENDPOINT
+ << "MaybeSendBlocked called on stream without flow control";
+ return;
+ }
if (flow_controller_->ShouldSendBlocked()) {
session_->SendBlocked(id_);
}
@@ -833,7 +845,8 @@
rst_sent_ = true;
}
- if (flow_controller_->FlowControlViolation() ||
+ if (!flow_controller_.has_value() ||
+ flow_controller_->FlowControlViolation() ||
connection_flow_controller_->FlowControlViolation()) {
return;
}
@@ -855,6 +868,12 @@
return;
}
+ if (!flow_controller_.has_value()) {
+ QUIC_BUG << ENDPOINT
+ << "OnWindowUpdateFrame called on stream without flow control";
+ return;
+ }
+
if (flow_controller_->UpdateSendWindowOffset(frame.max_data)) {
// Let session unblock this stream.
session_->MarkConnectionLevelWriteBlocked(id_);
@@ -863,6 +882,12 @@
bool QuicStream::MaybeIncreaseHighestReceivedOffset(
QuicStreamOffset new_offset) {
+ if (!flow_controller_.has_value()) {
+ QUIC_BUG << ENDPOINT
+ << "MaybeIncreaseHighestReceivedOffset called on stream without "
+ "flow control";
+ return false;
+ }
uint64_t increment =
new_offset - flow_controller_->highest_received_byte_offset();
if (!flow_controller_->UpdateHighestReceivedOffset(new_offset)) {
@@ -881,6 +906,11 @@
}
void QuicStream::AddBytesSent(QuicByteCount bytes) {
+ if (!flow_controller_.has_value()) {
+ QUIC_BUG << ENDPOINT
+ << "AddBytesSent called on stream without flow control";
+ return;
+ }
flow_controller_->AddBytesSent(bytes);
if (stream_contributes_to_connection_flow_control_) {
connection_flow_controller_->AddBytesSent(bytes);
@@ -894,6 +924,12 @@
// QuicStreamSequencers used by QuicCryptoStream.
return;
}
+ if (!flow_controller_.has_value()) {
+ QUIC_BUG
+ << ENDPOINT
+ << "AddBytesConsumed called on non-crypto stream without flow control";
+ return;
+ }
// Only adjust stream level flow controller if still reading.
if (!read_side_closed_) {
flow_controller_->AddBytesConsumed(bytes);
@@ -905,6 +941,11 @@
}
bool QuicStream::ConfigSendWindowOffset(QuicStreamOffset new_offset) {
+ if (!flow_controller_.has_value()) {
+ QUIC_BUG << ENDPOINT
+ << "ConfigSendWindowOffset called on stream without flow control";
+ return false;
+ }
if (perspective_ == Perspective::IS_CLIENT &&
session()->version().AllowsLowFlowControlLimits() &&
new_offset < flow_controller_->send_window_offset()) {
@@ -1072,7 +1113,14 @@
bool fin = fin_buffered_;
// How much data flow control permits to be written.
- QuicByteCount send_window = flow_controller_->SendWindowSize();
+ QuicByteCount send_window;
+ if (flow_controller_.has_value()) {
+ send_window = flow_controller_->SendWindowSize();
+ } else {
+ send_window = std::numeric_limits<QuicByteCount>::max();
+ QUIC_BUG << ENDPOINT
+ << "WriteBufferedData called on stream without flow control";
+ }
if (stream_contributes_to_connection_flow_control_) {
send_window =
std::min(send_window, connection_flow_controller_->SendWindowSize());
