Ensure QUIC crypto send buffer does not exceed its size limit Note to reviewer(s): Please double check that the new error code looks good and that it should indeed map to FLOW_CONTROL_ERROR (wasn't sure). Protected by FLAGS_quic_reloadable_flag_quic_bounded_crypto_send_buffer. PiperOrigin-RevId: 451167627
diff --git a/quiche/quic/core/quic_crypto_stream.cc b/quiche/quic/core/quic_crypto_stream.cc index 3839db3..c85b9ab 100644 --- a/quiche/quic/core/quic_crypto_stream.cc +++ b/quiche/quic/core/quic_crypto_stream.cc
@@ -13,6 +13,7 @@ #include "quiche/quic/core/crypto/crypto_utils.h" #include "quiche/quic/core/frames/quic_crypto_frame.h" #include "quiche/quic/core/quic_connection.h" +#include "quiche/quic/core/quic_error_codes.h" #include "quiche/quic/core/quic_session.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/core/quic_utils.h" @@ -145,17 +146,41 @@ return; } const bool had_buffered_data = HasBufferedCryptoFrames(); - // Append |data| to the send buffer for this encryption level. QuicStreamSendBuffer* send_buffer = &substreams_[QuicUtils::GetPacketNumberSpace(level)].send_buffer; QuicStreamOffset offset = send_buffer->stream_offset(); + + // Ensure this data does not cause the send buffer for this encryption level + // to exceed its size limit. + if (GetQuicReloadableFlag(quic_bounded_crypto_send_buffer)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_bounded_crypto_send_buffer); + QUIC_BUG_IF(quic_crypto_stream_offset_lt_bytes_written, + offset < send_buffer->stream_bytes_written()); + uint64_t current_buffer_size = + offset - std::min(offset, send_buffer->stream_bytes_written()); + if (current_buffer_size > 0) { + QUIC_CODE_COUNT(quic_received_crypto_data_with_non_empty_send_buffer); + if (BufferSizeLimitForLevel(level) < + (current_buffer_size + data.length())) { + QUIC_BUG(quic_crypto_send_buffer_overflow) + << absl::StrCat("Too much data for crypto send buffer with level: ", + EncryptionLevelToString(level), + ", current_buffer_size: ", current_buffer_size, + ", data length: ", data.length()); + OnUnrecoverableError(QUIC_INTERNAL_ERROR, + "Too much data for crypto send buffer"); + return; + } + } + } + + // Append |data| to the send buffer for this encryption level. send_buffer->SaveStreamData(data); if (kMaxStreamLength - offset < data.length()) { QUIC_BUG(quic_bug_10322_2) << "Writing too much crypto handshake data"; - // TODO(nharper): Switch this to an IETF QUIC error code, possibly - // INTERNAL_ERROR? - OnUnrecoverableError(QUIC_STREAM_LENGTH_OVERFLOW, + OnUnrecoverableError(QUIC_INTERNAL_ERROR, "Writing too much crypto handshake data"); + return; } if (had_buffered_data) { // Do not try to write if there is buffered data. @@ -228,7 +253,6 @@ QuicStream::OnStreamDataConsumed(bytes_consumed); } - bool QuicCryptoStream::HasPendingCryptoRetransmission() const { if (!QuicVersionUsesCryptoFrames(session()->transport_version())) { return false;
diff --git a/quiche/quic/core/quic_crypto_stream_test.cc b/quiche/quic/core/quic_crypto_stream_test.cc index ea99926..f5d1f2e 100644 --- a/quiche/quic/core/quic_crypto_stream_test.cc +++ b/quiche/quic/core/quic_crypto_stream_test.cc
@@ -13,8 +13,10 @@ #include "quiche/quic/core/crypto/crypto_handshake.h" #include "quiche/quic/core/crypto/crypto_protocol.h" #include "quiche/quic/core/crypto/null_encrypter.h" +#include "quiche/quic/core/quic_error_codes.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/core/quic_utils.h" +#include "quiche/quic/platform/api/quic_expect_bug.h" #include "quiche/quic/platform/api/quic_socket_address.h" #include "quiche/quic/platform/api/quic_test.h" #include "quiche/quic/test_tools/crypto_test_utils.h" @@ -652,6 +654,50 @@ } } +TEST_F(QuicCryptoStreamTest, WriteCryptoDataExceedsSendBufferLimit) { + if (!QuicVersionUsesCryptoFrames(connection_->transport_version())) { + return; + } + EXPECT_EQ(ENCRYPTION_INITIAL, connection_->encryption_level()); + int32_t buffer_limit = GetQuicFlag(FLAGS_quic_max_buffered_crypto_bytes); + + // Write data larger than the buffer limit, when there is no existing data in + // the buffer. Data is sent rather than closing the connection. + EXPECT_FALSE(stream_->HasBufferedCryptoFrames()); + int32_t over_limit = buffer_limit + 1; + EXPECT_CALL(*connection_, SendCryptoData(ENCRYPTION_INITIAL, over_limit, 0)) + // All the data is sent, no resulting buffer. + .WillOnce(Return(over_limit)); + std::string large_data(over_limit, 'a'); + stream_->WriteCryptoData(ENCRYPTION_INITIAL, large_data); + + // Write data to the buffer up to the limit. One byte gets sent. + EXPECT_FALSE(stream_->HasBufferedCryptoFrames()); + EXPECT_CALL(*connection_, + SendCryptoData(ENCRYPTION_INITIAL, buffer_limit, over_limit)) + .WillOnce(Return(1)); + std::string data(buffer_limit, 'a'); + stream_->WriteCryptoData(ENCRYPTION_INITIAL, data); + EXPECT_TRUE(stream_->HasBufferedCryptoFrames()); + + // Write another byte that is not sent (due to there already being data in the + // buffer); send buffer is now full. + EXPECT_CALL(*connection_, SendCryptoData(_, _, _)).Times(0); + std::string data2(1, 'a'); + stream_->WriteCryptoData(ENCRYPTION_INITIAL, data2); + EXPECT_TRUE(stream_->HasBufferedCryptoFrames()); + + // Writing an additional byte to the send buffer closes the connection. + if (GetQuicReloadableFlag(quic_bounded_crypto_send_buffer)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_bounded_crypto_send_buffer); + EXPECT_CALL(*connection_, CloseConnection(QUIC_INTERNAL_ERROR, _, _)); + EXPECT_QUIC_BUG( + stream_->WriteCryptoData(ENCRYPTION_INITIAL, data2), + "Too much data for crypto send buffer with level: ENCRYPTION_INITIAL, " + "current_buffer_size: 16384, data length: 1"); + } +} + TEST_F(QuicCryptoStreamTest, WriteBufferedCryptoFrames) { if (!QuicVersionUsesCryptoFrames(connection_->transport_version())) { return;
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index ddb7948..c4f9f91 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -31,6 +31,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_allow_client_enabled_bbr_v2, true) // If true, close read side but not write side in QuicSpdyStream::OnStreamReset(). QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_fix_on_stream_reset, true) +// If true, close the connection if a crypto send buffer exceeds its size limit. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_bounded_crypto_send_buffer, false) // If true, consider original connection ID as active before handshake completes. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_consider_original_connection_id_as_active_pre_handshake, false) // If true, consolidate more logic into SetRetransmissionAlarm to ensure the logic is applied consistently.