commit 1f928d33161daa8df4b8a85a72764d79d9cef0e4
author Harald Alvestrand <hta@webrtc.org> Thu Mar 28 11:29:38 2019 +0100
committer Commit Bot <commit-bot@chromium.org> Thu Mar 28 17:34:07 2019 +0000
tree 3eba7d17be7bbb197f33ac0e3772d14e618ce962
parent 4c7112a27a535990ed7d7a6d990391ca7ffdc3eb

Close data channels when ID assignment fails.

This prevents crashes due to unassigned IDs.

Bug: chromium:945256
Change-Id: I63f3a17cc7dff07dab58a6bc59fe3606b23e8e18
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/129902
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27349}

pc/data_channel.h

diff --git a/pc/data_channel.h b/pc/data_channel.h
index fa5c0f5..eef1279 100644
--- a/pc/data_channel.h
+++ b/pc/data_channel.h
@@ -147,6 +147,13 @@
   virtual uint64_t bytes_received() const { return bytes_received_; }
   virtual bool Send(const DataBuffer& buffer);
 
+  // Close immediately, ignoring any queued data or closing procedure.
+  // This is called for RTP data channels when SDP indicates a channel should
+  // be removed, or SCTP data channels when the underlying SctpTransport is
+  // being destroyed.
+  // It is also called by the PeerConnection if SCTP ID assignment fails.
+  void CloseAbruptly();
+
   // Called when the channel's ready to use.  That can happen when the
   // underlying DataMediaChannel becomes ready, or when this channel is a new
   // stream on an existing DataMediaChannel, and we've finished negotiation.
@@ -242,11 +249,6 @@
   };
 
   bool Init(const InternalDataChannelInit& config);
-  // Close immediately, ignoring any queued data or closing procedure.
-  // This is called for RTP data channels when SDP indicates a channel should
-  // be removed, or SCTP data channels when the underlying SctpTransport is
-  // being destroyed.
-  void CloseAbruptly();
   void UpdateState();
   void SetState(DataState state);
   void DisconnectFromProvider();

pc/peer_connection.cc

diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 211c5a7..5b702e0 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -5102,16 +5102,23 @@
 }
 
 void PeerConnection::AllocateSctpSids(rtc::SSLRole role) {
+  std::vector<rtc::scoped_refptr<DataChannel>> channels_to_close;
   for (const auto& channel : sctp_data_channels_) {
     if (channel->id() < 0) {
       int sid;
       if (!sid_allocator_.AllocateSid(role, &sid)) {
-        RTC_LOG(LS_ERROR) << "Failed to allocate SCTP sid.";
+        RTC_LOG(LS_ERROR) << "Failed to allocate SCTP sid, closing channel.";
+        channels_to_close.push_back(channel);
         continue;
       }
       channel->SetSctpSid(sid);
     }
   }
+  // Since closing modifies the list of channels, we have to do the actual
+  // closing outside the loop.
+  for (const auto& channel : channels_to_close) {
+    channel->CloseAbruptly();
+  }
 }
 
 void PeerConnection::OnSctpDataChannelClosed(DataChannel* channel) {

pc/peer_connection_end_to_end_unittest.cc

diff --git a/pc/peer_connection_end_to_end_unittest.cc b/pc/peer_connection_end_to_end_unittest.cc
index ae8366f..b37d00a 100644
--- a/pc/peer_connection_end_to_end_unittest.cc
+++ b/pc/peer_connection_end_to_end_unittest.cc
@@ -19,6 +19,7 @@
 #include "api/audio_codecs/audio_encoder_factory_template.h"
 #include "api/audio_codecs/builtin_audio_decoder_factory.h"
 #include "api/audio_codecs/builtin_audio_encoder_factory.h"
+#include "media/sctp/sctp_transport_internal.h"
 #include "rtc_base/gunit.h"
 #include "rtc_base/logging.h"
 
@@ -721,6 +722,29 @@
   // close message and be destroyed.
   rtc::Thread::Current()->ProcessMessages(100);
 }
+
+// Test behavior of creating too many datachannels.
+TEST_P(PeerConnectionEndToEndTest, TooManyDataChannelsOpenedBeforeConnecting) {
+  CreatePcs(webrtc::MockAudioEncoderFactory::CreateEmptyFactory(),
+            webrtc::MockAudioDecoderFactory::CreateEmptyFactory());
+
+  webrtc::DataChannelInit init;
+  std::vector<rtc::scoped_refptr<DataChannelInterface>> channels;
+  for (int i = 0; i <= cricket::kMaxSctpStreams / 2; i++) {
+    rtc::scoped_refptr<DataChannelInterface> caller_dc(
+        caller_->CreateDataChannel("data", init));
+    channels.push_back(std::move(caller_dc));
+  }
+  Negotiate();
+  WaitForConnection();
+  EXPECT_EQ_WAIT(callee_signaled_data_channels_.size(),
+                 static_cast<size_t>(cricket::kMaxSctpStreams / 2), kMaxWait);
+  EXPECT_EQ(DataChannelInterface::kOpen,
+            channels[(cricket::kMaxSctpStreams / 2) - 1]->state());
+  EXPECT_EQ(DataChannelInterface::kClosed,
+            channels[cricket::kMaxSctpStreams / 2]->state());
+}
+
 #endif  // HAVE_SCTP
 
 INSTANTIATE_TEST_SUITE_P(PeerConnectionEndToEndTest,