[C++] fix bounds checking on integer parsing (#4250)

* fix bounds checking on integer parsing

the previous code was allowing 255 for int8_t, similar for int16_t
and int32_t, and even negative values for unsignd types.

this patch fixes bounds checking for 8-bit, 16-bit and 32-bit types.
testing for both acceptable values and unacceptable values at the
boundaries are also improved.

bounds checking on 64-bit types isn't addressed by this patch.

* fix 'unary minus operator applied to unsigned type, result still unsigned'

* fix & placement

include/flatbuffers/idl.h

@@ -530,7 +530,7 @@ class Parser : public ParserState {
   // of the schema provided. Returns non-empty error on any problems.
   std::string ConformTo(const Parser &base);
 
-  FLATBUFFERS_CHECKED_ERROR CheckBitsFit(int64_t val, size_t bits);
+  FLATBUFFERS_CHECKED_ERROR CheckInRange(int64_t val, int64_t min, int64_t max);
 
 private:
   FLATBUFFERS_CHECKED_ERROR Error(const std::string &msg);

src/idl_parser.cpp

@@ -86,26 +86,29 @@ CheckedError Parser::Error(const std::string &msg) {
 
 inline CheckedError NoError() { return CheckedError(false); }
 
+inline std::string OutOfRangeErrorMsg(int64_t val, const std::string &op,
+                                      int64_t limit) {
+  const std::string cause = NumToString(val) + op + NumToString(limit);
+  return "constant does not fit (" + cause + ")";
+}
+
 // Ensure that integer values we parse fit inside the declared integer type.
-CheckedError Parser::CheckBitsFit(int64_t val, size_t bits) {
-  // Left-shifting a 64-bit value by 64 bits or more is undefined
-  // behavior (C99 6.5.7), so check *before* we shift.
-  if (bits < 64) {
-    // Bits we allow to be used.
-    auto mask = static_cast<int64_t>((1ull << bits) - 1);
-    if ((val & ~mask) != 0 &&  // Positive or unsigned.
-        (val |  mask) != -1)   // Negative.
-      return Error("constant does not fit in a " + NumToString(bits) +
-                   "-bit field");
-  }
-  return NoError();
+CheckedError Parser::CheckInRange(int64_t val, int64_t min, int64_t max) {
+  if (val < min)
+    return Error(OutOfRangeErrorMsg(val, " < ", min));
+  else if (val > max)
+    return Error(OutOfRangeErrorMsg(val, " > ", max));
+  else
+    return NoError();
 }
 
 // atot: templated version of atoi/atof: convert a string to an instance of T.
 template<typename T> inline CheckedError atot(const char *s, Parser &parser,
                                               T *val) {
   int64_t i = StringToInt(s);
-  ECHECK(parser.CheckBitsFit(i, sizeof(T) * 8));
+  const int64_t min = std::numeric_limits<T>::min();
+  const int64_t max = std::numeric_limits<T>::max();
+  ECHECK(parser.CheckInRange(i, min, max));
   *val = (T)i;
   return NoError();
 }

tests/test.cpp

@@ -981,7 +981,7 @@ void TestError(const char *src, const char *error_substr,
 // Also useful for coverage, making sure these paths are run.
 void ErrorTest() {
   // In order they appear in idl_parser.cpp
-  TestError("table X { Y:byte; } root_type X; { Y: 999 }", "bit field");
+  TestError("table X { Y:byte; } root_type X; { Y: 999 }", "does not fit");
   TestError(".0", "floating point");
   TestError("\"\0", "illegal");
   TestError("\"\\q", "escape code");
@@ -1069,32 +1069,51 @@ void EnumStringsTest() {
 }
 
 void IntegerOutOfRangeTest() {
-  TestError("table T { F:byte; } root_type T; { F:256 }",
+  TestError("table T { F:byte; } root_type T; { F:128 }",
             "constant does not fit");
-  TestError("table T { F:byte; } root_type T; { F:-257 }",
+  TestError("table T { F:byte; } root_type T; { F:-129 }",
             "constant does not fit");
   TestError("table T { F:ubyte; } root_type T; { F:256 }",
             "constant does not fit");
-  TestError("table T { F:ubyte; } root_type T; { F:-257 }",
+  TestError("table T { F:ubyte; } root_type T; { F:-1 }",
             "constant does not fit");
-  TestError("table T { F:short; } root_type T; { F:65536 }",
+  TestError("table T { F:short; } root_type T; { F:32768 }",
             "constant does not fit");
-  TestError("table T { F:short; } root_type T; { F:-65537 }",
+  TestError("table T { F:short; } root_type T; { F:-32769 }",
             "constant does not fit");
   TestError("table T { F:ushort; } root_type T; { F:65536 }",
             "constant does not fit");
-  TestError("table T { F:ushort; } root_type T; { F:-65537 }",
+  TestError("table T { F:ushort; } root_type T; { F:-1 }",
             "constant does not fit");
-  TestError("table T { F:int; } root_type T; { F:4294967296 }",
+  TestError("table T { F:int; } root_type T; { F:2147483648 }",
             "constant does not fit");
-  TestError("table T { F:int; } root_type T; { F:-4294967297 }",
+  TestError("table T { F:int; } root_type T; { F:-2147483649 }",
             "constant does not fit");
   TestError("table T { F:uint; } root_type T; { F:4294967296 }",
             "constant does not fit");
-  TestError("table T { F:uint; } root_type T; { F:-4294967297 }",
+  TestError("table T { F:uint; } root_type T; { F:-1 }",
             "constant does not fit");
 }
 
+void IntegerBoundaryTest() {
+  TEST_EQ(TestValue<int8_t>("{ Y:127 }","byte"), 127);
+  TEST_EQ(TestValue<int8_t>("{ Y:-128 }","byte"), -128);
+  TEST_EQ(TestValue<uint8_t>("{ Y:255 }","ubyte"), 255);
+  TEST_EQ(TestValue<uint8_t>("{ Y:0 }","ubyte"), 0);
+  TEST_EQ(TestValue<int16_t>("{ Y:32767 }","short"), 32767);
+  TEST_EQ(TestValue<int16_t>("{ Y:-32768 }","short"), -32768);
+  TEST_EQ(TestValue<uint16_t>("{ Y:65535 }","ushort"), 65535);
+  TEST_EQ(TestValue<uint16_t>("{ Y:0 }","ushort"), 0);
+  TEST_EQ(TestValue<int32_t>("{ Y:2147483647 }","int"), 2147483647);
+  TEST_EQ(TestValue<int32_t>("{ Y:-2147483648 }","int"), (-2147483647 - 1));
+  TEST_EQ(TestValue<uint32_t>("{ Y:4294967295 }","uint"), 4294967295);
+  TEST_EQ(TestValue<uint32_t>("{ Y:0 }","uint"), 0);
+  TEST_EQ(TestValue<int64_t>("{ Y:9223372036854775807 }","long"), 9223372036854775807);
+  TEST_EQ(TestValue<int64_t>("{ Y:-9223372036854775808 }","long"), (-9223372036854775807 - 1));
+  TEST_EQ(TestValue<uint64_t>("{ Y:18446744073709551615 }","ulong"), 18446744073709551615U);
+  TEST_EQ(TestValue<uint64_t>("{ Y:0 }","ulong"), 0);
+}
+
 void UnicodeTest() {
   flatbuffers::Parser parser;
   // Without setting allow_non_utf8 = true, we treat \x sequences as byte sequences
@@ -1515,6 +1534,7 @@ int main(int /*argc*/, const char * /*argv*/[]) {
   ValueTest();
   EnumStringsTest();
   IntegerOutOfRangeTest();
+  IntegerBoundaryTest();
   UnicodeTest();
   UnicodeTestAllowNonUTF8();
   UnicodeTestGenerateTextFailsOnNonUTF8();