NetEq: Fixing a bug that caused rtc::checked_cast to trigger

This is a bug that was introduced in
https://codereview.webrtc.org/1230503003, where the variable "int
temp_bufsize" was changed to a size_t. If the packet buffer was
flushed while inserting a packet, temp_bufsize became negative, which
was tested later in an if-statement. Now, with size_t instead, it
would just become very large, and the if-statement would never see a
negative value. The effect was that the packet size in samples could
be updated with a very large positive number, causing an overflow
which triggered rtc::checked_cast in
StatisticsCalculator::GetNetworkStatistics, line 220.

Also adding a test to reproduce the crash. Without the fix, the test
results in the above mentioned checked_cast to trigger. With the fix,
everything works fine.

BUG=chromium:525260

Review URL: https://codereview.webrtc.org/1307893004

Cr-Commit-Position: refs/heads/master@{#9802}

webrtc/modules/audio_coding/neteq/neteq_impl.cc

@@ -612,7 +612,8 @@ int NetEqImpl::InsertPacketInternal(const WebRtcRTPHeader& rtp_header,
   }
 
   // Insert packets in buffer.
-  size_t temp_bufsize = packet_buffer_->NumPacketsInBuffer();
+  const size_t buffer_length_before_insert =
+      packet_buffer_->NumPacketsInBuffer();
   ret = packet_buffer_->InsertPacketList(
       &packet_list,
       *decoder_database_,
@@ -668,14 +669,18 @@ int NetEqImpl::InsertPacketInternal(const WebRtcRTPHeader& rtp_header,
   delay_manager_->LastDecoderType(dec_info->codec_type);
   if (delay_manager_->last_pack_cng_or_dtmf() == 0) {
     // Calculate the total speech length carried in each packet.
-    temp_bufsize = packet_buffer_->NumPacketsInBuffer() - temp_bufsize;
-    temp_bufsize *= decoder_frame_length_;
+    const size_t buffer_length_after_insert =
+        packet_buffer_->NumPacketsInBuffer();
 
-    if ((temp_bufsize > 0) &&
-        (temp_bufsize != decision_logic_->packet_length_samples())) {
-      decision_logic_->set_packet_length_samples(temp_bufsize);
-      delay_manager_->SetPacketAudioLength(
-          static_cast<int>((1000 * temp_bufsize) / fs_hz_));
+    if (buffer_length_after_insert > buffer_length_before_insert) {
+      const size_t packet_length_samples =
+          (buffer_length_after_insert - buffer_length_before_insert) *
+          decoder_frame_length_;
+      if (packet_length_samples != decision_logic_->packet_length_samples()) {
+        decision_logic_->set_packet_length_samples(packet_length_samples);
+        delay_manager_->SetPacketAudioLength(
+            rtc::checked_cast<int>((1000 * packet_length_samples) / fs_hz_));
+      }
     }
 
     // Update statistics.

webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc

@@ -13,6 +13,7 @@
 
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/base/safe_conversions.h"
 #include "webrtc/modules/audio_coding/neteq/accelerate.h"
 #include "webrtc/modules/audio_coding/neteq/expand.h"
 #include "webrtc/modules/audio_coding/neteq/mock/mock_audio_decoder.h"
@@ -902,4 +903,43 @@ TEST_F(NetEqImplTest, UnsupportedDecoder) {
   EXPECT_EQ(kChannels, num_channels);
 }
 
+// This test inserts packets until the buffer is flushed. After that, it asks
+// NetEq for the network statistics. The purpose of the test is to make sure
+// that even though the buffer size increment is negative (which it becomes when
+// the packet causing a flush is inserted), the packet length stored in the
+// decision logic remains valid.
+TEST_F(NetEqImplTest, FloodBufferAndGetNetworkStats) {
+  UseNoMocks();
+  CreateInstance();
+
+  const size_t kPayloadLengthSamples = 80;
+  const size_t kPayloadLengthBytes = 2 * kPayloadLengthSamples;  // PCM 16-bit.
+  const uint8_t kPayloadType = 17;   // Just an arbitrary number.
+  const uint32_t kReceiveTime = 17;  // Value doesn't matter for this test.
+  uint8_t payload[kPayloadLengthBytes] = {0};
+  WebRtcRTPHeader rtp_header;
+  rtp_header.header.payloadType = kPayloadType;
+  rtp_header.header.sequenceNumber = 0x1234;
+  rtp_header.header.timestamp = 0x12345678;
+  rtp_header.header.ssrc = 0x87654321;
+
+  EXPECT_EQ(NetEq::kOK,
+            neteq_->RegisterPayloadType(kDecoderPCM16B, kPayloadType));
+
+  // Insert packets until the buffer flushes.
+  for (size_t i = 0; i <= config_.max_packets_in_buffer; ++i) {
+    EXPECT_EQ(i, packet_buffer_->NumPacketsInBuffer());
+    EXPECT_EQ(NetEq::kOK,
+              neteq_->InsertPacket(rtp_header, payload, kPayloadLengthBytes,
+                                   kReceiveTime));
+    rtp_header.header.timestamp +=
+        rtc::checked_cast<uint32_t>(kPayloadLengthSamples);
+    ++rtp_header.header.sequenceNumber;
+  }
+  EXPECT_EQ(1u, packet_buffer_->NumPacketsInBuffer());
+
+  // Ask for network statistics. This should not crash.
+  NetEqNetworkStatistics stats;
+  EXPECT_EQ(NetEq::kOK, neteq_->NetworkStatistics(&stats));
+}
 }  // namespace webrtc