Merge pull request #646 from efidler/ubsan

Fix undefined behaviour

CMakeLists.txt

@@ -1,4 +1,8 @@
 CMAKE_MINIMUM_REQUIRED(VERSION 2.8)
+if(POLICY CMP0025)
+  # detect Apple's Clang
+  cmake_policy(SET CMP0025 NEW)
+endif()
 if(POLICY CMP0054)
   cmake_policy(SET CMP0054 NEW)
 endif()
@@ -28,6 +32,9 @@ option(RAPIDJSON_BUILD_THIRDPARTY_GTEST
 
 option(RAPIDJSON_BUILD_CXX11 "Build rapidjson with C++11 (gcc/clang)" ON)
 
+option(RAPIDJSON_BUILD_ASAN "Build rapidjson with address sanitizer (gcc/clang)" OFF)
+option(RAPIDJSON_BUILD_UBSAN "Build rapidjson with undefined behavior sanitizer (gcc/clang)" OFF)
+
 option(RAPIDJSON_HAS_STDSTRING "" OFF)
 if(RAPIDJSON_HAS_STDSTRING)
     add_definitions(-DRAPIDJSON_HAS_STDSTRING)
@@ -51,11 +58,35 @@ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
             set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
         endif()
     endif()
+    if (RAPIDJSON_BUILD_ASAN)
+        if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS "4.8.0")
+            message(FATAL_ERROR "GCC < 4.8 doesn't support the address sanitizer")
+        else()
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address")
+        endif()
+    endif()
+    if (RAPIDJSON_BUILD_UBSAN)
+        if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS "4.9.0")
+            message(FATAL_ERROR "GCC < 4.9 doesn't support the undefined behavior sanitizer")
+        else()
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined")
+        endif()
+    endif()
 elseif (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -march=native -Wall -Wextra -Werror -Wno-missing-field-initializers")
     if (RAPIDJSON_BUILD_CXX11)
         set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
     endif()
+    if (RAPIDJSON_BUILD_ASAN)
+        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address")
+    endif()
+    if (RAPIDJSON_BUILD_UBSAN)
+        if (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang")
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error")
+        else()
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined")
+        endif()
+    endif()
 elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")
     add_definitions(-D_CRT_SECURE_NO_WARNINGS=1)
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /EHsc")

include/rapidjson/document.h

@@ -23,6 +23,7 @@
 #include "memorystream.h"
 #include "encodedstream.h"
 #include <new>      // placement new
+#include <limits>
 
 #ifdef _MSC_VER
 RAPIDJSON_DIAG_PUSH
@@ -952,12 +953,16 @@ public:
         if (IsUint64()) {
             uint64_t u = GetUint64();
             volatile double d = static_cast<double>(u);
-            return static_cast<uint64_t>(d) == u;
+            return (d >= 0.0)
+                && (d < static_cast<double>(std::numeric_limits<uint64_t>::max()))
+                && (u == static_cast<uint64_t>(d));
         }
         if (IsInt64()) {
             int64_t i = GetInt64();
             volatile double d = static_cast<double>(i);
-            return static_cast< int64_t>(d) == i;
+            return (d >= static_cast<double>(std::numeric_limits<int64_t>::min()))
+                && (d < static_cast<double>(std::numeric_limits<int64_t>::max()))
+                && (i == static_cast<int64_t>(d));
         }
         return true; // double, int, uint are always lossless
     }
@@ -973,6 +978,9 @@ public:
     bool IsLosslessFloat() const {
         if (!IsNumber()) return false;
         double a = GetDouble();
+        if (a < static_cast<double>(-std::numeric_limits<float>::max())
+                || a > static_cast<double>(std::numeric_limits<float>::max()))
+            return false;
         double b = static_cast<double>(static_cast<float>(a));
         return a >= b && a <= b;    // Prevent -Wfloat-equal
     }

include/rapidjson/encodings.h

@@ -154,7 +154,11 @@ struct UTF8 {
         }
 
         unsigned char type = GetRange(static_cast<unsigned char>(c));
-        *codepoint = (0xFF >> type) & static_cast<unsigned char>(c);
+        if (type >= 32) {
+            *codepoint = 0;
+        } else {
+            *codepoint = (0xFF >> type) & static_cast<unsigned char>(c);
+        }
         bool result = true;
         switch (type) {
         case 2: TAIL(); return result;

include/rapidjson/internal/dtoa.h

@@ -102,7 +102,8 @@ inline void DigitGen(const DiyFp& W, const DiyFp& Mp, uint64_t delta, char* buff
         kappa--;
         if (p2 < delta) {
             *K += kappa;
-            GrisuRound(buffer, *len, delta, p2, one.f, wp_w.f * kPow10[-static_cast<int>(kappa)]);
+            int index = -static_cast<int>(kappa);
+            GrisuRound(buffer, *len, delta, p2, one.f, wp_w.f * (index < 9 ? kPow10[-static_cast<int>(kappa)] : 0));
             return;
         }
     }

include/rapidjson/internal/strtod.h

@@ -142,7 +142,7 @@ inline bool StrtodDiyFp(const char* decimals, size_t length, size_t decimalPosit
     size_t remaining = length - i;
     const unsigned kUlpShift = 3;
     const unsigned kUlp = 1 << kUlpShift;
-    int error = (remaining == 0) ? 0 : kUlp / 2;
+    int64_t error = (remaining == 0) ? 0 : kUlp / 2;
 
     DiyFp v(significand, 0);
     v = v.Normalize();

include/rapidjson/pointer.h

@@ -767,8 +767,12 @@ private:
         tokenCount_ = rhs.tokenCount_ + extraToken;
         tokens_ = static_cast<Token *>(allocator_->Malloc(tokenCount_ * sizeof(Token) + (nameBufferSize + extraNameBufferSize) * sizeof(Ch)));
         nameBuffer_ = reinterpret_cast<Ch *>(tokens_ + tokenCount_);
-        std::memcpy(tokens_, rhs.tokens_, rhs.tokenCount_ * sizeof(Token));
+        if (rhs.tokenCount_ > 0) {
-        std::memcpy(nameBuffer_, rhs.nameBuffer_, nameBufferSize * sizeof(Ch));
+            std::memcpy(tokens_, rhs.tokens_, rhs.tokenCount_ * sizeof(Token));
+        }
+        if (nameBufferSize > 0) {
+            std::memcpy(nameBuffer_, rhs.nameBuffer_, nameBufferSize * sizeof(Ch));
+        }
 
         // Adjust pointers to name buffer
         std::ptrdiff_t diff = nameBuffer_ - rhs.nameBuffer_;

include/rapidjson/reader.h

@@ -43,6 +43,7 @@ RAPIDJSON_DIAG_OFF(4702)  // unreachable code
 
 #ifdef __clang__
 RAPIDJSON_DIAG_PUSH
+RAPIDJSON_DIAG_OFF(old-style-cast)
 RAPIDJSON_DIAG_OFF(padded)
 RAPIDJSON_DIAG_OFF(switch-enum)
 #endif

include/rapidjson/schema.h

@@ -413,9 +413,11 @@ public:
                 }
             }
 
-        AssignIfExist(allOf_, *schemaDocument, p, value, GetAllOfString(), document);
+        if (schemaDocument) {
-        AssignIfExist(anyOf_, *schemaDocument, p, value, GetAnyOfString(), document);
+            AssignIfExist(allOf_, *schemaDocument, p, value, GetAllOfString(), document);
-        AssignIfExist(oneOf_, *schemaDocument, p, value, GetOneOfString(), document);
+            AssignIfExist(anyOf_, *schemaDocument, p, value, GetAnyOfString(), document);
+            AssignIfExist(oneOf_, *schemaDocument, p, value, GetOneOfString(), document);
+        }
 
         if (const ValueType* v = GetMember(value, GetNotString())) {
             schemaDocument->CreateSchema(&not_, p.Append(GetNotString(), allocator_), *v, document);
@@ -578,7 +580,9 @@ public:
     }
 
     ~Schema() {
-        allocator_->Free(enum_);
+        if (allocator_) {
+            allocator_->Free(enum_);
+        }
         if (properties_) {
             for (SizeType i = 0; i < propertyCount_; i++)
                 properties_[i].~Property();

test/unittest/CMakeLists.txt

@@ -1,3 +1,5 @@
+include(CheckCXXCompilerFlag)
+
 set(UNITTEST_SOURCES
 	allocatorstest.cpp
     bigintegertest.cpp
@@ -38,11 +40,14 @@ if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror -Wall -Wextra -Weffc++ -Wswitch-default -Wfloat-equal")
 elseif (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror -Wall -Wextra -Weffc++ -Wswitch-default -Wfloat-equal -Wimplicit-fallthrough -Weverything")
-	# If the user is running a newer version of Clang that includes the
+    # If the user is running a newer version of Clang that includes the
-	# -Wdouble-promotion, we will ignore that warning.
+    # -Wdouble-promotion, we will ignore that warning.
-	# if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 3.7)
+    if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 3.7)
-	#     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-double-promotion")
+        CHECK_CXX_COMPILER_FLAG("-Wno-double-promotion" HAS_NO_DOUBLE_PROMOTION)
-	# endif()
+        if (HAS_NO_DOUBLE_PROMOTION)
+            set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-double-promotion")
+        endif()
+    endif()
 elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "MSVC")
     # Force to always compile with /W4
     if(CMAKE_CXX_FLAGS MATCHES "/W[0-4]")

test/unittest/dtoatest.cpp

@@ -37,6 +37,7 @@ TEST(dtoa, normal) {
     TEST_DTOA(1.2345678, "1.2345678");
     TEST_DTOA(0.123456789012, "0.123456789012");
     TEST_DTOA(1234567.8, "1234567.8");
+    TEST_DTOA(-79.39773355813419, "-79.39773355813419");
     TEST_DTOA(0.000001, "0.000001");
     TEST_DTOA(0.0000001, "1e-7");
     TEST_DTOA(1e30, "1e30");

test/unittest/itoatest.cpp

@@ -84,6 +84,8 @@ static void Verify(void(*f)(T, char*), char* (*g)(T, char*)) {
                 VerifyValue<T>(Traits<T>::Negate(i + 1), f, g);
             }
             last = i;
+            if (i > static_cast<T>(std::numeric_limits<T>::max() / static_cast<T>(power)))
+                break;
             i *= power;
         } while (last < i);
     }

test/unittest/schematest.cpp

@@ -111,7 +111,7 @@ TEST(SchemaValidator, Hasher) {
     EXPECT_FALSE(d.HasParseError());\
     EXPECT_TRUE(expected == d.Accept(validator));\
     EXPECT_TRUE(expected == validator.IsValid());\
-    if (expected && !validator.IsValid()) {\
+    if ((expected) && !validator.IsValid()) {\
         StringBuffer sb;\
         validator.GetInvalidSchemaPointer().StringifyUriFragment(sb);\
         printf("Invalid schema: %s\n", sb.GetString());\

test/unittest/valuetest.cpp

@@ -545,8 +545,10 @@ TEST(Value, Int64) {
     // Templated functions
     EXPECT_TRUE(z.Is<int64_t>());
     EXPECT_EQ(i, z.Get<int64_t>());
+#if 0 // signed integer underflow is undefined behaviour
     EXPECT_EQ(i - 1, z.Set(i - 1).Get<int64_t>());
     EXPECT_EQ(i - 2, z.Set<int64_t>(i - 2).Get<int64_t>());
+#endif
 }
 
 TEST(Value, Uint64) {
@@ -671,6 +673,7 @@ TEST(Value, Float) {
 }
 
 TEST(Value, IsLosslessDouble) {
+    EXPECT_TRUE(Value(0.0).IsLosslessDouble());
     EXPECT_TRUE(Value(12.34).IsLosslessDouble());
     EXPECT_TRUE(Value(-123).IsLosslessDouble());
     EXPECT_TRUE(Value(2147483648u).IsLosslessDouble());
@@ -679,8 +682,19 @@ TEST(Value, IsLosslessDouble) {
     EXPECT_TRUE(Value(RAPIDJSON_UINT64_C2(0xA0000000, 0x00000000)).IsLosslessDouble());
 #endif
 
-    EXPECT_FALSE(Value(-static_cast<int64_t>(RAPIDJSON_UINT64_C2(0x7FFFFFFF, 0xFFFFFFFF))).IsLosslessDouble());
+    EXPECT_FALSE(Value(static_cast<int64_t>(RAPIDJSON_UINT64_C2(0x7FFFFFFF, 0xFFFFFFFF))).IsLosslessDouble()); // INT64_MAX
-    EXPECT_FALSE(Value(RAPIDJSON_UINT64_C2(0xFFFFFFFF, 0xFFFFFFFF)).IsLosslessDouble());
+    EXPECT_FALSE(Value(-static_cast<int64_t>(RAPIDJSON_UINT64_C2(0x7FFFFFFF, 0xFFFFFFFF))).IsLosslessDouble()); // -INT64_MAX
+    EXPECT_TRUE(Value(-static_cast<int64_t>(RAPIDJSON_UINT64_C2(0x7FFFFFFF, 0xFFFFFFFF)) - 1).IsLosslessDouble()); // INT64_MIN
+    EXPECT_FALSE(Value(RAPIDJSON_UINT64_C2(0xFFFFFFFF, 0xFFFFFFFF)).IsLosslessDouble()); // UINT64_MAX
+
+    EXPECT_TRUE(Value(3.4028234e38f).IsLosslessDouble()); // FLT_MAX
+    EXPECT_TRUE(Value(-3.4028234e38f).IsLosslessDouble()); // -FLT_MAX
+    EXPECT_TRUE(Value(1.17549435e-38f).IsLosslessDouble()); // FLT_MIN
+    EXPECT_TRUE(Value(-1.17549435e-38f).IsLosslessDouble()); // -FLT_MIN
+    EXPECT_TRUE(Value(1.7976931348623157e+308).IsLosslessDouble()); // DBL_MAX
+    EXPECT_TRUE(Value(-1.7976931348623157e+308).IsLosslessDouble()); // -DBL_MAX
+    EXPECT_TRUE(Value(2.2250738585072014e-308).IsLosslessDouble()); // DBL_MIN
+    EXPECT_TRUE(Value(-2.2250738585072014e-308).IsLosslessDouble()); // -DBL_MIN
 }
 
 TEST(Value, IsLosslessFloat) {
@@ -1119,14 +1133,18 @@ TEST(Value, ArrayHelperRangeFor) {
 
     {
         int i = 0;
-        for (auto& v : x.GetArray())
+        for (auto& v : x.GetArray()) {
-            EXPECT_EQ(i++, v.GetInt());
+            EXPECT_EQ(i, v.GetInt());
+            i++;
+        }
         EXPECT_EQ(i, 10);
     }
     {
         int i = 0;
-        for (const auto& v : const_cast<const Value&>(x).GetArray())
+        for (const auto& v : const_cast<const Value&>(x).GetArray()) {
-            EXPECT_EQ(i++, v.GetInt());
+            EXPECT_EQ(i, v.GetInt());
+            i++;
+        }
         EXPECT_EQ(i, 10);
     }
 

test/unittest/writertest.cpp

@@ -439,11 +439,9 @@ TEST(Writer, InvalidEventSequence) {
     }
 }
 
-extern double zero; // clang -Wmissing-variable-declarations
-double zero = 0.0;	// Use global variable to prevent compiler warning
-
 TEST(Writer, NaN) {
-    double nan = zero / zero;
+    double nan = std::numeric_limits<double>::quiet_NaN();
+
     EXPECT_TRUE(internal::Double(nan).IsNan());
     StringBuffer buffer;
     {
@@ -461,7 +459,8 @@ TEST(Writer, NaN) {
 }
 
 TEST(Writer, Inf) {
-    double inf = 1.0 / zero;
+    double inf = std::numeric_limits<double>::infinity();
+
     EXPECT_TRUE(internal::Double(inf).IsInf());
     StringBuffer buffer;
     {