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;