From 60ffc31ae14709a5592510a4af8917472fe421cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= <sprang@webrtc.org>
Date: Tue, 30 Jul 2019 22:03:49 +0200
Subject: [PATCH] Fix potential crash if nack is being processed when media
 gets disabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is a race that can happen if a nack arrives before media is
disabled, but the packet is not processed until after the disabling
is complete.

Bug: webrtc:10633, b/138636698
Change-Id: Ic90462b815163ab58c324e5cdb95c8d199c0b772
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147277
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28718}
---
 modules/rtp_rtcp/source/rtp_sender.cc         |  6 ++-
 .../rtp_rtcp/source/rtp_sender_unittest.cc    | 54 ++++++++++++++++++-
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc
index 4ba645fea2..c381ce2fdd 100644
--- a/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -620,8 +620,10 @@ int32_t RTPSender::ReSendPacket(uint16_t packet_id) {
                   retransmit_packet =
                       absl::make_unique<RtpPacketToSend>(stored_packet);
                 }
-                retransmit_packet->set_retransmitted_sequence_number(
-                    stored_packet.SequenceNumber());
+                if (retransmit_packet) {
+                  retransmit_packet->set_retransmitted_sequence_number(
+                      stored_packet.SequenceNumber());
+                }
                 return retransmit_packet;
               });
       if (!packet) {
diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
index 2875d3dec1..dd36dc29dd 100644
--- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc
@@ -278,6 +278,7 @@ class RtpSenderTest : public ::testing::TestWithParam<TestConfig> {
                                                   int64_t capture_time_ms) {
     auto packet = rtp_sender_->AllocatePacket();
     packet->SetPayloadType(payload_type);
+    packet->set_packet_type(RtpPacketToSend::Type::kVideo);
     packet->SetMarker(marker_bit);
     packet->SetTimestamp(timestamp);
     packet->set_capture_time_ms(capture_time_ms);
@@ -294,8 +295,7 @@ class RtpSenderTest : public ::testing::TestWithParam<TestConfig> {
 
     // Packet should be stored in a send bucket.
     EXPECT_TRUE(rtp_sender_->SendToNetwork(
-        absl::make_unique<RtpPacketToSend>(*packet), kAllowRetransmission,
-        RtpPacketSender::kNormalPriority));
+        absl::make_unique<RtpPacketToSend>(*packet), kAllowRetransmission));
     return packet;
   }
 
@@ -2943,6 +2943,56 @@ TEST_P(RtpSenderTestWithoutPacer, ClearHistoryOnSequenceNumberCange) {
   EXPECT_EQ(rtp_sender_->ReSendPacket(packet_seqence_number), 0);
 }
 
+TEST_P(RtpSenderTest, IgnoresNackAfterDisablingMedia) {
+  const int64_t kRtt = 10;
+
+  rtp_sender_->SetSendingMediaStatus(true);
+  rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads);
+  rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload);
+  rtp_sender_->SetStorePacketsStatus(true, 10);
+  rtp_sender_->SetRtt(kRtt);
+
+  // Send a packet so it is in the packet history.
+  if (GetParam().pacer_references_packets) {
+    EXPECT_CALL(mock_paced_sender_, InsertPacket);
+    SendGenericPacket();
+    rtp_sender_->TimeToSendPacket(kSsrc, kSeqNum,
+                                  fake_clock_.TimeInMilliseconds(), false,
+                                  PacedPacketInfo());
+    ASSERT_EQ(1u, transport_.sent_packets_.size());
+
+    // Disable media sending and try to retransmit the packet, it should be put
+    // in the pacer queue.
+    rtp_sender_->SetSendingMediaStatus(false);
+    fake_clock_.AdvanceTimeMilliseconds(kRtt);
+    EXPECT_CALL(mock_paced_sender_, InsertPacket);
+    EXPECT_GT(rtp_sender_->ReSendPacket(kSeqNum), 0);
+
+    // Time to send the retransmission. It should fail and the send packet
+    // counter should not increase.
+    rtp_sender_->TimeToSendPacket(kSsrc, kSeqNum,
+                                  fake_clock_.TimeInMilliseconds(), true,
+                                  PacedPacketInfo());
+    ASSERT_EQ(1u, transport_.sent_packets_.size());
+  } else {
+    std::unique_ptr<RtpPacketToSend> packet_to_pace;
+    EXPECT_CALL(mock_paced_sender_, EnqueuePacket)
+        .WillOnce([&](std::unique_ptr<RtpPacketToSend> packet) {
+          packet_to_pace = std::move(packet);
+        });
+
+    SendGenericPacket();
+    rtp_sender_->TrySendPacket(packet_to_pace.get(), PacedPacketInfo());
+
+    ASSERT_EQ(1u, transport_.sent_packets_.size());
+
+    // Disable media sending and try to retransmit the packet, it should fail.
+    rtp_sender_->SetSendingMediaStatus(false);
+    fake_clock_.AdvanceTimeMilliseconds(kRtt);
+    EXPECT_LT(rtp_sender_->ReSendPacket(kSeqNum), 0);
+  }
+}
+
 INSTANTIATE_TEST_SUITE_P(WithAndWithoutOverhead,
                          RtpSenderTest,
                          ::testing::Values(TestConfig{false, false},