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

Index: webrtc/modules/video_coding/packet_buffer.cc
diff --git a/webrtc/modules/video_coding/packet_buffer.cc b/webrtc/modules/video_coding/packet_buffer.cc
index 8b6de047ca5bb6aa3e7f542dacdd8f48cf6c5559..5107c80ca35a017476e22656086753502aa12f93 100644
--- a/webrtc/modules/video_coding/packet_buffer.cc
+++ b/webrtc/modules/video_coding/packet_buffer.cc
@@ -251,14 +251,14 @@ std::vector<std::unique_ptr<RtpFrameObject>> PacketBuffer::FindFrames(
       // 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);
@@ -278,6 +278,9 @@ std::vector<std::unique_ptr<RtpFrameObject>> PacketBuffer::FindFrames(
           }
         }
 
+        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,
@@ -345,19 +348,26 @@ bool PacketBuffer::GetBitstream(const RtpFrameObject& frame,
   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;
     }
 
-    const uint8_t* source = data_buffer_[index].dataPtr;
     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;
     memcpy(destination, source, length);
     destination += length;
     index = (index + 1) % size_;
     ++seq_num;
-  }
+  } while (index != end);
+
   return true;
 }