Avoid integer overflow errors when parsing Exp-Golomb codes

media/filters/h264_parser.cc

 // Copyright 2014 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 "media/filters/h264_parser.h"
+
+#include <limits>
+
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
-
 #include "media/base/decrypt_config.h"
-#include "media/filters/h264_parser.h"
 
 namespace media {
 
 bool H264SliceHeader::IsPSlice() const {
   return (slice_type % 5 == kPSlice);
 }
 
 bool H264SliceHeader::IsBSlice() const {
   return (slice_type % 5 == kBSlice);
 }
@@ skipping 289 matching lines @@
   // Count the number of contiguous zero bits.
   do {
     READ_BITS_OR_RETURN(1, &bit);
     num_bits++;
   } while (bit == 0);
 
   if (num_bits > 31)
     return kInvalidStream;
 
   // Calculate exp-Golomb code value of size num_bits.
+  // Special case for |num_bits| == 31 to avoid integer overflow. The only
+  // valid representation as an int is 2^31 - 1, so the remaining bits must
+  // be 0 or else the number is too large.
+  if (num_bits == 31) {
+    // TODO(jrummell): This file should be converted to int32_t as a lot of

[DaleCurtis, 2016/04/07 19:02:17]

I don't think this is necessary, but up to you. We also use int as int32_t in
chrome.

[jrummell, 2016/04/07 20:42:47]

happy to remove my name from a TODO.

+    // places assume that the sizeof(int) is 32 bits.
+    static_assert(sizeof(int) == sizeof(int32_t), "int must be 32 bits");
+
+    *val = std::numeric_limits<int>::max();
+    READ_BITS_OR_RETURN(num_bits, &rest);
+    return (rest == 0) ? kOk : kInvalidStream;
+  }
+
   *val = (1 << num_bits) - 1;

[DaleCurtis, 2016/04/07 02:08:23]

Is it sufficient to just have (1u << num_bits)

[sandersd (OOO until July 31), 2016/04/07 17:56:16]

It is correct to restrict to num_bits < 32, use entirely unsigned math, and then
verify at the end that the result is <= INT_MAX. It doesn't seem to make
anything simpler.

[DaleCurtis, 2016/04/07 19:02:17]

dalecurtis@xorax /tmp $ cat test.cc
#include <stdint.h>
int main(int argc, char* argv[]) {
  int32_t a = (1 << 31) - 1;
  return a;
}

dalecurtis@xorax /tmp $ clang -Wall test.cc
test.cc:4:25: warning: overflow in expression; result is 2147483647 with type
'int' [-Winteger-overflow]
  int32_t a = (1 << 31) - 1;
                        ^
1 warning generated.

Switching to 1u seems sufficient. I'd just move this calculation to l.323, and
delete lines l.327..l.331

[jrummell, 2016/04/07 20:42:47]

Done.

-
   if (num_bits > 0) {
     READ_BITS_OR_RETURN(num_bits, &rest);
     *val += rest;
   }
 
   return kOk;
 }
 
 H264Parser::Result H264Parser::ReadSE(int* val) {
   int ue;
@@ skipping 1005 matching lines @@
 
     default:
       DVLOG(4) << "Unsupported SEI message";
       break;
   }
 
   return kOk;
 }
 
 }  // namespace media