From 3c6b46fc16b8e595d5aa54f0ba1fa15aa527134c Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Thu, 20 Apr 2023 15:44:59 +0000 Subject: [PATCH] Reland "dcsctp: Support zero checksum packets" This reverts commit 45eae346930aedbf4624d054e0633c94b397b8ec. It was found not to be the root cause of the performance regression, so it's safe to reland. Bug: webrtc:14997 Change-Id: I67c90752875bf4071cbdd5adfa462a37f4d4ceab Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/302162 Commit-Queue: Victor Boivie Reviewed-by: Mirko Bonadei Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Cr-Commit-Position: refs/heads/main@{#39910} --- 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, 219 insertions(+), 81 deletions(-) diff --git a/net/dcsctp/packet/sctp_packet.cc b/net/dcsctp/packet/sctp_packet.cc index cc66235122..de407bb4c7 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(); + // Checksum is at offset 8 - written when calling Build(), if configured. } 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() { +std::vector SctpPacket::Builder::Build(bool write_checksum) { std::vector out; out_.swap(out); - if (!out.empty()) { + if (!out.empty() && write_checksum) { uint32_t crc = GenerateCrc32C(out); BoundedByteWriter(out).Store32<8>(crc); } @@ -105,9 +105,8 @@ std::vector SctpPacket::Builder::Build() { return out; } -absl::optional SctpPacket::Parse( - rtc::ArrayView data, - bool disable_checksum_verification) { +absl::optional SctpPacket::Parse(rtc::ArrayView data, + const DcSctpOptions& options) { if (data.size() < kHeaderSize + kChunkTlvHeaderSize || data.size() > kMaxUdpPacketSize) { RTC_DLOG(LS_WARNING) << "Invalid packet size"; @@ -126,19 +125,28 @@ absl::optional SctpPacket::Parse( std::vector data_copy = std::vector(data.begin(), data.end()); - // 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; + 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); } - // 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 4c6234e0c9..0d348b448f 100644 --- a/net/dcsctp/packet/sctp_packet.h +++ b/net/dcsctp/packet/sctp_packet.h @@ -74,7 +74,9 @@ 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. - std::vector Build(); + // 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); private: VerificationTag verification_tag_; @@ -87,9 +89,8 @@ class SctpPacket { }; // Parses `data` as an SCTP packet and returns it if it validates. - static absl::optional Parse( - rtc::ArrayView data, - bool disable_checksum_verification = false); + static absl::optional Parse(rtc::ArrayView data, + const DcSctpOptions& options); // 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 7438315eec..fcdd161c0d 100644 --- a/net/dcsctp/packet/sctp_packet_test.cc +++ b/net/dcsctp/packet/sctp_packet_test.cc @@ -32,9 +32,13 @@ 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) { /* @@ -87,7 +91,8 @@ 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)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(data, kVerifyChecksumOptions)); 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)); @@ -130,7 +135,8 @@ 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)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(data, kVerifyChecksumOptions)); EXPECT_EQ(packet.common_header().source_port, 1234); EXPECT_EQ(packet.common_header().destination_port, 4321); EXPECT_EQ(packet.common_header().verification_tag, @@ -172,7 +178,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).has_value()); + EXPECT_FALSE(SctpPacket::Parse(data, kVerifyChecksumOptions).has_value()); } TEST(SctpPacketTest, DeserializePacketDontValidateChecksum) { @@ -202,7 +208,8 @@ TEST(SctpPacketTest, DeserializePacketDontValidateChecksum) { ASSERT_HAS_VALUE_AND_ASSIGN( SctpPacket packet, - SctpPacket::Parse(data, /*disable_checksum_verification=*/true)); + SctpPacket::Parse(data, {.disable_checksum_verification = true, + .enable_zero_checksum = false})); EXPECT_EQ(packet.common_header().source_port, 5000); EXPECT_EQ(packet.common_header().destination_port, 5000); EXPECT_EQ(packet.common_header().verification_tag, @@ -220,7 +227,8 @@ TEST(SctpPacketTest, SerializeAndDeserializeSingleChunk) { b.Add(init); std::vector serialized = b.Build(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(serialized)); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket packet, SctpPacket::Parse(serialized, kVerifyChecksumOptions)); EXPECT_EQ(packet.common_header().verification_tag, kVerificationTag); @@ -250,7 +258,8 @@ TEST(SctpPacketTest, SerializeAndDeserializeThreeChunks) { std::vector serialized = b.Build(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(serialized)); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket packet, SctpPacket::Parse(serialized, kVerifyChecksumOptions)); EXPECT_EQ(packet.common_header().verification_tag, kVerificationTag); @@ -279,7 +288,8 @@ TEST(SctpPacketTest, ParseAbortWithEmptyCause) { /*filled_in_verification_tag=*/true, Parameters::Builder().Add(UserInitiatedAbortCause("")).Build())); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(b.Build())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket packet, SctpPacket::Parse(b.Build(), kVerifyChecksumOptions)); EXPECT_EQ(packet.common_header().verification_tag, kVerificationTag); @@ -298,7 +308,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, true).has_value()); + EXPECT_FALSE(SctpPacket::Parse(data, kVerifyChecksumOptions).has_value()); } TEST(SctpPacketTest, ReturnsCorrectSpaceAvailableToStayWithinMTU) { @@ -338,5 +348,95 @@ 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 4511bed4a4..d19798054c 100644 --- a/net/dcsctp/public/dcsctp_options.h +++ b/net/dcsctp/public/dcsctp_options.h @@ -193,8 +193,17 @@ struct DcSctpOptions { // If RTO should be added to heartbeat_interval bool heartbeat_interval_include_rtt = true; - // Disables SCTP packet crc32 verification. Useful when running with fuzzers. + // Disables SCTP packet crc32 verification. For fuzzers only! 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 f831ba090c..139ec28055 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -755,8 +755,7 @@ void DcSctpSocket::ReceivePacket(rtc::ArrayView data) { packet_observer_->OnReceivedPacket(callbacks_.TimeMillis(), data); } - absl::optional packet = - SctpPacket::Parse(data, options_.disable_checksum_verification); + absl::optional packet = SctpPacket::Parse(data, options_); if (!packet.has_value()) { // https://tools.ietf.org/html/rfc4960#section-6.8 // "The default procedure for handling invalid SCTP packets is to @@ -798,7 +797,7 @@ void DcSctpSocket::ReceivePacket(rtc::ArrayView data) { } void DcSctpSocket::DebugPrintOutgoing(rtc::ArrayView payload) { - auto packet = SctpPacket::Parse(payload); + auto packet = SctpPacket::Parse(payload, options_); 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 f38624d606..0da893d9bd 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -67,9 +67,10 @@ 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); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -96,7 +97,7 @@ MATCHER_P(HasDataChunkWithStreamId, stream_id, "") { } MATCHER_P(HasDataChunkWithPPID, ppid, "") { - absl::optional packet = SctpPacket::Parse(arg); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -123,7 +124,7 @@ MATCHER_P(HasDataChunkWithPPID, ppid, "") { } MATCHER_P(HasDataChunkWithSsn, ssn, "") { - absl::optional packet = SctpPacket::Parse(arg); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -150,7 +151,7 @@ MATCHER_P(HasDataChunkWithSsn, ssn, "") { } MATCHER_P(HasDataChunkWithMid, mid, "") { - absl::optional packet = SctpPacket::Parse(arg); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -177,7 +178,7 @@ MATCHER_P(HasDataChunkWithMid, mid, "") { } MATCHER_P(HasSackWithCumAckTsn, tsn, "") { - absl::optional packet = SctpPacket::Parse(arg); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -204,7 +205,7 @@ MATCHER_P(HasSackWithCumAckTsn, tsn, "") { } MATCHER(HasSackWithNoGapAckBlocks, "") { - absl::optional packet = SctpPacket::Parse(arg); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -231,7 +232,7 @@ MATCHER(HasSackWithNoGapAckBlocks, "") { } MATCHER_P(HasReconfigWithStreams, streams_matcher, "") { - absl::optional packet = SctpPacket::Parse(arg); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -269,7 +270,7 @@ MATCHER_P(HasReconfigWithStreams, streams_matcher, "") { } MATCHER_P(HasReconfigWithResponse, result, "") { - absl::optional packet = SctpPacket::Parse(arg); + absl::optional packet = SctpPacket::Parse(arg, kDefaultOptions); if (!packet.has_value()) { *result_listener << "data didn't parse as an SctpPacket"; return false; @@ -328,7 +329,7 @@ std::unique_ptr GetPacketObserver(absl::string_view name) { struct SocketUnderTest { explicit SocketUnderTest(absl::string_view name, - const DcSctpOptions& opts = {}) + const DcSctpOptions& opts = kDefaultOptions) : options(FixupOptions(opts)), cb(name), socket(name, cb, GetPacketObserver(name), options) {} @@ -628,8 +629,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); EXPECT_EQ(init_packet.descriptors()[0].type, InitChunk::kType); AdvanceTime(a, z, a.options.t1_init_timeout); @@ -654,16 +656,18 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket resent_init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); EXPECT_EQ(resent_init_packet.descriptors()[0].type, InitChunk::kType); } @@ -687,8 +691,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); EXPECT_EQ(init_packet.descriptors()[0].type, CookieEchoChunk::kType); AdvanceTime(a, z, a.options.t1_init_timeout); @@ -714,16 +719,18 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket resent_init_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); EXPECT_EQ(resent_init_packet.descriptors()[0].type, CookieEchoChunk::kType); } @@ -751,8 +758,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket cookie_echo_packet1, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); 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); @@ -771,8 +779,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket cookie_echo_packet2, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); 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); @@ -836,8 +845,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); EXPECT_EQ(packet.descriptors()[0].type, ShutdownChunk::kType); EXPECT_TRUE(a.cb.ConsumeSentPacket().empty()); } @@ -847,8 +857,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z.options)); EXPECT_EQ(packet.descriptors()[0].type, AbortChunk::kType); EXPECT_TRUE(a.cb.ConsumeSentPacket().empty()); } @@ -956,8 +967,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket ack_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); ASSERT_THAT(ack_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( HeartbeatAckChunk ack, @@ -981,7 +993,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)); + SctpPacket::Parse(hb_packet_raw, z->options)); ASSERT_THAT(hb_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( HeartbeatRequestChunk hb, @@ -1018,8 +1030,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket hb_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); EXPECT_EQ(hb_packet.descriptors()[0].type, HeartbeatRequestChunk::kType); RTC_LOG(LS_INFO) << "Letting the heartbeat expire."; @@ -1072,7 +1085,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)); + SctpPacket::Parse(hb_packet_raw, z->options)); ASSERT_THAT(hb_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( HeartbeatRequestChunk hb, @@ -1092,8 +1105,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket another_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); EXPECT_EQ(another_packet.descriptors()[0].type, HeartbeatRequestChunk::kType); } @@ -1469,8 +1483,9 @@ 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())); + ASSERT_HAS_VALUE_AND_ASSIGN( + SctpPacket reply_packet, + SctpPacket::Parse(a.cb.ConsumeSentPacket(), z->options)); ASSERT_THAT(reply_packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( ErrorChunk error, ErrorChunk::Parse(reply_packet.descriptors()[0].data)); @@ -1517,7 +1532,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)); + SctpPacket::Parse(init_data, z.options)); ASSERT_HAS_VALUE_AND_ASSIGN( InitChunk init_chunk, InitChunk::Parse(init_packet.descriptors()[0].data)); @@ -2237,7 +2252,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)); + SctpPacket::Parse(init_data, z.options)); 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 faa0e3da06..d573192440 100644 --- a/net/dcsctp/socket/heartbeat_handler_test.cc +++ b/net/dcsctp/socket/heartbeat_handler_test.cc @@ -37,6 +37,7 @@ 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; } @@ -83,7 +84,8 @@ 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)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(payload, options_)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( @@ -101,7 +103,8 @@ TEST_F(HeartbeatHandlerTest, RepliesToHeartbeatRequests) { handler_.HandleHeartbeatRequest(std::move(request)); std::vector payload = callbacks_.ConsumeSentPacket(); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(payload, options_)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( @@ -120,7 +123,8 @@ 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)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(payload, options_)); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( @@ -143,7 +147,8 @@ 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)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(payload, options_)); 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 d3fa98752c..503cc89094 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); + absl::optional p = SctpPacket::Parse(payload, DcSctpOptions()); if (!p.has_value()) { EXPECT_TRUE(false); return {}; @@ -504,7 +504,8 @@ TEST_F(StreamResetHandlerTest, SendOutgoingResetRetransmitOnInProgress) { std::vector payload = callbacks_.ConsumeSentPacket(); ASSERT_FALSE(payload.empty()); - ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, SctpPacket::Parse(payload)); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet, + SctpPacket::Parse(payload, DcSctpOptions())); ASSERT_THAT(packet.descriptors(), SizeIs(1)); ASSERT_HAS_VALUE_AND_ASSIGN( ReConfigChunk reconfig2,