commit 691d4a0e0657af7a4c6cb2b954d21e8667f15f8b
author Florent Castelli <orphis@webrtc.org> Mon Mar 13 14:17:17 2023
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Mar 14 09:58:58 2023
tree 44e54e2b4807106b7522fb95c1f130434bec5d02
parent bced11ae3cafb4c055c552ff7ec847dce9b279c6

sctp: Properly drop messages with unknown PPID values

Bug: webrtc:14992
Change-Id: I535cd939949ba35072e407d73450093a512aa2ff
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/297403
Auto-Submit: Florent Castelli <orphis@webrtc.org>
Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org>
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39550}

media/sctp/dcsctp_transport.cc

diff --git a/media/sctp/dcsctp_transport.cc b/media/sctp/dcsctp_transport.cc
index 10eaa00..0181f7a 100644
--- a/media/sctp/dcsctp_transport.cc
+++ b/media/sctp/dcsctp_transport.cc
@@ -461,6 +461,7 @@
                         << "->OnMessageReceived(): Received an unknown PPID "
                         << message.ppid().value()
                         << " on an SCTP packet. Dropping.";
+    return;
   }
   receive_data_params.type = *type;
   receive_buffer_.Clear();

media/sctp/dcsctp_transport_unittest.cc

diff --git a/media/sctp/dcsctp_transport_unittest.cc b/media/sctp/dcsctp_transport_unittest.cc
index 08dc2ec..cb1ddec 100644
--- a/media/sctp/dcsctp_transport_unittest.cc
+++ b/media/sctp/dcsctp_transport_unittest.cc
@@ -31,7 +31,7 @@
 namespace webrtc {
 
 namespace {
-class MockDataChannelObserver : public DataChannelSink {
+class MockDataChannelSink : public DataChannelSink {
  public:
   MOCK_METHOD(void, OnConnected, ());
 
@@ -45,6 +45,8 @@
   MOCK_METHOD(void, OnTransportClosed, (RTCError));
 };
 
+static_assert(!std::is_abstract_v<MockDataChannelSink>);
+
 class Peer {
  public:
   Peer() : fake_packet_transport_("transport"), simulated_clock_(1000) {
@@ -60,16 +62,15 @@
     sctp_transport_ = std::make_unique<webrtc::DcSctpTransport>(
         rtc::Thread::Current(), &fake_packet_transport_, &simulated_clock_,
         std::move(mock_dcsctp_socket_factory));
-    sctp_transport_->SetDataChannelSink(&observer_);
-    sctp_transport_->SetOnConnectedCallback(
-        [this]() { observer_.OnConnected(); });
+    sctp_transport_->SetDataChannelSink(&sink_);
+    sctp_transport_->SetOnConnectedCallback([this]() { sink_.OnConnected(); });
   }
 
   rtc::FakePacketTransport fake_packet_transport_;
   webrtc::SimulatedClock simulated_clock_;
   dcsctp::MockDcSctpSocket* socket_;
   std::unique_ptr<webrtc::DcSctpTransport> sctp_transport_;
-  NiceMock<MockDataChannelObserver> observer_;
+  NiceMock<MockDataChannelSink> sink_;
 };
 }  // namespace
 
@@ -82,8 +83,8 @@
       .Times(1)
       .WillOnce(Invoke(peer_a.sctp_transport_.get(),
                        &dcsctp::DcSctpSocketCallbacks::OnConnected));
-  EXPECT_CALL(peer_a.observer_, OnReadyToSend);
-  EXPECT_CALL(peer_a.observer_, OnConnected);
+  EXPECT_CALL(peer_a.sink_, OnReadyToSend);
+  EXPECT_CALL(peer_a.sink_, OnConnected);
 
   peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024);
 }
@@ -105,10 +106,10 @@
     EXPECT_CALL(*peer_b.socket_, ResetStreams(ElementsAre(dcsctp::StreamID(1))))
         .WillOnce(Return(dcsctp::ResetStreamsStatus::kPerformed));
 
-    EXPECT_CALL(peer_a.observer_, OnChannelClosing(1)).Times(0);
-    EXPECT_CALL(peer_b.observer_, OnChannelClosing(1));
-    EXPECT_CALL(peer_a.observer_, OnChannelClosed(1));
-    EXPECT_CALL(peer_b.observer_, OnChannelClosed(1));
+    EXPECT_CALL(peer_a.sink_, OnChannelClosing(1)).Times(0);
+    EXPECT_CALL(peer_b.sink_, OnChannelClosing(1));
+    EXPECT_CALL(peer_a.sink_, OnChannelClosed(1));
+    EXPECT_CALL(peer_b.sink_, OnChannelClosed(1));
   }
 
   peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024);
@@ -147,10 +148,10 @@
     EXPECT_CALL(*peer_b.socket_, ResetStreams(ElementsAre(dcsctp::StreamID(1))))
         .WillOnce(Return(dcsctp::ResetStreamsStatus::kPerformed));
 
-    EXPECT_CALL(peer_a.observer_, OnChannelClosing(1)).Times(0);
-    EXPECT_CALL(peer_b.observer_, OnChannelClosing(1)).Times(0);
-    EXPECT_CALL(peer_a.observer_, OnChannelClosed(1));
-    EXPECT_CALL(peer_b.observer_, OnChannelClosed(1));
+    EXPECT_CALL(peer_a.sink_, OnChannelClosing(1)).Times(0);
+    EXPECT_CALL(peer_b.sink_, OnChannelClosing(1)).Times(0);
+    EXPECT_CALL(peer_a.sink_, OnChannelClosed(1));
+    EXPECT_CALL(peer_b.sink_, OnChannelClosed(1));
   }
 
   peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024);
@@ -230,4 +231,33 @@
   EXPECT_EQ(cricket::SDR_SUCCESS, result);
 }
 
+TEST(DcSctpTransportTest, DeliversMessage) {
+  rtc::AutoThread main_thread;
+  Peer peer_a;
+
+  EXPECT_CALL(peer_a.sink_,
+              OnDataReceived(1, webrtc::DataMessageType::kBinary, _))
+      .Times(1);
+
+  peer_a.sctp_transport_->OpenStream(1);
+  peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024);
+
+  static_cast<dcsctp::DcSctpSocketCallbacks*>(peer_a.sctp_transport_.get())
+      ->OnMessageReceived(
+          dcsctp::DcSctpMessage(dcsctp::StreamID(1), dcsctp::PPID(53), {0}));
+}
+
+TEST(DcSctpTransportTest, DropMessageWithUnknownPpid) {
+  rtc::AutoThread main_thread;
+  Peer peer_a;
+
+  EXPECT_CALL(peer_a.sink_, OnDataReceived(_, _, _)).Times(0);
+
+  peer_a.sctp_transport_->OpenStream(1);
+  peer_a.sctp_transport_->Start(5000, 5000, 256 * 1024);
+
+  static_cast<dcsctp::DcSctpSocketCallbacks*>(peer_a.sctp_transport_.get())
+      ->OnMessageReceived(
+          dcsctp::DcSctpMessage(dcsctp::StreamID(1), dcsctp::PPID(1337), {0}));
+}
 }  // namespace webrtc