Revert "dcsctp: Negotiate zero checksum"
This reverts commit a736f30a5fddfa9a6af02a0a916da09bcac49d0d.
Due to a downstream project not supporting this
new handover state, it fails. Handover state needs
to be submitted first, then downstream project needs
to be updated, and finally the code changes can be
submitted.
Bug: webrtc:14997
Change-Id: I8551e349408d9bf4eb593cb948279d659467fe20
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/302821
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Victor Boivie <boivie@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39923}
diff --git a/net/dcsctp/public/dcsctp_handover_state.h b/net/dcsctp/public/dcsctp_handover_state.h
index f277ebc..253f4da 100644
--- a/net/dcsctp/public/dcsctp_handover_state.h
+++ b/net/dcsctp/public/dcsctp_handover_state.h
@@ -40,7 +40,6 @@
bool partial_reliability = false;
bool message_interleaving = false;
bool reconfig = false;
- bool zero_checksum = false;
uint16_t negotiated_maximum_incoming_streams = 0;
uint16_t negotiated_maximum_outgoing_streams = 0;
};
diff --git a/net/dcsctp/public/dcsctp_socket.h b/net/dcsctp/public/dcsctp_socket.h
index 35dc9c5..2df6a2c 100644
--- a/net/dcsctp/public/dcsctp_socket.h
+++ b/net/dcsctp/public/dcsctp_socket.h
@@ -245,10 +245,6 @@
// peers.
bool uses_message_interleaving = false;
- // Indicates if draft-tuexen-tsvwg-sctp-zero-checksum-00 has been negotiated
- // by both peers.
- bool uses_zero_checksum = false;
-
// The number of negotiated incoming and outgoing streams, which is configured
// locally as `DcSctpOptions::announced_maximum_incoming_streams` and
// `DcSctpOptions::announced_maximum_outgoing_streams`, and which will be
diff --git a/net/dcsctp/socket/capabilities.h b/net/dcsctp/socket/capabilities.h
index 286509a..fa3be37 100644
--- a/net/dcsctp/socket/capabilities.h
+++ b/net/dcsctp/socket/capabilities.h
@@ -21,8 +21,6 @@
bool message_interleaving = false;
// RFC6525 Stream Reconfiguration
bool reconfig = false;
- // https://datatracker.ietf.org/doc/draft-tuexen-tsvwg-sctp-zero-checksum/
- bool zero_checksum = false;
// Negotiated maximum incoming and outgoing stream count.
uint16_t negotiated_maximum_incoming_streams = 0;
uint16_t negotiated_maximum_outgoing_streams = 0;
diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc
index 1dc4ae1..139ec280 100644
--- a/net/dcsctp/socket/dcsctp_socket.cc
+++ b/net/dcsctp/socket/dcsctp_socket.cc
@@ -56,7 +56,6 @@
#include "net/dcsctp/packet/parameter/parameter.h"
#include "net/dcsctp/packet/parameter/state_cookie_parameter.h"
#include "net/dcsctp/packet/parameter/supported_extensions_parameter.h"
-#include "net/dcsctp/packet/parameter/zero_checksum_acceptable_chunk_parameter.h"
#include "net/dcsctp/packet/sctp_packet.h"
#include "net/dcsctp/packet/tlv_trait.h"
#include "net/dcsctp/public/dcsctp_message.h"
@@ -118,11 +117,6 @@
capabilities.reconfig = true;
}
- if (options.enable_zero_checksum &&
- parameters.get<ZeroChecksumAcceptableChunkParameter>().has_value()) {
- capabilities.zero_checksum = true;
- }
-
capabilities.negotiated_maximum_incoming_streams = std::min(
options.announced_maximum_incoming_streams, peer_nbr_outbound_streams);
capabilities.negotiated_maximum_outgoing_streams = std::min(
@@ -143,9 +137,6 @@
chunk_types.push_back(IDataChunk::kType);
chunk_types.push_back(IForwardTsnChunk::kType);
}
- if (options.enable_zero_checksum) {
- builder.Add(ZeroChecksumAcceptableChunkParameter());
- }
builder.Add(SupportedExtensionsParameter(std::move(chunk_types)));
}
@@ -288,10 +279,7 @@
connect_params_.initial_tsn, params_builder.Build());
SctpPacket::Builder b(VerificationTag(0), options_);
b.Add(init);
- // https://www.ietf.org/archive/id/draft-tuexen-tsvwg-sctp-zero-checksum-01.html#section-4.2
- // "When an end point sends a packet containing an INIT chunk, it MUST include
- // a correct CRC32c checksum in the packet containing the INIT chunk."
- packet_sender_.Send(b, /*write_checksum=*/true);
+ packet_sender_.Send(b);
}
void DcSctpSocket::MakeConnectionParameters() {
@@ -332,7 +320,6 @@
size_t a_rwnd,
TieTag tie_tag) {
metrics_.uses_message_interleaving = capabilities.message_interleaving;
- metrics_.uses_zero_checksum = capabilities.zero_checksum;
metrics_.negotiated_maximum_incoming_streams =
capabilities.negotiated_maximum_incoming_streams;
metrics_.negotiated_maximum_outgoing_streams =
@@ -364,7 +351,6 @@
capabilities.message_interleaving =
state.capabilities.message_interleaving;
capabilities.reconfig = state.capabilities.reconfig;
- capabilities.zero_checksum = state.capabilities.zero_checksum;
capabilities.negotiated_maximum_incoming_streams =
state.capabilities.negotiated_maximum_incoming_streams;
capabilities.negotiated_maximum_outgoing_streams =
@@ -1223,9 +1209,7 @@
options_.announced_maximum_incoming_streams,
connect_params_.initial_tsn, params_builder.Build());
b.Add(init_ack);
- // If the peer has signaled that it supports zero checksum, INIT-ACK can then
- // have its checksum as zero.
- packet_sender_.Send(b, /*write_checksum=*/!capabilities.zero_checksum);
+ packet_sender_.Send(b);
}
void DcSctpSocket::HandleInitAck(
diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc
index c1901b2..0da893d 100644
--- a/net/dcsctp/socket/dcsctp_socket_test.cc
+++ b/net/dcsctp/socket/dcsctp_socket_test.cc
@@ -23,7 +23,6 @@
#include "api/array_view.h"
#include "net/dcsctp/common/handover_testing.h"
#include "net/dcsctp/packet/chunk/chunk.h"
-#include "net/dcsctp/packet/chunk/cookie_ack_chunk.h"
#include "net/dcsctp/packet/chunk/cookie_echo_chunk.h"
#include "net/dcsctp/packet/chunk/data_chunk.h"
#include "net/dcsctp/packet/chunk/data_common.h"
@@ -31,7 +30,6 @@
#include "net/dcsctp/packet/chunk/heartbeat_ack_chunk.h"
#include "net/dcsctp/packet/chunk/heartbeat_request_chunk.h"
#include "net/dcsctp/packet/chunk/idata_chunk.h"
-#include "net/dcsctp/packet/chunk/init_ack_chunk.h"
#include "net/dcsctp/packet/chunk/init_chunk.h"
#include "net/dcsctp/packet/chunk/sack_chunk.h"
#include "net/dcsctp/packet/chunk/shutdown_chunk.h"
@@ -60,10 +58,8 @@
using ::testing::_;
using ::testing::AllOf;
using ::testing::ElementsAre;
-using ::testing::Eq;
using ::testing::HasSubstr;
using ::testing::IsEmpty;
-using ::testing::Not;
using ::testing::SizeIs;
using ::testing::UnorderedElementsAre;
@@ -2868,156 +2864,5 @@
EXPECT_EQ(msg2->payload().size(), kSmallMessageSize);
}
-TEST_P(DcSctpSocketParametrizedTest, ZeroChecksumMetricsAreSet) {
- std::vector<std::pair<bool, bool>> combinations = {
- {false, false}, {false, true}, {true, false}, {true, true}};
- for (const auto& [a_enable, z_enable] : combinations) {
- DcSctpOptions a_options = {.enable_zero_checksum = a_enable};
- DcSctpOptions z_options = {.enable_zero_checksum = z_enable};
-
- SocketUnderTest a("A", a_options);
- auto z = std::make_unique<SocketUnderTest>("Z", z_options);
-
- ConnectSockets(a, *z);
- z = MaybeHandoverSocket(std::move(z));
-
- EXPECT_EQ(a.socket.GetMetrics()->uses_zero_checksum, a_enable && z_enable);
- EXPECT_EQ(z->socket.GetMetrics()->uses_zero_checksum, a_enable && z_enable);
- }
-}
-
-TEST(DcSctpSocketTest, AlwaysSendsInitWithNonZeroChecksum) {
- DcSctpOptions options = {.enable_zero_checksum = true};
- SocketUnderTest a("A", options);
-
- a.socket.Connect();
- std::vector<uint8_t> data = a.cb.ConsumeSentPacket();
- ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
- SctpPacket::Parse(data, options));
- EXPECT_THAT(packet.descriptors(),
- ElementsAre(testing::Field(&SctpPacket::ChunkDescriptor::type,
- InitChunk::kType)));
- EXPECT_THAT(packet.common_header().checksum, Not(Eq(0u)));
-}
-
-TEST(DcSctpSocketTest, MaySendInitAckWithZeroChecksum) {
- DcSctpOptions options = {.enable_zero_checksum = true};
- SocketUnderTest a("A", options);
- SocketUnderTest z("Z", options);
-
- a.socket.Connect();
- z.socket.ReceivePacket(a.cb.ConsumeSentPacket()); // INIT
-
- std::vector<uint8_t> data = z.cb.ConsumeSentPacket();
- ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
- SctpPacket::Parse(data, options));
- EXPECT_THAT(packet.descriptors(),
- ElementsAre(testing::Field(&SctpPacket::ChunkDescriptor::type,
- InitAckChunk::kType)));
- EXPECT_THAT(packet.common_header().checksum, 0u);
-}
-
-TEST(DcSctpSocketTest, AlwaysSendsCookieEchoWithNonZeroChecksum) {
- DcSctpOptions options = {.enable_zero_checksum = true};
- SocketUnderTest a("A", options);
- SocketUnderTest z("Z", options);
-
- a.socket.Connect();
- z.socket.ReceivePacket(a.cb.ConsumeSentPacket()); // INIT
- a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); // INIT-ACK
-
- std::vector<uint8_t> data = a.cb.ConsumeSentPacket();
- ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
- SctpPacket::Parse(data, options));
- EXPECT_THAT(packet.descriptors(),
- ElementsAre(testing::Field(&SctpPacket::ChunkDescriptor::type,
- CookieEchoChunk::kType)));
- EXPECT_THAT(packet.common_header().checksum, Not(Eq(0u)));
-}
-
-TEST(DcSctpSocketTest, SendsCookieAckWithZeroChecksum) {
- DcSctpOptions options = {.enable_zero_checksum = true};
- SocketUnderTest a("A", options);
- SocketUnderTest z("Z", options);
-
- a.socket.Connect();
- z.socket.ReceivePacket(a.cb.ConsumeSentPacket()); // INIT
- a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); // INIT-ACK
- z.socket.ReceivePacket(a.cb.ConsumeSentPacket()); // COOKIE-ECHO
-
- std::vector<uint8_t> data = z.cb.ConsumeSentPacket();
- ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
- SctpPacket::Parse(data, options));
- EXPECT_THAT(packet.descriptors(),
- ElementsAre(testing::Field(&SctpPacket::ChunkDescriptor::type,
- CookieAckChunk::kType)));
- EXPECT_THAT(packet.common_header().checksum, 0u);
-}
-
-TEST_P(DcSctpSocketParametrizedTest, SendsDataWithZeroChecksum) {
- DcSctpOptions options = {.enable_zero_checksum = true};
- SocketUnderTest a("A", options);
- auto z = std::make_unique<SocketUnderTest>("Z", options);
-
- ConnectSockets(a, *z);
- z = MaybeHandoverSocket(std::move(z));
-
- std::vector<uint8_t> payload(a.options.mtu - 100);
- a.socket.Send(DcSctpMessage(StreamID(1), PPID(53), payload), {});
-
- std::vector<uint8_t> data = a.cb.ConsumeSentPacket();
- z->socket.ReceivePacket(data);
- ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
- SctpPacket::Parse(data, options));
- EXPECT_THAT(packet.descriptors(),
- ElementsAre(testing::Field(&SctpPacket::ChunkDescriptor::type,
- DataChunk::kType)));
- EXPECT_THAT(packet.common_header().checksum, 0u);
-
- MaybeHandoverSocketAndSendMessage(a, std::move(z));
-}
-
-TEST_P(DcSctpSocketParametrizedTest, AllPacketsAfterConnectHaveZeroChecksum) {
- DcSctpOptions options = {.enable_zero_checksum = true};
- SocketUnderTest a("A", options);
- auto z = std::make_unique<SocketUnderTest>("Z", options);
-
- ConnectSockets(a, *z);
- z = MaybeHandoverSocket(std::move(z));
-
- // Send large messages in both directions, and verify that they arrive and
- // that every packet has zero checksum.
- std::vector<uint8_t> payload(kLargeMessageSize);
- a.socket.Send(DcSctpMessage(StreamID(1), PPID(53), payload), kSendOptions);
- z->socket.Send(DcSctpMessage(StreamID(1), PPID(53), payload), kSendOptions);
-
- for (;;) {
- if (auto data = a.cb.ConsumeSentPacket(); !data.empty()) {
- ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
- SctpPacket::Parse(data, options));
- EXPECT_THAT(packet.common_header().checksum, 0u);
- z->socket.ReceivePacket(std::move(data));
-
- } else if (auto data = z->cb.ConsumeSentPacket(); !data.empty()) {
- ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
- SctpPacket::Parse(data, options));
- EXPECT_THAT(packet.common_header().checksum, 0u);
- a.socket.ReceivePacket(std::move(data));
-
- } else {
- break;
- }
- }
-
- absl::optional<DcSctpMessage> msg1 = z->cb.ConsumeReceivedMessage();
- ASSERT_TRUE(msg1.has_value());
- EXPECT_THAT(msg1->payload(), SizeIs(kLargeMessageSize));
-
- absl::optional<DcSctpMessage> msg2 = a.cb.ConsumeReceivedMessage();
- ASSERT_TRUE(msg2.has_value());
- EXPECT_THAT(msg2->payload(), SizeIs(kLargeMessageSize));
-
- MaybeHandoverSocketAndSendMessage(a, std::move(z));
-}
} // namespace
} // namespace dcsctp
diff --git a/net/dcsctp/socket/packet_sender.cc b/net/dcsctp/socket/packet_sender.cc
index f0134ee..85392e2 100644
--- a/net/dcsctp/socket/packet_sender.cc
+++ b/net/dcsctp/socket/packet_sender.cc
@@ -21,12 +21,12 @@
SendPacketStatus)> on_sent_packet)
: callbacks_(callbacks), on_sent_packet_(std::move(on_sent_packet)) {}
-bool PacketSender::Send(SctpPacket::Builder& builder, bool write_checksum) {
+bool PacketSender::Send(SctpPacket::Builder& builder) {
if (builder.empty()) {
return false;
}
- std::vector<uint8_t> payload = builder.Build(write_checksum);
+ std::vector<uint8_t> payload = builder.Build();
SendPacketStatus status = callbacks_.SendPacketWithStatus(payload);
on_sent_packet_(payload, status);
diff --git a/net/dcsctp/socket/packet_sender.h b/net/dcsctp/socket/packet_sender.h
index 395c2ef..7af4d3c 100644
--- a/net/dcsctp/socket/packet_sender.h
+++ b/net/dcsctp/socket/packet_sender.h
@@ -25,7 +25,7 @@
SendPacketStatus)> on_sent_packet);
// Sends the packet, and returns true if it was sent successfully.
- bool Send(SctpPacket::Builder& builder, bool write_checksum = true);
+ bool Send(SctpPacket::Builder& builder);
private:
DcSctpSocketCallbacks& callbacks_;
diff --git a/net/dcsctp/socket/state_cookie.cc b/net/dcsctp/socket/state_cookie.cc
index 624d783..86be77a 100644
--- a/net/dcsctp/socket/state_cookie.cc
+++ b/net/dcsctp/socket/state_cookie.cc
@@ -42,7 +42,6 @@
buffer.Store8<30>(capabilities_.reconfig);
buffer.Store16<32>(capabilities_.negotiated_maximum_incoming_streams);
buffer.Store16<34>(capabilities_.negotiated_maximum_outgoing_streams);
- buffer.Store8<36>(capabilities_.zero_checksum);
return cookie;
}
@@ -75,7 +74,6 @@
capabilities.reconfig = buffer.Load8<30>() != 0;
capabilities.negotiated_maximum_incoming_streams = buffer.Load16<32>();
capabilities.negotiated_maximum_outgoing_streams = buffer.Load16<34>();
- capabilities.zero_checksum = buffer.Load8<36>() != 0;
return StateCookie(verification_tag, initial_tsn, a_rwnd, tie_tag,
capabilities);
diff --git a/net/dcsctp/socket/state_cookie.h b/net/dcsctp/socket/state_cookie.h
index 34cd6d3..a26dbf8 100644
--- a/net/dcsctp/socket/state_cookie.h
+++ b/net/dcsctp/socket/state_cookie.h
@@ -27,7 +27,7 @@
// Do not trust anything in it; no pointers or anything like that.
class StateCookie {
public:
- static constexpr size_t kCookieSize = 37;
+ static constexpr size_t kCookieSize = 36;
StateCookie(VerificationTag initiate_tag,
TSN initial_tsn,
diff --git a/net/dcsctp/socket/state_cookie_test.cc b/net/dcsctp/socket/state_cookie_test.cc
index 19be71a..7d8e133 100644
--- a/net/dcsctp/socket/state_cookie_test.cc
+++ b/net/dcsctp/socket/state_cookie_test.cc
@@ -21,7 +21,6 @@
Capabilities capabilities = {.partial_reliability = true,
.message_interleaving = false,
.reconfig = true,
- .zero_checksum = true,
.negotiated_maximum_incoming_streams = 123,
.negotiated_maximum_outgoing_streams = 234};
StateCookie cookie(VerificationTag(123), TSN(456),
@@ -37,7 +36,6 @@
EXPECT_TRUE(deserialized.capabilities().partial_reliability);
EXPECT_FALSE(deserialized.capabilities().message_interleaving);
EXPECT_TRUE(deserialized.capabilities().reconfig);
- EXPECT_TRUE(deserialized.capabilities().zero_checksum);
EXPECT_EQ(deserialized.capabilities().negotiated_maximum_incoming_streams,
123);
EXPECT_EQ(deserialized.capabilities().negotiated_maximum_outgoing_streams,
diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc
index 8bb1e8b..1dcf394 100644
--- a/net/dcsctp/socket/transmission_control_block.cc
+++ b/net/dcsctp/socket/transmission_control_block.cc
@@ -163,7 +163,7 @@
} else {
builder.Add(retransmission_queue_.CreateForwardTsn());
}
- Send(builder);
+ packet_sender_.Send(builder);
// https://datatracker.ietf.org/doc/html/rfc3758
// "IMPLEMENTATION NOTE: An implementation may wish to limit the number of
// duplicate FORWARD TSN chunks it sends by ... waiting a full RTT before
@@ -198,7 +198,7 @@
builder.Add(DataChunk(tsn, std::move(data), false));
}
}
- Send(builder);
+ packet_sender_.Send(builder);
}
void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder,
@@ -245,13 +245,7 @@
}
}
- // https://www.ietf.org/archive/id/draft-tuexen-tsvwg-sctp-zero-checksum-02.html#section-4.2
- // "When an end point sends a packet containing a COOKIE ECHO chunk, it MUST
- // include a correct CRC32c checksum in the packet containing the COOKIE
- // ECHO chunk."
- bool write_checksum =
- !capabilities_.zero_checksum || cookie_echo_chunk_.has_value();
- if (!packet_sender_.Send(builder, write_checksum)) {
+ if (!packet_sender_.Send(builder)) {
break;
}
@@ -280,9 +274,6 @@
if (capabilities_.reconfig) {
sb << "Reconfig,";
}
- if (capabilities_.zero_checksum) {
- sb << "ZeroChecksum,";
- }
sb << " max_in=" << capabilities_.negotiated_maximum_incoming_streams;
sb << " max_out=" << capabilities_.negotiated_maximum_outgoing_streams;
@@ -303,7 +294,6 @@
state.capabilities.partial_reliability = capabilities_.partial_reliability;
state.capabilities.message_interleaving = capabilities_.message_interleaving;
state.capabilities.reconfig = capabilities_.reconfig;
- state.capabilities.zero_checksum = capabilities_.zero_checksum;
state.capabilities.negotiated_maximum_incoming_streams =
capabilities_.negotiated_maximum_incoming_streams;
state.capabilities.negotiated_maximum_outgoing_streams =
diff --git a/net/dcsctp/socket/transmission_control_block.h b/net/dcsctp/socket/transmission_control_block.h
index 46a39d5..fc66fcc 100644
--- a/net/dcsctp/socket/transmission_control_block.h
+++ b/net/dcsctp/socket/transmission_control_block.h
@@ -80,8 +80,7 @@
return tx_error_counter_.IsExhausted();
}
void Send(SctpPacket::Builder& builder) override {
- packet_sender_.Send(builder,
- /*write_checksum=*/!capabilities_.zero_checksum);
+ packet_sender_.Send(builder);
}
// Other accessors
diff --git a/net/dcsctp/socket/transmission_control_block_test.cc b/net/dcsctp/socket/transmission_control_block_test.cc
index 6106fbb..40aea58 100644
--- a/net/dcsctp/socket/transmission_control_block_test.cc
+++ b/net/dcsctp/socket/transmission_control_block_test.cc
@@ -92,7 +92,6 @@
capabilities_.negotiated_maximum_outgoing_streams = 2000;
capabilities_.message_interleaving = true;
capabilities_.partial_reliability = true;
- capabilities_.zero_checksum = true;
capabilities_.reconfig = true;
TransmissionControlBlock tcb(
@@ -100,10 +99,9 @@
kMyVerificationTag, kMyInitialTsn, kPeerVerificationTag, kPeerInitialTsn,
kArwnd, kTieTag, sender_, on_connection_established.AsStdFunction());
- EXPECT_EQ(
- tcb.ToString(),
- "verification_tag=000001c8, last_cumulative_ack=999, "
- "capabilities=PR,IL,Reconfig,ZeroChecksum, max_in=1000 max_out=2000");
+ EXPECT_EQ(tcb.ToString(),
+ "verification_tag=000001c8, last_cumulative_ack=999, "
+ "capabilities=PR,IL,Reconfig, max_in=1000 max_out=2000");
}
TEST_F(TransmissionControlBlockTest, IsInitiallyHandoverReady) {
