From 45eae346930aedbf4624d054e0633c94b397b8ec Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Mon, 17 Apr 2023 17:46:59 +0000 Subject: [PATCH] Revert "dcsctp: Support zero checksum packets" This reverts commit bd46bb7660fbc9f31799196058f7a1957f50aa31. Reason for revert: There is a slight performance degradation pointing to this CL, so revert this to be able to confirm if it is the culprit. Original change's description: > dcsctp: Support zero checksum packets > > If configured, the packet parser will allow packets with > a set checksum of zero. In that case, the correct checksum > will not even be calculated, avoiding a CPU intensive > calculation. > > Also, if specified when building a packet, the checksum can > be opted to be not calculated and written to the packet. > This is to be used when draft-tuexen-tsvwg-sctp-zero-checksum > has been negotiated, except for some packets during association > establishment. > > This is mainly a preparation CL and follow-up CL will enable > this feature. > > Low-Coverage-Reason: Affects debug logging code not run in tests > Bug: webrtc:14997 > Change-Id: I3207ac3a626df039ee2990403c2edd6429f17617 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/298481 > Reviewed-by: Harald Alvestrand > Commit-Queue: Victor Boivie > Cr-Commit-Position: refs/heads/main@{#39737} Bug: webrtc:14997 Change-Id: Ie22267abb4bcd25d5af07875eb933c51ed5be853 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301580 Reviewed-by: Mirko Bonadei Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#39878} --- net/dcsctp/packet/sctp_packet.cc | 44 +++---- net/dcsctp/packet/sctp_packet.h | 9 +- net/dcsctp/packet/sctp_packet_test.cc | 116 ++---------------- net/dcsctp/public/dcsctp_options.h | 11 +- net/dcsctp/socket/dcsctp_socket.cc | 5 +- net/dcsctp/socket/dcsctp_socket_test.cc | 97 +++++++-------- net/dcsctp/socket/heartbeat_handler_test.cc | 13 +- .../socket/stream_reset_handler_test.cc | 5 +- 8 files changed, 81 insertions(+), 219 deletions(-) diff --git a/net/dcsctp/packet/sctp_packet.cc b/net/dcsctp/packet/sctp_packet.cc index de407bb4c7..cc66235122 100644 --- a/net/dcsctp/packet/sctp_packet.cc +++ b/net/dcsctp/packet/sctp_packet.cc @@ -62,7 +62,7 @@ SctpPacket::Builder& SctpPacket::Builder::Add(const Chunk& chunk) { buffer.Store16<0>(source_port_); buffer.Store16<2>(dest_port_); buffer.Store32<4>(*verification_tag_); - // Checksum is at offset 8 - written when calling Build(), if configured. + // Checksum is at offset 8 - written when calling Build(); } RTC_DCHECK(IsDivisibleBy4(out_.size())); @@ -89,11 +89,11 @@ size_t SctpPacket::Builder::bytes_remaining() const { return max_packet_size_ - out_.size(); } -std::vector SctpPacket::Builder::Build(bool write_checksum) { +std::vector SctpPacket::Builder::Build() { std::vector out; out_.swap(out); - if (!out.empty() && write_checksum) { + if (!out.empty()) { uint32_t crc = GenerateCrc32C(out); BoundedByteWriter(out).Store32<8>(crc); } @@ -105,8 +105,9 @@ std::vector SctpPacket::Builder::Build(bool write_checksum) { return out; } -absl::optional SctpPacket::Parse(rtc::ArrayView data, - const DcSctpOptions& options) { +absl::optional SctpPacket::Parse( + rtc::ArrayView data, + bool disable_checksum_verification) { if (data.size() < kHeaderSize + kChunkTlvHeaderSize || data.size() > kMaxUdpPacketSize) { RTC_DLOG(LS_WARNING) << "Invalid packet size"; @@ -125,28 +126,19 @@ absl::optional SctpPacket::Parse(rtc::ArrayView data, std::vector data_copy = std::vector(data.begin(), data.end()); - if (options.disable_checksum_verification || - (options.enable_zero_checksum && common_header.checksum == 0u)) { - // https://www.ietf.org/archive/id/draft-tuexen-tsvwg-sctp-zero-checksum-01.html#section-4.3: - // If an end point has sent the Zero Checksum Acceptable Chunk Parameter in - // an INIT or INIT ACK chunk, it MUST accept SCTP packets using an incorrect - // checksum value of zero in addition to SCTP packets containing the correct - // CRC32c checksum value for this association. - } else { - // Verify the checksum. The checksum field must be zero when that's done. - BoundedByteWriter(data_copy).Store32<8>(0); - uint32_t calculated_checksum = GenerateCrc32C(data_copy); - if (calculated_checksum != common_header.checksum) { - RTC_DLOG(LS_WARNING) << rtc::StringFormat( - "Invalid packet checksum, packet_checksum=0x%08x, " - "calculated_checksum=0x%08x", - common_header.checksum, calculated_checksum); - return absl::nullopt; - } - // Restore the checksum in the header. - BoundedByteWriter(data_copy).Store32<8>( - common_header.checksum); + // Verify the checksum. The checksum field must be zero when that's done. + BoundedByteWriter(data_copy).Store32<8>(0); + uint32_t calculated_checksum = GenerateCrc32C(data_copy); + if (!disable_checksum_verification && + calculated_checksum != common_header.checksum) { + RTC_DLOG(LS_WARNING) << rtc::StringFormat( + "Invalid packet checksum, packet_checksum=0x%08x, " + "calculated_checksum=0x%08x", + common_header.checksum, calculated_checksum); + return absl::nullopt; } + // Restore the checksum in the header. + BoundedByteWriter(data_copy).Store32<8>(common_header.checksum); // Validate and parse the chunk headers in the message. /* diff --git a/net/dcsctp/packet/sctp_packet.h b/net/dcsctp/packet/sctp_packet.h index 0d348b448f..4c6234e0c9 100644 --- a/net/dcsctp/packet/sctp_packet.h +++ b/net/dcsctp/packet/sctp_packet.h @@ -74,9 +74,7 @@ class SctpPacket { // Returns the payload of the build SCTP packet. The Builder will be cleared // after having called this function, and can be used to build a new packet. - // If `write_checksum` is set to false, a value of zero (0) will be written - // as the packet's checksum, instead of the crc32c value. - std::vector Build(bool write_checksum = true); + std::vector Build(); private: VerificationTag verification_tag_; @@ -89,8 +87,9 @@ class SctpPacket { }; // Parses `data` as an SCTP packet and returns it if it validates. - static absl::optional Parse(rtc::ArrayView data, - const DcSctpOptions& options); + static absl::optional Parse( + rtc::ArrayView data, + bool disable_checksum_verification = false); // Returns the SCTP common header. const CommonHeader& common_header() const { return common_header_; } diff --git a/net/dcsctp/packet/sctp_packet_test.cc b/net/dcsctp/packet/sctp_packet_test.cc index fcdd161c0d..7438315eec 100644 --- a/net/dcsctp/packet/sctp_packet_test.cc +++ b/net/dcsctp/packet/sctp_packet_test.cc @@ -32,13 +32,9 @@ namespace dcsctp { namespace { -using ::testing::ElementsAre; using ::testing::SizeIs; constexpr VerificationTag kVerificationTag = VerificationTag(0x12345678); -constexpr DcSctpOptions kVerifyChecksumOptions = - DcSctpOptions{.disable_checksum_verification = false, - .enable_zero_checksum = false}; TEST(SctpPacketTest, DeserializeSimplePacketFromCapture) { /* @@ -91,8 +87,7 @@ TEST(SctpPacketTest, DeserializeSimplePacketFromCapture) { 0x68, 0x2a, 0xc2, 0x97, 0x80, 0x04, 0x00, 0x06, 0x00, 0x01, 0x00, 0x00, 0x80, 0x03, 0x00, 0x06, 0x80, 0xc1, 0x00, 0x00}; - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, - SctpPacket::Parse(data, kVerifyChecksumOptions)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(data)); EXPECT_EQ(packet.common_header().source_port, 5000); EXPECT_EQ(packet.common_header().destination_port, 5000); EXPECT_EQ(packet.common_header().verification_tag, VerificationTag(0)); @@ -135,8 +130,7 @@ TEST(SctpPacketTest, DeserializePacketWithTwoChunks) { 0x03, 0x00, 0x00, 0x10, 0xae, 0xa9, 0x52, 0x52, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, - SctpPacket::Parse(data, kVerifyChecksumOptions)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(data)); EXPECT_EQ(packet.common_header().source_port, 1234); EXPECT_EQ(packet.common_header().destination_port, 4321); EXPECT_EQ(packet.common_header().verification_tag, @@ -178,7 +172,7 @@ TEST(SctpPacketTest, DeserializePacketWithWrongChecksum) { 0xf5, 0x31, 0x03, 0x00, 0x00, 0x10, 0x55, 0x08, 0x36, 0x40, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; - EXPECT_FALSE(SctpPacket::Parse(data, kVerifyChecksumOptions).has_value()); + EXPECT_FALSE(SctpPacket::Parse(data).has_value()); } TEST(SctpPacketTest, DeserializePacketDontValidateChecksum) { @@ -208,8 +202,7 @@ TEST(SctpPacketTest, DeserializePacketDontValidateChecksum) { ASSERT_HAS_VALUE_AND_ASSIGN( SctpPacket packet, - SctpPacket::Parse(data, {.disable_checksum_verification = true, - .enable_zero_checksum = false})); + SctpPacket::Parse(data, /*disable_checksum_verification=*/true)); EXPECT_EQ(packet.common_header().source_port, 5000); EXPECT_EQ(packet.common_header().destination_port, 5000); EXPECT_EQ(packet.common_header().verification_tag, @@ -227,8 +220,7 @@ TEST(SctpPacketTest, SerializeAndDeserializeSingleChunk) { b.Add(init); std::vector serialized = b.Build(); - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket packet, SctpPacket::Parse(serialized, kVerifyChecksumOptions)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(serialized)); EXPECT_EQ(packet.common_header().verification_tag, kVerificationTag); @@ -258,8 +250,7 @@ TEST(SctpPacketTest, SerializeAndDeserializeThreeChunks) { std::vector serialized = b.Build(); - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket packet, SctpPacket::Parse(serialized, kVerifyChecksumOptions)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(serialized)); EXPECT_EQ(packet.common_header().verification_tag, kVerificationTag); @@ -288,8 +279,7 @@ TEST(SctpPacketTest, ParseAbortWithEmptyCause) { /*filled_in_verification_tag=*/true, Parameters::Builder().Add(UserInitiatedAbortCause("")).Build())); - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket packet, SctpPacket::Parse(b.Build(), kVerifyChecksumOptions)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(b.Build())); EXPECT_EQ(packet.common_header().verification_tag, kVerificationTag); @@ -308,7 +298,7 @@ TEST(SctpPacketTest, DetectPacketWithZeroSizeChunk) { uint8_t data[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0x0a, 0x0a, 0x0a, 0x5c, 0x0a, 0x0a, 0x0a, 0x0a, 0x0a, 0x00, 0x00, 0x00}; - EXPECT_FALSE(SctpPacket::Parse(data, kVerifyChecksumOptions).has_value()); + EXPECT_FALSE(SctpPacket::Parse(data, true).has_value()); } TEST(SctpPacketTest, ReturnsCorrectSpaceAvailableToStayWithinMTU) { @@ -348,95 +338,5 @@ TEST(SctpPacketTest, ReturnsCorrectSpaceAvailableToStayWithinMTU) { EXPECT_EQ(builder.bytes_remaining(), 0u); // Hand-calculated. } -TEST(SctpPacketTest, AcceptsZeroSetZeroChecksum) { - /* - Stream Control Transmission Protocol, Src Port: 5000 (5000), - Dst Port: 5000 (5000) - Source port: 5000 - Destination port: 5000 - Verification tag: 0x0eddca08 - [Association index: 1] - Checksum: 0x00000000 [unverified] - [Checksum Status: Unverified] - SACK chunk (Cumulative TSN: 1426601536, a_rwnd: 131072, - gaps: 0, duplicate TSNs: 0) - Chunk type: SACK (3) - Chunk flags: 0x00 - Chunk length: 16 - Cumulative TSN ACK: 1426601536 - Advertised receiver window credit (a_rwnd): 131072 - Number of gap acknowledgement blocks: 0 - Number of duplicated TSNs: 0 - */ - - uint8_t data[] = {0x13, 0x88, 0x13, 0x88, 0x0e, 0xdd, 0xca, 0x08, 0x00, 0x00, - 0x00, 0x00, 0x03, 0x00, 0x00, 0x10, 0x55, 0x08, 0x36, 0x40, - 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; - - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket packet, - SctpPacket::Parse(data, {.disable_checksum_verification = false, - .enable_zero_checksum = true})); - EXPECT_EQ(packet.common_header().source_port, 5000); - EXPECT_EQ(packet.common_header().destination_port, 5000); - EXPECT_EQ(packet.common_header().verification_tag, - VerificationTag(0x0eddca08u)); - EXPECT_EQ(packet.common_header().checksum, 0x00000000u); -} - -TEST(SctpPacketTest, RejectsNonZeroIncorrectChecksumWhenZeroChecksumIsActive) { - /* - Stream Control Transmission Protocol, Src Port: 5000 (5000), - Dst Port: 5000 (5000) - Source port: 5000 - Destination port: 5000 - Verification tag: 0x0eddca08 - [Association index: 1] - Checksum: 0x00000001 [unverified] - [Checksum Status: Unverified] - SACK chunk (Cumulative TSN: 1426601536, a_rwnd: 131072, - gaps: 0, duplicate TSNs: 0) - Chunk type: SACK (3) - Chunk flags: 0x00 - Chunk length: 16 - Cumulative TSN ACK: 1426601536 - Advertised receiver window credit (a_rwnd): 131072 - Number of gap acknowledgement blocks: 0 - Number of duplicated TSNs: 0 - */ - - uint8_t data[] = {0x13, 0x88, 0x13, 0x88, 0x0e, 0xdd, 0xca, 0x08, 0x01, 0x00, - 0x00, 0x00, 0x03, 0x00, 0x00, 0x10, 0x55, 0x08, 0x36, 0x40, - 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}; - - EXPECT_FALSE(SctpPacket::Parse(data, {.disable_checksum_verification = false, - .enable_zero_checksum = true}) - .has_value()); -} - -TEST(SctpPacketTest, WritePacketWithCalculatedChecksum) { - SctpPacket::Builder b(kVerificationTag, {}); - b.Add(SackChunk(/*cumulative_tsn_ack=*/TSN(999), /*a_rwnd=*/456, - /*gap_ack_blocks=*/{}, - /*duplicate_tsns=*/{})); - EXPECT_THAT(b.Build(), - ElementsAre(0x13, 0x88, 0x13, 0x88, 0x12, 0x34, 0x56, 0x78, // - 0x07, 0xe8, 0x38, 0x77, // checksum - 0x03, 0x00, 0x00, 0x10, 0x00, 0x00, 0x03, 0xe7, 0x00, - 0x00, 0x01, 0xc8, 0x00, 0x00, 0x00, 0x00)); -} - -TEST(SctpPacketTest, WritePacketWithZeroChecksum) { - SctpPacket::Builder b(kVerificationTag, {}); - b.Add(SackChunk(/*cumulative_tsn_ack=*/TSN(999), /*a_rwnd=*/456, - /*gap_ack_blocks=*/{}, - /*duplicate_tsns=*/{})); - EXPECT_THAT(b.Build(/*write_checksum=*/false), - ElementsAre(0x13, 0x88, 0x13, 0x88, 0x12, 0x34, 0x56, 0x78, // - 0x00, 0x00, 0x00, 0x00, // checksum - 0x03, 0x00, 0x00, 0x10, 0x00, 0x00, 0x03, 0xe7, 0x00, - 0x00, 0x01, 0xc8, 0x00, 0x00, 0x00, 0x00)); -} - } // namespace } // namespace dcsctp diff --git a/net/dcsctp/public/dcsctp_options.h b/net/dcsctp/public/dcsctp_options.h index d19798054c..4511bed4a4 100644 --- a/net/dcsctp/public/dcsctp_options.h +++ b/net/dcsctp/public/dcsctp_options.h @@ -193,17 +193,8 @@ struct DcSctpOptions { // If RTO should be added to heartbeat_interval bool heartbeat_interval_include_rtt = true; - // Disables SCTP packet crc32 verification. For fuzzers only! + // Disables SCTP packet crc32 verification. Useful when running with fuzzers. bool disable_checksum_verification = false; - - // Controls the acceptance of zero checksum, as defined in - // https://datatracker.ietf.org/doc/draft-tuexen-tsvwg-sctp-zero-checksum/ - // This should only be enabled if the packet integrity can be ensured by lower - // layers, which DTLS will do in WebRTC, as defined by RFC8261. - // - // This will also enable sending packets without a checksum value (set to 0) - // once both peers have negotiated this feature. - bool enable_zero_checksum = false; }; } // namespace dcsctp diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc index 139ec28055..f831ba090c 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -755,7 +755,8 @@ void DcSctpSocket::ReceivePacket(rtc::ArrayView data) { packet_observer_->OnReceivedPacket(callbacks_.TimeMillis(), data); } - absl::optional packet = SctpPacket::Parse(data, options_); + absl::optional packet = + SctpPacket::Parse(data, options_.disable_checksum_verification); if (!packet.has_value()) { // https://tools.ietf.org/html/rfc4960#section-6.8 // "The default procedure for handling invalid SCTP packets is to @@ -797,7 +798,7 @@ void DcSctpSocket::ReceivePacket(rtc::ArrayView data) { } void DcSctpSocket::DebugPrintOutgoing(rtc::ArrayView payload) { - auto packet = SctpPacket::Parse(payload, options_); + auto packet = SctpPacket::Parse(payload); RTC_DCHECK(packet.has_value()); for (const auto& desc : packet->descriptors()) { diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index 0da893d9bd..f38624d606 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -67,10 +67,9 @@ constexpr SendOptions kSendOptions; constexpr size_t kLargeMessageSize = DcSctpOptions::kMaxSafeMTUSize * 20; constexpr size_t kSmallMessageSize = 10; constexpr int kMaxBurstPackets = 4; -constexpr DcSctpOptions kDefaultOptions; MATCHER_P(HasDataChunkWithStreamId, stream_id, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -97,7 +96,7 @@ MATCHER_P(HasDataChunkWithStreamId, stream_id, "") { } MATCHER_P(HasDataChunkWithPPID, ppid, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -124,7 +123,7 @@ MATCHER_P(HasDataChunkWithPPID, ppid, "") { } MATCHER_P(HasDataChunkWithSsn, ssn, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -151,7 +150,7 @@ MATCHER_P(HasDataChunkWithSsn, ssn, "") { } MATCHER_P(HasDataChunkWithMid, mid, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -178,7 +177,7 @@ MATCHER_P(HasDataChunkWithMid, mid, "") { } MATCHER_P(HasSackWithCumAckTsn, tsn, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -205,7 +204,7 @@ MATCHER_P(HasSackWithCumAckTsn, tsn, "") { } MATCHER(HasSackWithNoGapAckBlocks, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -232,7 +231,7 @@ MATCHER(HasSackWithNoGapAckBlocks, "") { } MATCHER_P(HasReconfigWithStreams, streams_matcher, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -270,7 +269,7 @@ MATCHER_P(HasReconfigWithStreams, streams_matcher, "") { } MATCHER_P(HasReconfigWithResponse, result, "") { - absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); + absl::optional packet = SctpPacket::Parse(arg); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -329,7 +328,7 @@ std::unique_ptr GetPacketObserver(absl::string_view name) { struct SocketUnderTest { explicit SocketUnderTest(absl::string_view name, - const DcSctpOptions& opts = kDefaultOptions) + const DcSctpOptions& opts = {}) : options(FixupOptions(opts)), cb(name), socket(name, cb, GetPacketObserver(name), options) {} @@ -629,9 +628,8 @@ TEST(DcSctpSocketTest, ResendInitAndEstablishConnection) { a.socket.Connect(); // INIT is never received by Z. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket init_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(init_packet.descriptors()[0].type, InitChunk::kType); AdvanceTime(a, z, a.options.t1_init_timeout); @@ -656,18 +654,16 @@ TEST(DcSctpSocketTest, ResendingInitTooManyTimesAborts) { a.socket.Connect(); // INIT is never received by Z. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket init_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(init_packet.descriptors()[0].type, InitChunk::kType); for (int i = 0; i < *a.options.max_init_retransmits; ++i) { AdvanceTime(a, z, a.options.t1_init_timeout * (1 << i)); // INIT is resent - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket resent_init_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket resent_init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(resent_init_packet.descriptors()[0].type, InitChunk::kType); } @@ -691,9 +687,8 @@ TEST(DcSctpSocketTest, ResendCookieEchoAndEstablishConnection) { a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); // COOKIE_ECHO is never received by Z. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket init_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(init_packet.descriptors()[0].type, CookieEchoChunk::kType); AdvanceTime(a, z, a.options.t1_init_timeout); @@ -719,18 +714,16 @@ TEST(DcSctpSocketTest, ResendingCookieEchoTooManyTimesAborts) { a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); // COOKIE_ECHO is never received by Z. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket init_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(init_packet.descriptors()[0].type, CookieEchoChunk::kType); for (int i = 0; i < *a.options.max_init_retransmits; ++i) { AdvanceTime(a, z, a.options.t1_cookie_timeout * (1 << i)); // COOKIE_ECHO is resent - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket resent_init_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket resent_init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(resent_init_packet.descriptors()[0].type, CookieEchoChunk::kType); } @@ -758,9 +751,8 @@ TEST(DcSctpSocketTest, DoesntSendMorePacketsUntilCookieAckHasBeenReceived) { a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); // COOKIE_ECHO is never received by Z. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket cookie_echo_packet1, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket cookie_echo_packet1, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_THAT(cookie_echo_packet1.descriptors(), SizeIs(2)); EXPECT_EQ(cookie_echo_packet1.descriptors()[0].type, CookieEchoChunk::kType); EXPECT_EQ(cookie_echo_packet1.descriptors()[1].type, DataChunk::kType); @@ -779,9 +771,8 @@ TEST(DcSctpSocketTest, DoesntSendMorePacketsUntilCookieAckHasBeenReceived) { AdvanceTime(a, z, a.options.t1_cookie_timeout - a.options.rto_initial); // And this COOKIE-ECHO and DATA is also lost - never received by Z. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket cookie_echo_packet2, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket cookie_echo_packet2, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_THAT(cookie_echo_packet2.descriptors(), SizeIs(2)); EXPECT_EQ(cookie_echo_packet2.descriptors()[0].type, CookieEchoChunk::kType); EXPECT_EQ(cookie_echo_packet2.descriptors()[1].type, DataChunk::kType); @@ -845,9 +836,8 @@ TEST(DcSctpSocketTest, ShutdownTimerExpiresTooManyTimeClosesConnection) { AdvanceTime(a, z, DurationMs(a.options.rto_initial * (1 << i))); // Dropping every shutdown chunk. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(packet.descriptors()[0].type, ShutdownChunk::kType); EXPECT_TRUE(a.cb.ConsumeSentPacket().empty()); } @@ -857,9 +847,8 @@ TEST(DcSctpSocketTest, ShutdownTimerExpiresTooManyTimeClosesConnection) { a.options.rto_initial * (1 << *a.options.max_retransmissions)); EXPECT_EQ(a.socket.state(), SocketState::kClosed); - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(packet.descriptors()[0].type, AbortChunk::kType); EXPECT_TRUE(a.cb.ConsumeSentPacket().empty()); } @@ -967,9 +956,8 @@ TEST_P(DcSctpSocketParametrizedTest, SendingHeartbeatAnswersWithAck) { a.socket.ReceivePacket(b.Build()); // HEARTBEAT_ACK is sent as a reply. Capture it. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket ack_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket ack_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); ASSERT_THAT(ack_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( HeartbeatAckChunk ack, @@ -993,7 +981,7 @@ TEST_P(DcSctpSocketParametrizedTest, ExpectHeartbeatToBeSent) { std::vector hb_packet_raw = a.cb.ConsumeSentPacket(); ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket hb_packet, - SctpPacket::Parse(hb_packet_raw, z->options)); + SctpPacket::Parse(hb_packet_raw)); ASSERT_THAT(hb_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( HeartbeatRequestChunk hb, @@ -1030,9 +1018,8 @@ TEST_P(DcSctpSocketParametrizedTest, AdvanceTime(a, *z, time_to_next_hearbeat); // Dropping every heartbeat. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket hb_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket hb_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(hb_packet.descriptors()[0].type, HeartbeatRequestChunk::kType); RTC_LOG(LS_INFO) << "Letting the heartbeat expire."; @@ -1085,7 +1072,7 @@ TEST_P(DcSctpSocketParametrizedTest, RecoversAfterASuccessfulAck) { std::vector hb_packet_raw = a.cb.ConsumeSentPacket(); ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket hb_packet, - SctpPacket::Parse(hb_packet_raw, z->options)); + SctpPacket::Parse(hb_packet_raw)); ASSERT_THAT(hb_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( HeartbeatRequestChunk hb, @@ -1105,9 +1092,8 @@ TEST_P(DcSctpSocketParametrizedTest, RecoversAfterASuccessfulAck) { RTC_LOG(LS_INFO) << "Expecting a new heartbeat"; AdvanceTime(a, *z, time_to_next_hearbeat); - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket another_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket another_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); EXPECT_EQ(another_packet.descriptors()[0].type, HeartbeatRequestChunk::kType); } @@ -1483,9 +1469,8 @@ TEST_P(DcSctpSocketParametrizedTest, ReceivingUnknownChunkRespondsWithError) { a.socket.ReceivePacket(b.Build()); // ERROR is sent as a reply. Capture it. - ASSERT_HAS_VALUE_AND_ASSIGN( - SctpPacket reply_packet, - SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket reply_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket())); ASSERT_THAT(reply_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( ErrorChunk error, ErrorChunk::Parse(reply_packet.descriptors()[0].data)); @@ -1532,7 +1517,7 @@ TEST(DcSctpSocketTest, PassingHighWatermarkWillOnlyAcceptCumAckTsn) { a.socket.Connect(); std::vector init_data = a.cb.ConsumeSentPacket(); ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, - SctpPacket::Parse(init_data, z.options)); + SctpPacket::Parse(init_data)); ASSERT_HAS_VALUE_AND_ASSIGN( InitChunk init_chunk, InitChunk::Parse(init_packet.descriptors()[0].data)); @@ -2252,7 +2237,7 @@ TEST(DcSctpSocketTest, ReceiveBothUnorderedAndOrderedWithSameTSN) { a.socket.Connect(); std::vector init_data = a.cb.ConsumeSentPacket(); ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, - SctpPacket::Parse(init_data, z.options)); + SctpPacket::Parse(init_data)); ASSERT_HAS_VALUE_AND_ASSIGN( InitChunk init_chunk, InitChunk::Parse(init_packet.descriptors()[0].data)); diff --git a/net/dcsctp/socket/heartbeat_handler_test.cc b/net/dcsctp/socket/heartbeat_handler_test.cc index d573192440..faa0e3da06 100644 --- a/net/dcsctp/socket/heartbeat_handler_test.cc +++ b/net/dcsctp/socket/heartbeat_handler_test.cc @@ -37,7 +37,6 @@ DcSctpOptions MakeOptions(DurationMs heartbeat_interval) { DcSctpOptions options; options.heartbeat_interval_include_rtt = false; options.heartbeat_interval = heartbeat_interval; - options.enable_zero_checksum = false; return options; } @@ -84,8 +83,7 @@ TEST_F(HeartbeatHandlerTest, HasRunningHeartbeatIntervalTimer) { // Validate that a heartbeat request was sent. std::vector payload = callbacks_.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, - SctpPacket::Parse(payload, options_)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( @@ -103,8 +101,7 @@ TEST_F(HeartbeatHandlerTest, RepliesToHeartbeatRequests) { handler_.HandleHeartbeatRequest(std::move(request)); std::vector payload = callbacks_.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, - SctpPacket::Parse(payload, options_)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( @@ -123,8 +120,7 @@ TEST_F(HeartbeatHandlerTest, SendsHeartbeatRequestsOnIdleChannel) { // Grab the request, and make a response. std::vector payload = callbacks_.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, - SctpPacket::Parse(payload, options_)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( @@ -147,8 +143,7 @@ TEST_F(HeartbeatHandlerTest, DoesntObserveInvalidHeartbeats) { // Grab the request, and make a response. std::vector payload = callbacks_.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, - SctpPacket::Parse(payload, options_)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( diff --git a/net/dcsctp/socket/stream_reset_handler_test.cc b/net/dcsctp/socket/stream_reset_handler_test.cc index 503cc89094..d3fa98752c 100644 --- a/net/dcsctp/socket/stream_reset_handler_test.cc +++ b/net/dcsctp/socket/stream_reset_handler_test.cc @@ -149,7 +149,7 @@ class StreamResetHandlerTest : public testing::Test { } std::vector responses; - absl::optional p = SctpPacket::Parse(payload, DcSctpOptions()); + absl::optional p = SctpPacket::Parse(payload); if (!p.has_value()) { EXPECT_TRUE(false); return {}; @@ -504,8 +504,7 @@ TEST_F(StreamResetHandlerTest, SendOutgoingResetRetransmitOnInProgress) { std::vector payload = callbacks_.ConsumeSentPacket(); ASSERT_FALSE(payload.empty()); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, - SctpPacket::Parse(payload, DcSctpOptions())); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( ReConfigChunk reconfig2,