Merge pull request #542 from paullouisageneau/fix-dc-teardown

Fix message ping-pong on Data Channel close

src/impl/peerconnection.cpp

@@ -273,7 +273,8 @@ shared_ptr<SctpTransport> PeerConnection::initSctpTransport() {
 
 		auto remote = remoteDescription();
 		if (!remote || !remote->application())
-			throw std::logic_error("Starting SCTP transport without remote application description");
+			throw std::logic_error(
+			    "Starting SCTP transport without remote application description");
 
 		SctpTransport::Ports ports = {};
 		ports.local = local->application()->sctpPort().value_or(DEFAULT_SCTP_PORT);
@@ -418,10 +419,18 @@ void PeerConnection::forwardMessage(message_ptr message) {
 		if (!iceTransport || !sctpTransport)
 			return;
 
+		// See https://tools.ietf.org/html/rfc8832
 		const byte dataChannelOpenMessage{0x03};
 		uint16_t remoteParity = (iceTransport->role() == Description::Role::Active) ? 1 : 0;
-		if (message->type == Message::Control && *message->data() == dataChannelOpenMessage &&
-		    stream % 2 == remoteParity) {
+		if (message->type == Message::Control) {
+			if (message->size() == 0 || *message->data() != dataChannelOpenMessage)
+				return; // ignore
+
+			if (stream % 2 != remoteParity) {
+				// The odd/even rule is violated, close the DataChannel
+				sctpTransport->closeStream(message->stream);
+				return;
+			}
 
 			channel =
 			    std::make_shared<NegotiatedDataChannel>(weak_from_this(), sctpTransport, stream);
@@ -430,6 +439,7 @@ void PeerConnection::forwardMessage(message_ptr message) {
 
 			std::unique_lock lock(mDataChannelsMutex); // we are going to emplace
 			mDataChannels.emplace(stream, channel);
+
 		} else {
 			// Invalid, close the DataChannel
 			sctpTransport->closeStream(message->stream);

src/impl/sctptransport.cpp

@@ -441,8 +441,12 @@ bool SctpTransport::flush() {
 void SctpTransport::closeStream(unsigned int stream) {
 	std::lock_guard lock(mSendMutex);
 
-	// This method must not call the buffered callback synchronously
+	// RFC 8831 6.7. Closing a Data Channel
+	// Closing of a data channel MUST be signaled by resetting the corresponding outgoing streams
+	// See https://tools.ietf.org/html/rfc8831#section-6.7
 	mSendQueue.push(make_message(0, Message::Reset, to_uint16(stream)));
+
+	// This method must not call the buffered callback synchronously
 	mProcessor.enqueue(&SctpTransport::flush, this);
 }
 
@@ -852,12 +856,11 @@ void SctpTransport::processNotification(const union sctp_notification *notify, s
 			PLOG_VERBOSE << "SCTP reset event, " << desc.str();
 		}
 
-		if (flags & SCTP_STREAM_RESET_OUTGOING_SSN) {
-			for (int i = 0; i < count; ++i) {
-				uint16_t streamId = reset_event.strreset_stream_list[i];
-				closeStream(streamId);
-			}
-		}
+		// RFC 8831 6.7. Closing a Data Channel
+		// If one side decides to close the data channel, it resets the corresponding outgoing
+		// stream. When the peer sees that an incoming stream was reset, it also resets its
+		// corresponding outgoing stream.
+		// See https://tools.ietf.org/html/rfc8831#section-6.7
 		if (flags & SCTP_STREAM_RESET_INCOMING_SSN) {
 			const byte dataChannelCloseMessage{0x04};
 			for (int i = 0; i < count; ++i) {