Check frame coded size in H264 parsers to avoid integer overflows

content/common/gpu/media/h264_decoder.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
@@ skipping 1068 matching lines @@
     DVLOG(1) << "frame_mbs_only_flag != 1 not supported";
     return false;
   }
 
   // Calculate picture height/width in macroblocks and pixels
   // (spec 7.4.2.1.1, 7.4.3).
   int width_mb = sps->pic_width_in_mbs_minus1 + 1;
   int height_mb = (2 - sps->frame_mbs_only_flag) *
                   (sps->pic_height_in_map_units_minus1 + 1);
 
+  if (width_mb > std::numeric_limits<int>::max() / 16 ||

[Pawel Osciak, 2016/04/20 04:22:09]

Could we perhaps use:
base::IsValueInRangeForNumericType<int>(width_mb * 16) ?

[servolk, 2016/04/20 17:32:53]

Well, we are actually trying to avoid integer overflow when multiplying width_mb
by 16. So correct me if I'm missing something, but I think your suggested code
would do multiplication first (with potential integer overflow, since boths
multiplicands are ints), then would pass the multiplication result into
IsValueInRangeForNumericType, which would always be in range, since the overflow
has already happened. We could probably add an explicit widening cast on
width_mb before attempting multiplication, but that would result in uglier
(IMHO) code. I believe base::IsValueInRangeForNumericType is mostly useful in
situations where you are about to make a narrowing cast and want to ensure that
it's safe. This is not that situation.
Besides, I believe that dividing by 16 is optimized into a simple bit shift by
any decent modern compiler, so the performance shouldn't be an issue here.

[Pawel Osciak, 2016/04/21 01:03:53]

Yes, unless the result was of a larger type:
int a = INT_MAX; uint64_t b = a * 16; will not result in an overflow. I thought
that was how this template worked. In any case, saying something like:
base::IsValueInRangeForNumericType<int>(width_mb * 16u) should be enough.

[servolk, 2016/04/21 01:24:06]

First of all, you probably meant 16ull, as 16u is still the same bitness as int,
just unsigned.
I understand that we could work around this by explicitly widening one of the
arguments (or both). But to be honest, I don't understand why it would be any
better than the current code. Why would we resort to doing 128-bit
multiplication (on 64-bit platforms), when we could achieve the same goal with
64-bit arithmetic with the current code? How is it any better than dividing by
16, which is going to be a simple bitshift?

+      height_mb > std::numeric_limits<int>::max() / 16) {
+    DVLOG(1) << "Picture size is too big: width_mb=" << width_mb
+             << " height_mb=" << height_mb;
+    return false;
+  }
+
   gfx::Size new_pic_size(16 * width_mb, 16 * height_mb);
   if (new_pic_size.IsEmpty()) {
     DVLOG(1) << "Invalid picture size: " << new_pic_size.ToString();
     return false;
   }
 
   if (!pic_size_.IsEmpty() && new_pic_size == pic_size_) {
     // Already have surfaces and this SPS keeps the same resolution,
     // no need to request a new set.
     return true;
@@ skipping 315 matching lines @@
 
 gfx::Size H264Decoder::GetPicSize() const {
   return pic_size_;
 }
 
 size_t H264Decoder::GetRequiredNumOfPictures() const {
   return dpb_.max_num_pics() + kPicsInPipeline;
 }
 
 }  // namespace content