Prevent OOB reads for truncated H264 STAP-A packets.

webrtc/modules/video_coding/main/source/session_info.cc

Index: webrtc/modules/video_coding/main/source/session_info.cc
diff --git a/webrtc/modules/video_coding/main/source/session_info.cc b/webrtc/modules/video_coding/main/source/session_info.cc
index 8eba432643ce0164614f9bc82c412c399efbeaaa..744e1e63c0bcd4f6e7e56da7337e23c5883eb5ed 100644
--- a/webrtc/modules/video_coding/main/source/session_info.cc
+++ b/webrtc/modules/video_coding/main/source/session_info.cc
@@ -116,9 +116,10 @@ int VCMSessionInfo::NumPackets() const {
   return packets_.size();
 }
 
-size_t VCMSessionInfo::InsertBuffer(uint8_t* frame_buffer,
-                                    PacketIterator packet_it) {
-  VCMPacket& packet = *packet_it;
+bool VCMSessionInfo::InsertBuffer(uint8_t* frame_buffer,
+                                  size_t* inserted_length,
+                                  PacketIterator packet_it) {
+  VCMPacket* packet = &(*packet_it);
   PacketIterator it;
 
   // Calculate the offset into the frame buffer for this packet.
@@ -128,49 +129,52 @@ size_t VCMSessionInfo::InsertBuffer(uint8_t* frame_buffer,
 
   // Set the data pointer to pointing to the start of this packet in the
   // frame buffer.
-  const uint8_t* packet_buffer = packet.dataPtr;
-  packet.dataPtr = frame_buffer + offset;
+  const uint8_t* packet_buffer = packet->dataPtr;
+  packet->dataPtr = frame_buffer + offset;
 
   // We handle H.264 STAP-A packets in a special way as we need to remove the
   // two length bytes between each NAL unit, and potentially add start codes.
   const size_t kH264NALHeaderLengthInBytes = 1;
   const size_t kLengthFieldLength = 2;
-  if (packet.codecSpecificHeader.codec == kRtpVideoH264 &&
-      packet.codecSpecificHeader.codecHeader.H264.packetization_type ==
+  if (packet->codecSpecificHeader.codec == kRtpVideoH264 &&
+      packet->codecSpecificHeader.codecHeader.H264.packetization_type ==
           kH264StapA) {
-    size_t required_length = 0;
+    size_t required_length = kH264NALHeaderLengthInBytes;
     const uint8_t* nalu_ptr = packet_buffer + kH264NALHeaderLengthInBytes;
-    while (nalu_ptr < packet_buffer + packet.sizeBytes) {
+    while (nalu_ptr < packet_buffer + packet->sizeBytes) {
       size_t length = BufferToUWord16(nalu_ptr);
       required_length +=
-          length + (packet.insertStartCode ? kH264StartCodeLengthBytes : 0);
+          length + (packet->insertStartCode ? kH264StartCodeLengthBytes : 0);

[stefan-webrtc, 2015/07/23 12:36:04]

As discussed, the problem is that this start code overflows the packet size. We
have to take into account the number of nal units when allocating the frame
buffer size in frame_buffer.cc.

       nalu_ptr += kLengthFieldLength + length;
     }
-    ShiftSubsequentPackets(packet_it, required_length);
+    if (required_length > packet->sizeBytes)
+      return false;
+    ShiftSubsequentPackets(packet_it,
+                           required_length - kH264NALHeaderLengthInBytes);
     nalu_ptr = packet_buffer + kH264NALHeaderLengthInBytes;
     uint8_t* frame_buffer_ptr = frame_buffer + offset;
-    while (nalu_ptr < packet_buffer + packet.sizeBytes) {
+    while (nalu_ptr < packet_buffer + packet->sizeBytes) {
       size_t length = BufferToUWord16(nalu_ptr);
       nalu_ptr += kLengthFieldLength;
-      frame_buffer_ptr += Insert(nalu_ptr,
-                                 length,
-                                 packet.insertStartCode,
-                                 const_cast<uint8_t*>(frame_buffer_ptr));
+      frame_buffer_ptr +=
+          Insert(nalu_ptr, length, packet->insertStartCode, frame_buffer_ptr);
       nalu_ptr += length;
     }
-    packet.sizeBytes = required_length;
-    return packet.sizeBytes;
+    packet->sizeBytes = required_length - kH264NALHeaderLengthInBytes;

[pbos-webrtc, 2015/07/17 14:08:53]

Was this part (not including kH264NALHeaderLengthInBytes) correct before? This
should be a no-op now since kH264NALHeaderLengthInBytes is added at line 142.

+    *inserted_length = packet->sizeBytes;
+    return true;
   }
   ShiftSubsequentPackets(
       packet_it,
-      packet.sizeBytes +
-          (packet.insertStartCode ? kH264StartCodeLengthBytes : 0));
+      packet->sizeBytes +
+          (packet->insertStartCode ? kH264StartCodeLengthBytes : 0));
 
-  packet.sizeBytes = Insert(packet_buffer,
-                            packet.sizeBytes,
-                            packet.insertStartCode,
-                            const_cast<uint8_t*>(packet.dataPtr));
-  return packet.sizeBytes;
+  packet->sizeBytes = Insert(packet_buffer,
+                            packet->sizeBytes,
+                            packet->insertStartCode,
+                            const_cast<uint8_t*>(packet->dataPtr));
+  *inserted_length = packet->sizeBytes;
+  return true;
 }
 
 size_t VCMSessionInfo::Insert(const uint8_t* buffer,
@@ -513,12 +517,15 @@ int VCMSessionInfo::InsertPacket(const VCMPacket& packet,
   // The insert operation invalidates the iterator |rit|.
   PacketIterator packet_list_it = packets_.insert(rit.base(), packet);
 
-  size_t returnLength = InsertBuffer(frame_buffer, packet_list_it);
+  size_t returnLength;
+  if (!InsertBuffer(frame_buffer, &returnLength, packet_list_it))
+    return kSizeError;
   UpdateCompleteSession();
-  if (decode_error_mode == kWithErrors)
+  if (decode_error_mode == kWithErrors) {
     decodable_ = true;
-  else if (decode_error_mode == kSelectiveErrors)
+  } else if (decode_error_mode == kSelectiveErrors) {
     UpdateDecodableSession(frame_data);
+  }
   return static_cast<int>(returnLength);
 }