Check frame coded size in H264 parsers to avoid integer overflows

content/common/gpu/media/h264_decoder.cc

Index: content/common/gpu/media/h264_decoder.cc
diff --git a/content/common/gpu/media/h264_decoder.cc b/content/common/gpu/media/h264_decoder.cc
index 46ffa2a45085f0b554898ad149ef551696e38bc9..f8504f3c53cec01b970b2c8df04b13036a669172 100644
--- a/content/common/gpu/media/h264_decoder.cc
+++ b/content/common/gpu/media/h264_decoder.cc
@@ -1086,6 +1086,13 @@ bool H264Decoder::ProcessSPS(int sps_id, bool* need_new_buffers) {
   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();