dcsctp: Handle re-received stream reset requests
When re-receiving a stream reset request with the same request
sequence number, reply with the same result as previous time. In
case the original request was deferred, and "in progress" was
replied, it's important to not indicate that it was performed
successfully as that will make the sender believe it has completed
before it did.
Bug: webrtc:14277
Change-Id: I5c7082dc285180d62061d7ebebe05566e5c4195c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/274080
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38012}
diff --git a/net/dcsctp/socket/stream_reset_handler.cc b/net/dcsctp/socket/stream_reset_handler.cc
index 9d86953..c81b34b 100644
--- a/net/dcsctp/socket/stream_reset_handler.cc
+++ b/net/dcsctp/socket/stream_reset_handler.cc
@@ -134,11 +134,16 @@
ReconfigRequestSN req_seq_nbr,
std::vector<ReconfigurationResponseParameter>& responses) {
if (req_seq_nbr == last_processed_req_seq_nbr_) {
- // This has already been performed previously.
+ // https://www.rfc-editor.org/rfc/rfc6525.html#section-5.2.1 "If the
+ // received RE-CONFIG chunk contains at least one request and based on the
+ // analysis of the Re-configuration Request Sequence Numbers this is the
+ // last received RE-CONFIG chunk (i.e., a retransmission), the same
+ // RE-CONFIG chunk MUST to be sent back in response, as it was earlier."
RTC_DLOG(LS_VERBOSE) << log_prefix_ << "req=" << *req_seq_nbr
- << " already processed";
+ << " already processed, returning result="
+ << ToString(last_processed_req_result_);
responses.push_back(ReconfigurationResponseParameter(
- req_seq_nbr, ResponseResult::kSuccessNothingToDo));
+ req_seq_nbr, last_processed_req_result_));
return false;
}
@@ -170,20 +175,18 @@
}
if (ValidateReqSeqNbr(req->request_sequence_number(), responses)) {
- ResponseResult result;
-
RTC_DLOG(LS_VERBOSE) << log_prefix_
<< "Reset outgoing streams with req_seq_nbr="
<< *req->request_sequence_number();
last_processed_req_seq_nbr_ = req->request_sequence_number();
- result = reassembly_queue_->ResetStreams(
+ last_processed_req_result_ = reassembly_queue_->ResetStreams(
*req, data_tracker_->last_cumulative_acked_tsn());
- if (result == ResponseResult::kSuccessPerformed) {
+ if (last_processed_req_result_ == ResponseResult::kSuccessPerformed) {
ctx_->callbacks().OnIncomingStreamsReset(req->stream_ids());
}
responses.push_back(ReconfigurationResponseParameter(
- req->request_sequence_number(), result));
+ req->request_sequence_number(), last_processed_req_result_));
}
}
diff --git a/net/dcsctp/socket/stream_reset_handler.h b/net/dcsctp/socket/stream_reset_handler.h
index 6e49665..fa32e5f 100644
--- a/net/dcsctp/socket/stream_reset_handler.h
+++ b/net/dcsctp/socket/stream_reset_handler.h
@@ -88,8 +88,9 @@
last_processed_req_seq_nbr_(
handover_state ? ReconfigRequestSN(
handover_state->rx.last_completed_reset_req_sn)
- : ReconfigRequestSN(*ctx_->peer_initial_tsn() - 1)) {
- }
+ : ReconfigRequestSN(*ctx_->peer_initial_tsn() - 1)),
+ last_processed_req_result_(
+ ReconfigurationResponseParameter::Result::kSuccessNothingToDo) {}
// Initiates reset of the provided streams. While there can only be one
// ongoing stream reset request at any time, this method can be called at any
@@ -224,6 +225,8 @@
// For incoming requests - last processed request sequence number.
ReconfigRequestSN last_processed_req_seq_nbr_;
+ // The result from last processed incoming request
+ ReconfigurationResponseParameter::Result last_processed_req_result_;
};
} // namespace dcsctp
diff --git a/net/dcsctp/socket/stream_reset_handler_test.cc b/net/dcsctp/socket/stream_reset_handler_test.cc
index 493b4c4..d3fa987 100644
--- a/net/dcsctp/socket/stream_reset_handler_test.cc
+++ b/net/dcsctp/socket/stream_reset_handler_test.cc
@@ -586,36 +586,20 @@
EXPECT_THAT(responses[0].result(), ResponseResult::kSuccessNothingToDo);
}
-TEST_F(StreamResetHandlerTest, SendSameRequestTwiceReturnsNothingToDo) {
- reasm_->Add(kPeerInitialTsn, gen_.Ordered({1, 2, 3, 4}, "BE"));
- reasm_->Add(AddTo(kPeerInitialTsn, 1), gen_.Ordered({1, 2, 3, 4}, "BE"));
+TEST_F(StreamResetHandlerTest, SendSameRequestTwiceIsIdempotent) {
+ // Simulate that receiving the same chunk twice (due to network issues,
+ // or retransmissions, causing a RECONFIG to be re-received) is idempotent.
+ for (int i = 0; i < 2; ++i) {
+ Parameters::Builder builder;
+ builder.Add(OutgoingSSNResetRequestParameter(
+ kPeerInitialReqSn, ReconfigRequestSN(3), AddTo(kPeerInitialTsn, 1),
+ {StreamID(1)}));
- data_tracker_->Observe(kPeerInitialTsn);
- data_tracker_->Observe(AddTo(kPeerInitialTsn, 1));
- EXPECT_THAT(reasm_->FlushMessages(),
- UnorderedElementsAre(
- SctpMessageIs(StreamID(1), PPID(53), kShortPayload),
- SctpMessageIs(StreamID(1), PPID(53), kShortPayload)));
-
- Parameters::Builder builder1;
- builder1.Add(OutgoingSSNResetRequestParameter(
- kPeerInitialReqSn, ReconfigRequestSN(3), AddTo(kPeerInitialTsn, 1),
- {StreamID(1)}));
-
- std::vector<ReconfigurationResponseParameter> responses1 =
- HandleAndCatchResponse(ReConfigChunk(builder1.Build()));
- EXPECT_THAT(responses1, SizeIs(1));
- EXPECT_EQ(responses1[0].result(), ResponseResult::kSuccessPerformed);
-
- Parameters::Builder builder2;
- builder2.Add(OutgoingSSNResetRequestParameter(
- kPeerInitialReqSn, ReconfigRequestSN(3), AddTo(kPeerInitialTsn, 1),
- {StreamID(1)}));
-
- std::vector<ReconfigurationResponseParameter> responses2 =
- HandleAndCatchResponse(ReConfigChunk(builder2.Build()));
- EXPECT_THAT(responses2, SizeIs(1));
- EXPECT_EQ(responses2[0].result(), ResponseResult::kSuccessNothingToDo);
+ std::vector<ReconfigurationResponseParameter> responses1 =
+ HandleAndCatchResponse(ReConfigChunk(builder.Build()));
+ EXPECT_THAT(responses1, SizeIs(1));
+ EXPECT_EQ(responses1[0].result(), ResponseResult::kInProgress);
+ }
}
TEST_F(StreamResetHandlerTest,
