Avoid integer overflow errors when parsing Exp-Golomb codes

media/filters/h264_parser.cc

Index: media/filters/h264_parser.cc
diff --git a/media/filters/h264_parser.cc b/media/filters/h264_parser.cc
index d751de77fcaa06ebdabee2ca130385189b2fc80b..5b10d7bcbaae241139a4fafdeb2b4607717af264 100644
--- a/media/filters/h264_parser.cc
+++ b/media/filters/h264_parser.cc
@@ -2,13 +2,15 @@
 // 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 {
 
@@ -318,8 +320,20 @@ H264Parser::Result H264Parser::ReadUE(int* val) {
     return kInvalidStream;
 
   // Calculate exp-Golomb code value of size num_bits.
-  *val = (1 << num_bits) - 1;
+  // 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;