From 5625a86f5aa018d80d1dcc4890e064c7e85c6d1b Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Sun, 4 Sep 2022 19:37:32 +0000 Subject: [PATCH] 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 Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/main@{#38012} --- net/dcsctp/socket/stream_reset_handler.cc | 19 +++++---- net/dcsctp/socket/stream_reset_handler.h | 7 +++- .../socket/stream_reset_handler_test.cc | 42 ++++++------------- 3 files changed, 29 insertions(+), 39 deletions(-) diff --git a/net/dcsctp/socket/stream_reset_handler.cc b/net/dcsctp/socket/stream_reset_handler.cc index 9d86953f44..c81b34b626 100644 --- a/net/dcsctp/socket/stream_reset_handler.cc +++ b/net/dcsctp/socket/stream_reset_handler.cc @@ -134,11 +134,16 @@ bool StreamResetHandler::ValidateReqSeqNbr( ReconfigRequestSN req_seq_nbr, std::vector& 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 @@ void StreamResetHandler::HandleResetOutgoing( } 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 6e49665538..fa32e5fcc9 100644 --- a/net/dcsctp/socket/stream_reset_handler.h +++ b/net/dcsctp/socket/stream_reset_handler.h @@ -88,8 +88,9 @@ class StreamResetHandler { 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 @@ class StreamResetHandler { // 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 493b4c4bf7..d3fa98752c 100644 --- a/net/dcsctp/socket/stream_reset_handler_test.cc +++ b/net/dcsctp/socket/stream_reset_handler_test.cc @@ -586,36 +586,20 @@ TEST_F(StreamResetHandlerTest, SendIncomingResetJustReturnsNothingPerformed) { 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 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 responses2 = - HandleAndCatchResponse(ReConfigChunk(builder2.Build())); - EXPECT_THAT(responses2, SizeIs(1)); - EXPECT_EQ(responses2[0].result(), ResponseResult::kSuccessNothingToDo); + std::vector responses1 = + HandleAndCatchResponse(ReConfigChunk(builder.Build())); + EXPECT_THAT(responses1, SizeIs(1)); + EXPECT_EQ(responses1[0].result(), ResponseResult::kInProgress); + } } TEST_F(StreamResetHandlerTest,