Fix off-by-one bugs in video_coding::PacketBuffer when the buffer is filled with a single frame.

webrtc/modules/video_coding/packet_buffer.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 233 matching lines @@
     // If all packets of the frame is continuous, find the first packet of the
     // frame and create an RtpFrameObject.
     if (sequence_buffer_[index].frame_end) {
       size_t frame_size = 0;
       int max_nack_count = -1;
       uint16_t start_seq_num = seq_num;
 
       // Find the start index by searching backward until the packet with
       // the |frame_begin| flag is set.
       int start_index = index;
+      size_t tested_packets = 0;
 
       bool is_h264 = data_buffer_[start_index].codec == kVideoCodecH264;
       bool is_h264_keyframe = false;
       int64_t frame_timestamp = data_buffer_[start_index].timestamp;
 
-      // Since packet at |data_buffer_[index]| is already part of the frame
-      // we will have at most |size_ - 1| packets left to check.

[stefan-webrtc, 2017/08/01 11:24:17]

Was this comment not correct? Looks like we have changed it to check size_
packets instead of size_ - 1.

[philipel, 2017/08/01 11:51:23]

It was semi-correct. In this version |start_seq_num| was decremented at the end
of the loop, and when we exited the loop due to the loop condition it was in the
correct state. The problem was that we did not execute line 262-265 for the last
(actually the first in frame) packet.  This means the size of that packet was
not added to the total size of the frame.

-      for (size_t j = 0; j < size_ - 1; ++j) {
+      while (true) {
+        ++tested_packets;
         frame_size += data_buffer_[start_index].sizeBytes;
         max_nack_count =
             std::max(max_nack_count, data_buffer_[start_index].timesNacked);
         sequence_buffer_[start_index].frame_created = true;
 
         if (!is_h264 && sequence_buffer_[start_index].frame_begin)
           break;
 
         if (is_h264 && !is_h264_keyframe) {
           const RTPVideoHeaderH264& header =
               data_buffer_[start_index].video_header.codecHeader.H264;
           for (size_t i = 0; i < header.nalus_length; ++i) {
             if (header.nalus[i].type == H264::NaluType::kIdr) {
               is_h264_keyframe = true;
               break;
             }
           }
         }
 
+        if (tested_packets == size_)
+          break;
+
         start_index = start_index > 0 ? start_index - 1 : size_ - 1;
 
         // In the case of H264 we don't have a frame_begin bit (yes,
         // |frame_begin| might be set to true but that is a lie). So instead
         // we traverese backwards as long as we have a previous packet and
         // the timestamp of that packet is the same as this one. This may cause
         // the PacketBuffer to hand out incomplete frames.
         // See: https://bugs.chromium.org/p/webrtc/issues/detail?id=7106
         if (is_h264 &&
             (!sequence_buffer_[start_index].used ||
@@ skipping 47 matching lines @@
   }
 }
 
 bool PacketBuffer::GetBitstream(const RtpFrameObject& frame,
                                 uint8_t* destination) {
   rtc::CritScope lock(&crit_);
 
   size_t index = frame.first_seq_num() % size_;
   size_t end = (frame.last_seq_num() + 1) % size_;
   uint16_t seq_num = frame.first_seq_num();
-  while (index != end) {
+  uint8_t* destination_end = destination + frame.size();
+
+  do {
     if (!sequence_buffer_[index].used ||
-        sequence_buffer_[index].seq_num != seq_num) {
+        sequence_buffer_[index].seq_num != seq_num ||
+        data_buffer_[index].seqNum != seq_num) {

[stefan-webrtc, 2017/08/01 11:24:17]

What does this mean? That the frame doesn't have all packets? Should that happen
here?

[philipel, 2017/08/01 11:51:23]

It should not happen, no. I'll change it to a DCHECK.

[stefan-webrtc, 2017/08/01 12:06:22]

Is it safe to CHECK, or is it possible to trigger with an intentionally broken
bitstream? What happens if it's triggered? Maybe we should have a DCHECK and
catch it with an if statement?

[philipel, 2017/08/01 13:00:05]

It should not be possible to have the sequence_buffer_[index].seq_num  !=
data_buffer_[index].seqNum.

Changed it to a DCHECK.

[stefan-webrtc, 2017/08/01 15:14:52]

My question was if it should be a CHECK instead. But maybe not? Depends on how
bad it would be if it actually happened... :)

       return false;
     }
 
+    size_t length = data_buffer_[index].sizeBytes;
+    if (destination + length > destination_end)
+      return false;

[stefan-webrtc, 2017/08/01 11:24:17]

Should we log that the frame is too large for the decode buffer? Should that
even be possible?

[philipel, 2017/08/01 11:51:23]

This check is not really related to the bug at hand, it's just a check I added
to avoid writing OOB, which can make many people sad.

[stefan-webrtc, 2017/08/01 12:06:22]

Right, but it means that the frame is too large. We probably want to know that,
so maybe we should log at least?

[philipel, 2017/08/01 13:00:05]

Added warning if this happens.

+
     const uint8_t* source = data_buffer_[index].dataPtr;
-    size_t length = data_buffer_[index].sizeBytes;
     memcpy(destination, source, length);
     destination += length;
     index = (index + 1) % size_;
     ++seq_num;
-  }
+  } while (index != end);
+
   return true;
 }
 
 VCMPacket* PacketBuffer::GetPacket(uint16_t seq_num) {
   size_t index = seq_num % size_;
   if (!sequence_buffer_[index].used ||
       seq_num != sequence_buffer_[index].seq_num) {
     return nullptr;
   }
   return &data_buffer_[index];
@@ skipping 31 matching lines @@
       missing_packets_.insert(*newest_inserted_seq_num_);
       ++*newest_inserted_seq_num_;
     }
   } else {
     missing_packets_.erase(seq_num);
   }
 }
 
 }  // namespace video_coding
 }  // namespace webrtc