Report the correct error when parameter length mismatches, and pipe error type code through the ParsingError() functions.
PiperOrigin-RevId: 601568034
diff --git a/quiche/quic/moqt/moqt_parser.cc b/quiche/quic/moqt/moqt_parser.cc
index cde099b..2b9e15f 100644
--- a/quiche/quic/moqt/moqt_parser.cc
+++ b/quiche/quic/moqt/moqt_parser.cc
@@ -92,7 +92,8 @@
size_t message_len = ProcessMessage(reader->PeekRemainingPayload(), fin);
if (message_len == 0) {
if (reader->BytesRemaining() > kMaxMessageHeaderSize) {
- ParseError("Cannot parse non-OBJECT messages > 2KB");
+ ParseError(MoqtError::kGenericError,
+ "Cannot parse non-OBJECT messages > 2KB");
return;
}
if (fin) {
@@ -537,12 +538,16 @@
}
void MoqtParser::ParseError(absl::string_view reason) {
+ ParseError(MoqtError::kProtocolViolation, reason);
+}
+
+void MoqtParser::ParseError(MoqtError error_code, absl::string_view reason) {
if (parsing_error_) {
return; // Don't send multiple parse errors.
}
no_more_data_ = true;
parsing_error_ = true;
- visitor_.OnParsingError(reason);
+ visitor_.OnParsingError(error_code, reason);
}
bool MoqtParser::ReadVarIntPieceVarInt62(quic::QuicDataReader& reader,
@@ -604,7 +609,8 @@
bool MoqtParser::StringViewToVarInt(absl::string_view& sv, uint64_t& vi) {
quic::QuicDataReader reader(sv);
if (static_cast<size_t>(reader.PeekVarInt62Length()) != sv.length()) {
- ParseError("Parameter length does not match varint encoding");
+ ParseError(MoqtError::kParameterLengthMismatch,
+ "Parameter length does not match varint encoding");
return false;
}
reader.ReadVarInt62(&vi);
diff --git a/quiche/quic/moqt/moqt_parser.h b/quiche/quic/moqt/moqt_parser.h
index 9dd63b5..2674885 100644
--- a/quiche/quic/moqt/moqt_parser.h
+++ b/quiche/quic/moqt/moqt_parser.h
@@ -47,7 +47,7 @@
virtual void OnUnannounceMessage(const MoqtUnannounce& message) = 0;
virtual void OnGoAwayMessage(const MoqtGoAway& message) = 0;
- virtual void OnParsingError(absl::string_view reason) = 0;
+ virtual void OnParsingError(MoqtError code, absl::string_view reason) = 0;
};
class QUICHE_EXPORT MoqtParser {
@@ -90,7 +90,9 @@
size_t ProcessUnannounce(quic::QuicDataReader& reader);
size_t ProcessGoAway(quic::QuicDataReader& reader);
+ // If |error| is not provided, assumes kProtocolViolation.
void ParseError(absl::string_view reason);
+ void ParseError(MoqtError error, absl::string_view reason);
// Reads an integer whose length is specified by a preceding VarInt62 and
// returns it in |result|. Returns false if parsing fails.
diff --git a/quiche/quic/moqt/moqt_parser_test.cc b/quiche/quic/moqt/moqt_parser_test.cc
index 32643d4..d5d8f45 100644
--- a/quiche/quic/moqt/moqt_parser_test.cc
+++ b/quiche/quic/moqt/moqt_parser_test.cc
@@ -225,15 +225,17 @@
goaway.new_session_uri = absl::string_view(string0_);
last_message_ = TestMessageBase::MessageStructuredData(goaway);
}
- void OnParsingError(absl::string_view reason) override {
+ void OnParsingError(MoqtError code, absl::string_view reason) override {
QUIC_LOG(INFO) << "Parsing error: " << reason;
parsing_error_ = reason;
+ parsing_error_code_ = code;
}
std::optional<absl::string_view> object_payload_;
bool end_of_message_ = false;
bool got_goaway_ = false;
std::optional<absl::string_view> parsing_error_;
+ MoqtError parsing_error_code_;
uint64_t messages_received_ = 0;
std::optional<TestMessageBase::MessageStructuredData> last_message_;
// Stored strings for last_message_. The visitor API does not promise the
@@ -409,6 +411,7 @@
EXPECT_EQ(visitor_.messages_received_, 0);
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_, "End of stream before complete message");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
// Tests for message-specific error cases, and behaviors for a single message
@@ -517,6 +520,7 @@
EXPECT_EQ(visitor_.messages_received_, 0);
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_, "ROLE parameter appears twice in SETUP");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, SetupRoleIsMissing) {
@@ -531,6 +535,25 @@
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_,
"ROLE parameter missing from CLIENT_SETUP message");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
+}
+
+TEST_F(MoqtMessageSpecificTest, SetupRoleVarintLengthIsWrong) {
+ MoqtParser parser(kRawQuic, visitor_);
+ char setup[] = {
+ 0x40, 0x40, // type
+ 0x02, 0x01, 0x02, // versions
+ 0x02, // 2 parameters
+ 0x00, 0x02, 0x03, // role = both, but length is 2
+ 0x01, 0x03, 0x66, 0x6f, 0x6f // path = "foo"
+ };
+ parser.ProcessData(absl::string_view(setup, sizeof(setup)), false);
+ EXPECT_EQ(visitor_.messages_received_, 0);
+ EXPECT_TRUE(visitor_.parsing_error_.has_value());
+ EXPECT_EQ(*visitor_.parsing_error_,
+ "Parameter length does not match varint encoding");
+
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kParameterLengthMismatch);
}
TEST_F(MoqtMessageSpecificTest, SetupPathFromServer) {
@@ -545,6 +568,7 @@
EXPECT_EQ(visitor_.messages_received_, 0);
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_, "PATH parameter in SERVER_SETUP");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, SetupPathAppearsTwice) {
@@ -561,6 +585,7 @@
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_,
"PATH parameter appears twice in CLIENT_SETUP");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, SetupPathOverWebtrans) {
@@ -576,6 +601,7 @@
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_,
"WebTransport connection is using PATH parameter in SETUP");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, SetupPathMissing) {
@@ -590,6 +616,7 @@
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_,
"PATH SETUP parameter missing from Client message over QUIC");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, SubscribeRequestAuthorizationInfoTwice) {
@@ -611,6 +638,7 @@
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_,
"AUTHORIZATION_INFO parameter appears twice in SUBSCRIBE_REQUEST");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, AnnounceAuthorizationInfoTwice) {
@@ -626,6 +654,7 @@
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_,
"AUTHORIZATION_INFO parameter appears twice in ANNOUNCE");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, FinMidPayload) {
@@ -637,6 +666,7 @@
EXPECT_EQ(visitor_.messages_received_, 1);
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_, "Received FIN mid-payload");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, PartialPayloadThenFin) {
@@ -650,6 +680,7 @@
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_,
"End of stream before complete OBJECT PAYLOAD");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, DataAfterFin) {
@@ -658,6 +689,7 @@
parser.ProcessData("foo", false);
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_, "Data after end of stream");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kProtocolViolation);
}
TEST_F(MoqtMessageSpecificTest, Setup2KB) {
@@ -676,6 +708,7 @@
EXPECT_EQ(visitor_.messages_received_, 0);
EXPECT_TRUE(visitor_.parsing_error_.has_value());
EXPECT_EQ(*visitor_.parsing_error_, "Cannot parse non-OBJECT messages > 2KB");
+ EXPECT_EQ(visitor_.parsing_error_code_, MoqtError::kGenericError);
}
TEST_F(MoqtMessageSpecificTest, UnknownMessageType) {
diff --git a/quiche/quic/moqt/moqt_session.cc b/quiche/quic/moqt/moqt_session.cc
index c79e515..7adb7ee 100644
--- a/quiche/quic/moqt/moqt_session.cc
+++ b/quiche/quic/moqt/moqt_session.cc
@@ -648,9 +648,9 @@
session_->pending_outgoing_announces_.erase(it);
}
-void MoqtSession::Stream::OnParsingError(absl::string_view reason) {
- session_->Error(MoqtError::kProtocolViolation,
- absl::StrCat("Parse error: ", reason));
+void MoqtSession::Stream::OnParsingError(MoqtError error_code,
+ absl::string_view reason) {
+ session_->Error(error_code, absl::StrCat("Parse error: ", reason));
}
bool MoqtSession::Stream::CheckIfIsControlStream() {
diff --git a/quiche/quic/moqt/moqt_session.h b/quiche/quic/moqt/moqt_session.h
index 6620b3c..01f6fcf 100644
--- a/quiche/quic/moqt/moqt_session.h
+++ b/quiche/quic/moqt/moqt_session.h
@@ -161,7 +161,8 @@
void OnAnnounceErrorMessage(const MoqtAnnounceError& message) override;
void OnUnannounceMessage(const MoqtUnannounce& /*message*/) override {}
void OnGoAwayMessage(const MoqtGoAway& /*message*/) override {}
- void OnParsingError(absl::string_view reason) override;
+ void OnParsingError(MoqtError error_code,
+ absl::string_view reason) override;
quic::Perspective perspective() const {
return session_->parameters_.perspective;
