VP8Parser: Fix calculation of DCT partition sizes.

media/filters/vp8_parser.cc

Index: media/filters/vp8_parser.cc
diff --git a/media/filters/vp8_parser.cc b/media/filters/vp8_parser.cc
index 0c7739c910a65a4b3fdcdf15af23a148340028ef..727f31b55f4a566aeaa4438433d3fab75a55795c 100644
--- a/media/filters/vp8_parser.cc
+++ b/media/filters/vp8_parser.cc
@@ -793,24 +793,41 @@ bool Vp8Parser::ParsePartitions(Vp8FrameHeader* fhdr) {
   CHECK_GE(fhdr->num_of_dct_partitions, 1u);
   CHECK_LE(fhdr->num_of_dct_partitions, kMaxDCTPartitions);
 
-  // Jump to the beginning of the first dct partition.
-  size_t first_dct_pos = fhdr->first_part_offset + fhdr->first_part_size;
+  // DCT partitions start after the first partition and partition size values
+  // that follow it. There are num_of_dct_partitions - 1 sizes stored in the
+  // stream after the first partition, each 3 bytes long. The size of last
+  // DCT partition is not stored in the stream, but is instead calculated by
+  // taking the remainder of the frame size after the penultimate DCT partition.
+  size_t first_dct_pos = fhdr->first_part_offset + fhdr->first_part_size +
+                         (fhdr->num_of_dct_partitions - 1) * 3;
+
+  // Make sure we have enough data for the first partition and partition sizes.
   if (fhdr->frame_size < first_dct_pos)
     return false;
-  const uint8_t* ptr = fhdr->data + first_dct_pos;
+
+  // Total size of all DCT partitions.
   size_t bytes_left = fhdr->frame_size - first_dct_pos;
 
+  // Position ourselves at the beginning of partition size values.
+  const uint8_t* ptr =
+      fhdr->data + fhdr->first_part_offset + fhdr->first_part_size;

[Ville-Mikko Rautio, 2015/02/13 23:03:02]

To minimize the chance of confusion it might be worth considering of using
exclusively either full name (partition) or just the acronym (part) in each
field of Vp8FrameHeader when referring to partitions.

[Pawel Osciak, 2015/02/13 23:07:21]

So you mean change num_of_dct_partitions to num_of_dct_parts?

[Ville-Mikko Rautio, 2015/02/13 23:17:38]

Either that or change (first_part_offset, first_part_size) to
(first_partition_offset, first_partition_size).

[Pawel Osciak, 2015/02/13 23:20:39]

I see, makes sense. Given that this will have to be merged, I'll keep things as
is for now and consider this for later changes. Thanks.

+
+  // Read sizes from the stream (if present).
   for (size_t i = 0; i < fhdr->num_of_dct_partitions - 1; ++i) {
-    // Need 3 bytes at the beginning of the partition to read its size from.
-    if (bytes_left < 3)
+    fhdr->dct_partition_sizes[i] = (ptr[2] << 16) | (ptr[1] << 8) | ptr[0];

[Ville-Mikko Rautio, 2015/02/13 23:03:02]

IIUC ptr[i] (i>=0) might refer out of allocated bounds, if frame buffer ends for
some reason right after previous partition. After all, num_of_dct_partitions
might be even faulty. Restore the if to prevent this or otherwise make sure
there will be at least three bytes left to read from ptr at this stage.

[Pawel Osciak, 2015/02/13 23:07:21]

I check this at l.801 above. I check that frame_size is at least enough to get
us to the beginning of first DCT partition, which is after first partition and
after (fhdr->num_of_dct_partitions - 1) * 3 bytes of sizes. So if
num_of_dct_partitions is faulty, that should catch it. Unless I'm missing
something?

[Ville-Mikko Rautio, 2015/02/13 23:17:38]

You're correct. Also on line 821 you'll exit if there's not enough data for next
partition.

+
+    // Make sure we have enough data in the stream for ith partition and
+    // subtract its size from total.
+    if (bytes_left < fhdr->dct_partition_sizes[i])
       return false;
 
-    fhdr->dct_partition_sizes[i] = (ptr[2] << 16) | (ptr[1] << 8) | ptr[0];
+    bytes_left -= fhdr->dct_partition_sizes[i];
 
-    ptr += fhdr->dct_partition_sizes[i] + 3;
-    bytes_left -= fhdr->dct_partition_sizes[i] + 3;
+    // Move to the position of the next partition size value.
+    ptr += 3;
   }
 
+  // The remainder of the data belongs to the last DCT partition.
   fhdr->dct_partition_sizes[fhdr->num_of_dct_partitions - 1] = bytes_left;
 
   DVLOG(4) << "Control part size: " << fhdr->first_part_size;