[llvm] d8f4f10 - [llvm][json] Fix UINT64 json parsing

[Walter Erquinigo via llvm-commits]

Author: Walter Erquinigo
Date: 2022-05-17T09:11:45-07:00
New Revision: d8f4f1027a92883067ecd4b01030484cffeb24d3

URL: https://github.com/llvm/llvm-project/commit/d8f4f1027a92883067ecd4b01030484cffeb24d3
DIFF: https://github.com/llvm/llvm-project/commit/d8f4f1027a92883067ecd4b01030484cffeb24d3.diff

LOG: [llvm][json] Fix UINT64 json parsing

https://reviews.llvm.org/D109347 added support for UINT64 json numeric
types. However, it seems that it didn't properly test uint64_t numbers
larger than the int64_t because the number parsing logic doesn't
have any special handling for these large numbers.

This diffs adds a handler for large numbers, and besides that, fixes the
parsing of signed types by checking for errno ERANGE, which is the
recommended way to check if parsing fails because of out of bounds
errors. Before this diff, strtoll was always returning a number within
the bounds of an int64_t and the bounds check it was doing was completely
superfluous.

As an interesting fact about the old implementation, when calling strtoll
with "18446744073709551615", the largest uint64_t, End was S.end(), even
though it didn't use all digits. Which means that this check can only be
used to identify if the numeric string is malformed or not.

This patch also adds additional tests for extreme cases.

Differential Revision: https://reviews.llvm.org/D125322

Added: 
    

Modified: 
    llvm/lib/Support/JSON.cpp
    llvm/unittests/Support/JSONTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Support/JSON.cpp b/llvm/lib/Support/JSON.cpp
index 20babbe56d861..b87e39f0a9633 100644
--- a/llvm/lib/Support/JSON.cpp
+++ b/llvm/lib/Support/JSON.cpp
@@ -509,13 +509,25 @@ bool Parser::parseNumber(char First, Value &Out) {
     S.push_back(next());
   char *End;
   // Try first to parse as integer, and if so preserve full 64 bits.
-  // strtoll returns long long >= 64 bits, so check it's in range too.
-  auto I = std::strtoll(S.c_str(), &End, 10);
-  if (End == S.end() && I >= std::numeric_limits<int64_t>::min() &&
-      I <= std::numeric_limits<int64_t>::max()) {
+  // We check for errno for out of bounds errors and for End == S.end()
+  // to make sure that the numeric string is not malformed.
+  errno = 0;
+  int64_t I = std::strtoll(S.c_str(), &End, 10);
+  if (End == S.end() && errno != ERANGE) {
     Out = int64_t(I);
     return true;
   }
+  // strtroull has a special handling for negative numbers, but in this
+  // case we don't want to do that because negative numbers were already
+  // handled in the previous block.
+  if (First != '-') {
+    errno = 0;
+    uint64_t UI = std::strtoull(S.c_str(), &End, 10);
+    if (End == S.end() && errno != ERANGE) {
+      Out = UI;
+      return true;
+    }
+  }
   // If it's not an integer
   Out = std::strtod(S.c_str(), &End);
   return End == S.end() || parseError("Invalid JSON value (number?)");

diff  --git a/llvm/unittests/Support/JSONTest.cpp b/llvm/unittests/Support/JSONTest.cpp
index ecfd2a5fe1a09..8d90a70cc99d0 100644
--- a/llvm/unittests/Support/JSONTest.cpp
+++ b/llvm/unittests/Support/JSONTest.cpp
@@ -362,19 +362,80 @@ TEST(JSONTest, U64Integers) {
   uint64_t Var = 3100100100;
   EXPECT_EQ(Val, Var);
 
+  Val = uint64_t{std::numeric_limits<uint64_t>::max()};
+  Var = std::numeric_limits<uint64_t>::max();
+  EXPECT_EQ(Val, Var);
+
   // Test the parse() part.
-  const char *Str = "4611686018427387905";
-  llvm::Expected<Value> Doc = parse(Str);
+  {
+    const char *Str = "4611686018427387905";
+    llvm::Expected<Value> Doc = parse(Str);
 
-  EXPECT_TRUE(!!Doc);
-  EXPECT_EQ(Doc->getAsInteger(), int64_t{4611686018427387905});
-  EXPECT_EQ(Doc->getAsUINT64(), uint64_t{4611686018427387905});
-
-  const char *Str2 = "-78278238238328222";
-  llvm::Expected<Value> Doc2 = parse(Str2);
-  EXPECT_TRUE(!!Doc2);
-  EXPECT_EQ(Doc2->getAsInteger(), int64_t{-78278238238328222});
-  EXPECT_EQ(Doc2->getAsUINT64(), llvm::None);
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), int64_t{4611686018427387905});
+    EXPECT_EQ(Doc->getAsUINT64(), uint64_t{4611686018427387905});
+  }
+
+  {
+    const char *Str = "-78278238238328222";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), int64_t{-78278238238328222});
+    EXPECT_EQ(Doc->getAsUINT64(), llvm::None);
+  }
+
+  // Test with the largest 64 signed int.
+  {
+    const char *Str = "9223372036854775807";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), int64_t{9223372036854775807});
+    EXPECT_EQ(Doc->getAsUINT64(), uint64_t{9223372036854775807});
+  }
+
+  // Test with the largest 64 unsigned int.
+  {
+    const char *Str = "18446744073709551615";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), None);
+    EXPECT_EQ(Doc->getAsUINT64(), uint64_t{18446744073709551615u});
+  }
+
+  // Test with a number that is too big for 64 bits.
+  {
+    const char *Str = "184467440737095516150";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), None);
+    EXPECT_EQ(Doc->getAsUINT64(), None);
+    // The number was parsed as a double.
+    EXPECT_TRUE(!!Doc->getAsNumber());
+  }
+
+  // Test with a negative number that is too small for 64 bits.
+  {
+    const char *Str = "-18446744073709551615";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_TRUE(!!Doc);
+    EXPECT_EQ(Doc->getAsInteger(), None);
+    EXPECT_EQ(Doc->getAsUINT64(), None);
+    // The number was parsed as a double.
+    EXPECT_TRUE(!!Doc->getAsNumber());
+  }
+  // Test with a large number that is malformed.
+  {
+    const char *Str = "184467440737095516150.12.12";
+    llvm::Expected<Value> Doc = parse(Str);
+
+    EXPECT_EQ("[1:27, byte=27]: Invalid JSON value (number?)",
+              llvm::toString(Doc.takeError()));
+  }
 }
 
 // Sample struct with typical JSON-mapping rules.