Move QuicDataReader::ReadVarIntU32 to QuicFramer for more detailed framer errors.
gfe-relnote: protected by gfe2_reloadable_flag_quic_enable_version_draft_25_v3
PiperOrigin-RevId: 302554288
Change-Id: I8ac6e2ba2ce4012a49d796ce7cb5b23879c1a12c
diff --git a/quic/core/quic_data_reader.cc b/quic/core/quic_data_reader.cc
index 0a09a9f..c82e051 100644
--- a/quic/core/quic_data_reader.cc
+++ b/quic/core/quic_data_reader.cc
@@ -174,19 +174,5 @@
return ReadStringPiece(result, result_length);
}
-bool QuicDataReader::ReadVarIntU32(uint32_t* result) {
- uint64_t temp_uint64;
- // TODO(fkastenholz): We should disambiguate read-errors from
- // value errors.
- if (!this->ReadVarInt62(&temp_uint64)) {
- return false;
- }
- if (temp_uint64 > kMaxQuicStreamId) {
- return false;
- }
- *result = static_cast<uint32_t>(temp_uint64);
- return true;
-}
-
#undef ENDPOINT // undef for jumbo builds
} // namespace quic
diff --git a/quic/core/quic_data_reader.h b/quic/core/quic_data_reader.h
index 6b5c4d0..13499ea 100644
--- a/quic/core/quic_data_reader.h
+++ b/quic/core/quic_data_reader.h
@@ -86,12 +86,6 @@
// Forwards the internal iterator on success.
// Returns true on success, false otherwise.
bool ReadStringPieceVarInt62(quiche::QuicheStringPiece* result);
-
- // Convenience method that reads a uint32_t.
- // Attempts to read a varint into a uint32_t. using ReadVarInt62 and
- // returns false if there is a read error or if the value is
- // greater than (2^32)-1.
- bool ReadVarIntU32(uint32_t* result);
};
} // namespace quic
diff --git a/quic/core/quic_data_writer_test.cc b/quic/core/quic_data_writer_test.cc
index 36038cf..e05af21 100644
--- a/quic/core/quic_data_writer_test.cc
+++ b/quic/core/quic_data_writer_test.cc
@@ -9,6 +9,7 @@
#include "net/third_party/quiche/src/quic/core/quic_connection_id.h"
#include "net/third_party/quiche/src/quic/core/quic_data_reader.h"
+#include "net/third_party/quiche/src/quic/core/quic_types.h"
#include "net/third_party/quiche/src/quic/core/quic_utils.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_expect_bug.h"
#include "net/third_party/quiche/src/quic/platform/api/quic_flags.h"
@@ -1081,7 +1082,7 @@
}
// Test encoding/decoding stream-id values.
-void EncodeDecodeStreamId(uint64_t value_in, bool expected_decode_result) {
+void EncodeDecodeStreamId(uint64_t value_in) {
char buffer[1 * kMultiVarCount];
memset(buffer, 0, sizeof(buffer));
@@ -1093,35 +1094,28 @@
QuicDataReader reader(buffer, sizeof(buffer),
quiche::Endianness::NETWORK_BYTE_ORDER);
QuicStreamId received_stream_id;
- bool read_result = reader.ReadVarIntU32(&received_stream_id);
- EXPECT_EQ(expected_decode_result, read_result);
- if (read_result) {
- EXPECT_EQ(value_in, received_stream_id);
- }
+ uint64_t temp;
+ EXPECT_TRUE(reader.ReadVarInt62(&temp));
+ received_stream_id = static_cast<QuicStreamId>(temp);
+ EXPECT_EQ(value_in, received_stream_id);
}
// Test writing & reading stream-ids of various value.
TEST_P(QuicDataWriterTest, StreamId1) {
// Check a 1-byte QuicStreamId, should work
- EncodeDecodeStreamId(UINT64_C(0x15), true);
+ EncodeDecodeStreamId(UINT64_C(0x15));
// Check a 2-byte QuicStream ID. It should work.
- EncodeDecodeStreamId(UINT64_C(0x1567), true);
+ EncodeDecodeStreamId(UINT64_C(0x1567));
// Check a QuicStreamId that requires 4 bytes of encoding
// This should work.
- EncodeDecodeStreamId(UINT64_C(0x34567890), true);
+ EncodeDecodeStreamId(UINT64_C(0x34567890));
// Check a QuicStreamId that requires 8 bytes of encoding
// but whose value is in the acceptable range.
// This should work.
- EncodeDecodeStreamId(UINT64_C(0xf4567890), true);
-
- // Check QuicStreamIds that require 8 bytes of encoding
- // and whose value is not acceptable.
- // This should fail.
- EncodeDecodeStreamId(UINT64_C(0x100000000), false);
- EncodeDecodeStreamId(UINT64_C(0x3fffffffffffffff), false);
+ EncodeDecodeStreamId(UINT64_C(0xf4567890));
}
TEST_P(QuicDataWriterTest, WriteRandomBytes) {
@@ -1167,9 +1161,7 @@
EXPECT_EQ(8, reader4.PeekVarInt62Length());
}
-// Test that ReadVarIntU32 works properly. Tests a valid stream count
-// (a 32 bit number) and an invalid one (a >32 bit number)
-TEST_P(QuicDataWriterTest, ValidU32) {
+TEST_P(QuicDataWriterTest, ValidStreamCount) {
char buffer[1024];
memset(buffer, 0, sizeof(buffer));
QuicDataWriter writer(sizeof(buffer), static_cast<char*>(buffer),
@@ -1178,23 +1170,12 @@
const QuicStreamCount write_stream_count = 0xffeeddcc;
EXPECT_TRUE(writer.WriteVarInt62(write_stream_count));
QuicStreamCount read_stream_count;
- EXPECT_TRUE(reader.ReadVarIntU32(&read_stream_count));
+ uint64_t temp;
+ EXPECT_TRUE(reader.ReadVarInt62(&temp));
+ read_stream_count = static_cast<QuicStreamId>(temp);
EXPECT_EQ(write_stream_count, read_stream_count);
}
-TEST_P(QuicDataWriterTest, InvalidU32) {
- char buffer[1024];
- memset(buffer, 0, sizeof(buffer));
- QuicDataWriter writer(sizeof(buffer), static_cast<char*>(buffer),
- quiche::Endianness::NETWORK_BYTE_ORDER);
- QuicDataReader reader(buffer, sizeof(buffer));
- EXPECT_TRUE(writer.WriteVarInt62(UINT64_C(0x1ffeeddcc)));
- QuicStreamCount read_stream_count = 123456;
- EXPECT_FALSE(reader.ReadVarIntU32(&read_stream_count));
- // If the value is bad, read_stream_count ought not change.
- EXPECT_EQ(123456u, read_stream_count);
-}
-
TEST_P(QuicDataWriterTest, Seek) {
char buffer[3] = {};
QuicDataWriter writer(QUICHE_ARRAYSIZE(buffer), buffer,
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc
index 725e947..d41b6b1 100644
--- a/quic/core/quic_framer.cc
+++ b/quic/core/quic_framer.cc
@@ -3441,8 +3441,7 @@
uint8_t frame_type,
QuicStreamFrame* frame) {
// Read stream id from the frame. It's always present.
- if (!reader->ReadVarIntU32(&frame->stream_id)) {
- set_detailed_error("Unable to read stream_id.");
+ if (!ReadUint32FromVarint62(reader, IETF_STREAM, &frame->stream_id)) {
return false;
}
@@ -5776,8 +5775,7 @@
// Get Stream ID from frame. ReadVarIntStreamID returns false
// if either A) there is a read error or B) the resulting value of
// the Stream ID is larger than the maximum allowed value.
- if (!reader->ReadVarIntU32(&frame->stream_id)) {
- set_detailed_error("Unable to read rst stream stream id.");
+ if (!ReadUint32FromVarint62(reader, IETF_RST_STREAM, &frame->stream_id)) {
return false;
}
@@ -5804,8 +5802,8 @@
bool QuicFramer::ProcessStopSendingFrame(
QuicDataReader* reader,
QuicStopSendingFrame* stop_sending_frame) {
- if (!reader->ReadVarIntU32(&stop_sending_frame->stream_id)) {
- set_detailed_error("Unable to read stop sending stream id.");
+ if (!ReadUint32FromVarint62(reader, IETF_STOP_SENDING,
+ &stop_sending_frame->stream_id)) {
return false;
}
@@ -5877,8 +5875,8 @@
bool QuicFramer::ProcessMaxStreamDataFrame(QuicDataReader* reader,
QuicWindowUpdateFrame* frame) {
- if (!reader->ReadVarIntU32(&frame->stream_id)) {
- set_detailed_error("Can not read MAX_STREAM_DATA stream id");
+ if (!ReadUint32FromVarint62(reader, IETF_MAX_STREAM_DATA,
+ &frame->stream_id)) {
return false;
}
if (!reader->ReadVarInt62(&frame->max_data)) {
@@ -5900,8 +5898,9 @@
bool QuicFramer::ProcessMaxStreamsFrame(QuicDataReader* reader,
QuicMaxStreamsFrame* frame,
uint64_t frame_type) {
- if (!reader->ReadVarIntU32(&frame->stream_count)) {
- set_detailed_error("Can not read MAX_STREAMS stream count.");
+ if (!ReadUint32FromVarint62(reader,
+ static_cast<QuicIetfFrameType>(frame_type),
+ &frame->stream_count)) {
return false;
}
frame->unidirectional = (frame_type == IETF_MAX_STREAMS_UNIDIRECTIONAL);
@@ -5943,8 +5942,8 @@
bool QuicFramer::ProcessStreamDataBlockedFrame(QuicDataReader* reader,
QuicBlockedFrame* frame) {
- if (!reader->ReadVarIntU32(&frame->stream_id)) {
- set_detailed_error("Can not read stream blocked stream id.");
+ if (!ReadUint32FromVarint62(reader, IETF_STREAM_DATA_BLOCKED,
+ &frame->stream_id)) {
return false;
}
if (!reader->ReadVarInt62(&frame->offset)) {
@@ -5966,8 +5965,9 @@
bool QuicFramer::ProcessStreamsBlockedFrame(QuicDataReader* reader,
QuicStreamsBlockedFrame* frame,
uint64_t frame_type) {
- if (!reader->ReadVarIntU32(&frame->stream_count)) {
- set_detailed_error("Can not read STREAMS_BLOCKED stream count.");
+ if (!ReadUint32FromVarint62(reader,
+ static_cast<QuicIetfFrameType>(frame_type),
+ &frame->stream_count)) {
return false;
}
if (frame->stream_count > QuicUtils::GetMaxStreamCount()) {
@@ -6064,6 +6064,24 @@
return true;
}
+bool QuicFramer::ReadUint32FromVarint62(QuicDataReader* reader,
+ QuicIetfFrameType type,
+ QuicStreamId* id) {
+ uint64_t temp_uint64;
+ if (!reader->ReadVarInt62(&temp_uint64)) {
+ set_detailed_error("Unable to read " + QuicIetfFrameTypeString(type) +
+ " frame stream id/count.");
+ return false;
+ }
+ if (temp_uint64 > kMaxQuicStreamId) {
+ set_detailed_error("Stream id/count of " + QuicIetfFrameTypeString(type) +
+ "frame is too large.");
+ return false;
+ }
+ *id = static_cast<uint32_t>(temp_uint64);
+ return true;
+}
+
uint8_t QuicFramer::GetStreamFrameTypeByte(const QuicStreamFrame& frame,
bool last_frame_in_packet) const {
if (VersionHasIetfQuicFrames(version_.transport_version)) {
diff --git a/quic/core/quic_framer.h b/quic/core/quic_framer.h
index a6d480b..746e6a1 100644
--- a/quic/core/quic_framer.h
+++ b/quic/core/quic_framer.h
@@ -996,6 +996,11 @@
void set_detailed_error(const char* error) { detailed_error_ = error; }
void set_detailed_error(std::string error) { detailed_error_ = error; }
+ // Returns false if the reading fails.
+ bool ReadUint32FromVarint62(QuicDataReader* reader,
+ QuicIetfFrameType type,
+ QuicStreamId* id);
+
std::string detailed_error_;
QuicFramerVisitorInterface* visitor_;
QuicErrorCode error_;
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc
index e23a454..049b81e 100644
--- a/quic/core/quic_framer_test.cc
+++ b/quic/core/quic_framer_test.cc
@@ -37,7 +37,6 @@
using testing::_;
using testing::Return;
-using testing::Truly;
namespace quic {
namespace test {
@@ -2223,7 +2222,7 @@
{"",
{ 0x08 | 0x01 | 0x02 | 0x04 }},
// stream id
- {"Unable to read stream_id.",
+ {"Unable to read IETF_STREAM frame stream id/count.",
{kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}},
// offset
{"Unable to read stream data offset.",
@@ -2288,7 +2287,7 @@
{"",
{ 0x08 | 0x01 | 0x02 | 0x04 }},
// stream id
- {"Unable to read stream_id.",
+ {"Unable to read IETF_STREAM frame stream id/count.",
{kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}},
// offset
{"Unable to read stream data offset.",
@@ -2546,7 +2545,7 @@
{"",
{0x08 | 0x01 | 0x02 | 0x04}},
// stream id
- {"Unable to read stream_id.",
+ {"Unable to read IETF_STREAM frame stream id/count.",
{kVarInt62TwoBytes + 0x03, 0x04}},
// offset
{"Unable to read stream data offset.",
@@ -2666,7 +2665,7 @@
{"",
{0x08 | 0x01 | 0x02 | 0x04}},
// stream id
- {"Unable to read stream_id.",
+ {"Unable to read IETF_STREAM frame stream id/count.",
{kVarInt62OneByte + 0x04}},
// offset
{"Unable to read stream data offset.",
@@ -4303,7 +4302,7 @@
{"",
{0x04}},
// stream id
- {"Unable to read rst stream stream id.",
+ {"Unable to read IETF_RST_STREAM frame stream id/count.",
{kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}},
// application error code
{"Unable to read rst stream error code.",
@@ -4947,7 +4946,7 @@
{"",
{0x11}},
// stream id
- {"Can not read MAX_STREAM_DATA stream id",
+ {"Unable to read IETF_MAX_STREAM_DATA frame stream id/count.",
{kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}},
// byte offset
{"Can not read MAX_STREAM_DATA byte-count",
@@ -5025,7 +5024,7 @@
{"",
{0x15}},
// stream id
- {"Can not read stream blocked stream id.",
+ {"Unable to read IETF_STREAM_DATA_BLOCKED frame stream id/count.",
{kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}},
// Offset
{"Can not read stream blocked offset.",
@@ -9636,7 +9635,7 @@
{"",
{0x15}},
// blocked offset
- {"Can not read stream blocked stream id.",
+ {"Unable to read IETF_STREAM_DATA_BLOCKED frame stream id/count.",
{kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}},
{"Can not read stream blocked offset.",
{kVarInt62EightBytes + 0x3a, 0x98, 0xFE, 0xDC, 0x32, 0x10, 0x76, 0x54}},
@@ -9724,7 +9723,7 @@
{"",
{0x12}},
// max. streams
- {"Can not read MAX_STREAMS stream count.",
+ {"Unable to read IETF_MAX_STREAMS_BIDIRECTIONAL frame stream id/count.",
{kVarInt62OneByte + 0x03}},
};
// clang-format on
@@ -9764,7 +9763,7 @@
{"",
{0x13}},
// max. streams
- {"Can not read MAX_STREAMS stream count.",
+ {"Unable to read IETF_MAX_STREAMS_UNIDIRECTIONAL frame stream id/count.",
{kVarInt62OneByte + 0x03}},
};
// clang-format on
@@ -9807,7 +9806,7 @@
{"",
{0x13}},
// max. streams
- {"Can not read MAX_STREAMS stream count.",
+ {"Unable to read IETF_MAX_STREAMS_UNIDIRECTIONAL frame stream id/count.",
{kVarInt62OneByte + 0x03}},
};
// clang-format on
@@ -9847,7 +9846,7 @@
{"",
{0x13}},
// max. streams
- {"Can not read MAX_STREAMS stream count.",
+ {"Unable to read IETF_MAX_STREAMS_UNIDIRECTIONAL frame stream id/count.",
{kVarInt62OneByte + 0x03}},
};
// clang-format on
@@ -10079,7 +10078,7 @@
{"",
{0x13}},
// stream count
- {"Can not read MAX_STREAMS stream count.",
+ {"Unable to read IETF_MAX_STREAMS_UNIDIRECTIONAL frame stream id/count.",
{kVarInt62OneByte + 0x00}},
};
// clang-format on
@@ -10122,7 +10121,8 @@
{"",
{0x16}},
// stream id
- {"Can not read STREAMS_BLOCKED stream count.",
+ {"Unable to read IETF_STREAMS_BLOCKED_BIDIRECTIONAL "
+ "frame stream id/count.",
{kVarInt62OneByte + 0x03}},
};
// clang-format on
@@ -10165,7 +10165,8 @@
{"",
{0x17}},
// stream id
- {"Can not read STREAMS_BLOCKED stream count.",
+ {"Unable to read IETF_STREAMS_BLOCKED_UNIDIRECTIONAL "
+ "frame stream id/count.",
{kVarInt62OneByte + 0x03}},
};
// clang-format on
@@ -10205,7 +10206,8 @@
{"",
{0x17}},
// stream id
- {"Can not read STREAMS_BLOCKED stream count.",
+ {"Unable to read IETF_STREAMS_BLOCKED_UNIDIRECTIONAL "
+ "frame stream id/count.",
{kVarInt62OneByte + 0x03}},
};
// clang-format on
@@ -10288,7 +10290,8 @@
{"",
{0x17}},
// stream id
- {"Can not read STREAMS_BLOCKED stream count.",
+ {"Unable to read IETF_STREAMS_BLOCKED_UNIDIRECTIONAL "
+ "frame stream id/count.",
{kVarInt62OneByte + 0x00}},
};
// clang-format on
@@ -10865,7 +10868,7 @@
{"",
{0x05}},
// stream id
- {"Unable to read stop sending stream id.",
+ {"Unable to read IETF_STOP_SENDING frame stream id/count.",
{kVarInt62FourBytes + 0x01, 0x02, 0x03, 0x04}},
{"Unable to read stop sending application error code.",
{kVarInt62FourBytes + 0x00, 0x00, 0x76, 0x54}},
