Fix undefined behaviour #646

efidler · 2016-05-31T18:57:07Z

Fix issues with undefined behaviour, mostly found by running the unittests under UBSAN on Clang/OSX and GCC5/Linux.

Issue #644

coveralls · 2016-05-31T19:29:42Z

Coverage remained the same at 100.0% when pulling e41b04a on efidler:ubsan into e6895ec on miloyip:master.

coveralls · 2016-05-31T20:10:08Z

Coverage remained the same at 100.0% when pulling d27a255 on efidler:ubsan into e6895ec on miloyip:master.

miloyip · 2016-06-01T02:02:35Z

include/rapidjson/document.h

@@ -952,12 +956,16 @@ class GenericValue {
        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()/2 + 1)) * 2)


Can you explain about this?

Casting a double that's out-of-range for the uint64_t is undefined behaviour, so this equality test has undefined results. I'm not 100% clear on what exactly IsLosslessDouble promises if it returns true, but it's certainly possible that you could get true from the equality test when you expect false.

I am not sure about the calculation (max / 2 + 1) * 2. I think it can simply construct a maximum double that can be converted to uint64_t.

The issue is UINT64_MAX can't be exactly represented in a double. I changed it to be strictly less than UINT64_MAX as a double and added some more tests.

coveralls · 2016-06-01T17:13:32Z

Coverage remained the same at 100.0% when pulling 875a9e1 on efidler:ubsan into e6895ec on miloyip:master.

coveralls · 2016-06-02T16:28:09Z

Coverage remained the same at 100.0% when pulling 6923197 on efidler:ubsan into e6895ec on miloyip:master.

efidler · 2016-06-06T14:55:38Z

any more concerns? is this ready to merge?

miloyip · 2016-06-07T01:41:36Z

Travis reported that clang build were failed. Can you try to solve that? Thanks.

efidler · 2016-06-07T01:44:39Z

it looks like travis is failing to create the clang toolchain in the VM. I don't think it has anything to do with the code itself.

efidler · 2016-06-07T01:54:59Z

Yes, LLVM removed their apt mirror, which broken travis builds for tons of projects. We could move to the Trusty Travis environment and use the built-in clang, which is 3.5.

miloyip · 2016-06-07T02:04:26Z

Thank you. I just realized this. I will try to fix the build before merging for safety.
travis-ci/travis-ci#6120

miloyip · 2016-06-10T02:54:01Z

Hi, I tried on OSX and seems the compiler version of clang is different. I tried to fixed with this:

--- a/test/unittest/CMakeLists.txt
+++ b/test/unittest/CMakeLists.txt
@@ -1,3 +1,5 @@
+include(CheckCXXCompilerFlag)
+
 set(UNITTEST_SOURCES
        allocatorstest.cpp
     bigintegertest.cpp
@@ -41,7 +43,10 @@ elseif (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
     # If the user is running a newer version of Clang that includes the
     # -Wdouble-promotion, we will ignore that warning.
     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)
+        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

Besides, there are problems in MSVC compilation in appveyor. Can you check if they can be fixed?

…e compiling with -Werror

…chable-code clang advises: "note: silence by adding parentheses to mark code as explicitly dead"

specifically, "expression with side effects has no effect in an unevaluated context"

UBSAN gave "runtime error: index 13 out of bounds for type 'const uint32_t [10]'"

UBSAN gave issues with the typeless Schema: runtime error: reference binding to null pointer of type 'rapidjson::GenericSchemaDocument<rapidjson::GenericValue<rapidjson::UTF16<wchar_t>, rapidjson::MemoryPoolAllocator<rapidjson::CrtAllocator> >, rapidjson::CrtAllocator>' and runtime error: member access within null pointer of type 'AllocatorType' (aka 'rapidjson::CrtAllocator')

UBSAN on Clang/Linux gave: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:43:45: note: nonnull attribute specified here

coveralls · 2016-06-13T14:44:06Z

Coverage remained the same at 100.0% when pulling 9eeae8d on efidler:ubsan into 2e66339 on miloyip:master.

coveralls · 2016-06-13T14:56:03Z

Coverage remained the same at 100.0% when pulling 9eeae8d on efidler:ubsan into 2e66339 on miloyip:master.

miloyip · 2016-06-14T03:25:01Z

I merged #654 and fixed some compiling issues with that.
Hope this PR can be updated as well. Thanks.

UBSAN gave for test/unittest/itoatest.cpp:87: runtime error: signed integer overflow: 4611686018427387904 * 2 cannot be represented in type 'long'

maybe these tests should just be deleted? UBSAN gave: runtime error: signed integer overflow: -9223372036854775808 - 1 cannot be represented in type 'long' runtime error: signed integer overflow: -9223372036854775808 - 2 cannot be represented in type 'long'

UBSAN gave: runtime error: division by zero

UBSAN gave during Reader.ParseNumber_FullPrecisionDouble test: include/rapidjson/internal/strtod.h:149:11: runtime error: shift exponent 46 is too large for 32-bit type 'int'

UBSAN gave in Regex.Unicode test: include/rapidjson/encodings.h:157:28: runtime error: shift exponent 32 is too large for 32-bit type 'int'

note that std::numeric_limits<uint64_t>::max() and std::numeric_limits<int64_t>::max() aren't exactly representable in a double, so we need to be strictly less to be definitely lossless UBSAN gave during Value.IsLosslessDouble test: include/rapidjson/document.h:955:42: runtime error: value 1.84467e+19 is outside the range of representable values of type 'unsigned long'

UBSAN gave in Value.IsLosslessFloat: include/rapidjson/document.h:981:38: runtime error: value 3.40282e+38 is outside the range of representable values of type 'float'

efidler · 2016-06-14T14:02:42Z

I just pushed an update. Sorry for the slow progress; I'm travelling.

coveralls · 2016-06-14T14:18:36Z

Coverage remained the same at 100.0% when pulling 8c40597 on efidler:ubsan into 2e66339 on miloyip:master.

efidler · 2016-06-14T23:44:48Z

ok, build passed

miloyip · 2016-06-15T01:35:14Z

Thank you very much.

efidler force-pushed the ubsan branch from abab0cd to e41b04a Compare May 31, 2016 19:17

efidler force-pushed the ubsan branch from e41b04a to d27a255 Compare May 31, 2016 19:35

efidler mentioned this pull request May 31, 2016

Undefined behaviour #644

Closed

miloyip reviewed Jun 1, 2016
View reviewed changes

efidler force-pushed the ubsan branch from d27a255 to 875a9e1 Compare June 1, 2016 16:13

efidler force-pushed the ubsan branch from 875a9e1 to 6923197 Compare June 2, 2016 16:15

efidler added 8 commits June 13, 2016 07:24

support building with ASAN and UBSAN on Clang and GCC

56bb999

Clang doesn't like the C-style casts in nmmintrin.h

89f6b8a

we do need to avoid the double-promotion warning on clang, since we'r…

13e3aa9

…e compiling with -Werror

with recent clang, when expected is false, this code triggers -Wunrea…

0352710

…chable-code clang advises: "note: silence by adding parentheses to mark code as explicitly dead"

with recent clang, this triggers -Wunevaluated-expression

5c77c92

specifically, "expression with side effects has no effect in an unevaluated context"

avoid array index out-of-bounds

fe550f3

UBSAN gave "runtime error: index 13 out of bounds for type 'const uint32_t [10]'"

avoid passing a null pointer to memcpy

61637d3

UBSAN on Clang/Linux gave: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:43:45: note: nonnull attribute specified here

efidler force-pushed the ubsan branch from 6923197 to b9535e9 Compare June 13, 2016 14:28

efidler added 8 commits June 14, 2016 07:01

avoid signed-integer overflow, which is undefined behavior

be1eedf

UBSAN gave for test/unittest/itoatest.cpp:87: runtime error: signed integer overflow: 4611686018427387904 * 2 cannot be represented in type 'long'

avoid division by zero, which is undefined behavior

df9b45a

UBSAN gave: runtime error: division by zero

avoid shift out-of-range error

9dcf51c

UBSAN gave during Reader.ParseNumber_FullPrecisionDouble test: include/rapidjson/internal/strtod.h:149:11: runtime error: shift exponent 46 is too large for 32-bit type 'int'

avoid shift out-of-range error

05f0592

UBSAN gave in Regex.Unicode test: include/rapidjson/encodings.h:157:28: runtime error: shift exponent 32 is too large for 32-bit type 'int'

range check in IsLosslessFloat to avoid undefined double->float cast

21acc56

UBSAN gave in Value.IsLosslessFloat: include/rapidjson/document.h:981:38: runtime error: value 3.40282e+38 is outside the range of representable values of type 'float'

test for no-double-promotion instead of just checking compiler version

8c40597

efidler force-pushed the ubsan branch from 9eeae8d to 8c40597 Compare June 14, 2016 14:01

miloyip merged commit 62ff0a9 into Tencent:master Jun 15, 2016

miloyip mentioned this pull request Jun 16, 2016

Warnings - Critical #658

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix undefined behaviour #646

Fix undefined behaviour #646

efidler commented May 31, 2016

coveralls commented May 31, 2016

coveralls commented May 31, 2016 •

edited

Loading

miloyip Jun 1, 2016

efidler Jun 1, 2016

miloyip Jun 2, 2016

efidler Jun 2, 2016

coveralls commented Jun 1, 2016 •

edited

Loading

coveralls commented Jun 2, 2016 •

edited

Loading

efidler commented Jun 6, 2016

miloyip commented Jun 7, 2016

efidler commented Jun 7, 2016

efidler commented Jun 7, 2016

miloyip commented Jun 7, 2016

miloyip commented Jun 10, 2016

coveralls commented Jun 13, 2016 •

edited

Loading

coveralls commented Jun 13, 2016 •

edited

Loading

miloyip commented Jun 14, 2016

efidler commented Jun 14, 2016

coveralls commented Jun 14, 2016 •

edited

Loading

efidler commented Jun 14, 2016

miloyip commented Jun 15, 2016

Fix undefined behaviour #646

Fix undefined behaviour #646

Conversation

efidler commented May 31, 2016

coveralls commented May 31, 2016

coveralls commented May 31, 2016 • edited Loading

miloyip Jun 1, 2016

Choose a reason for hiding this comment

efidler Jun 1, 2016

Choose a reason for hiding this comment

miloyip Jun 2, 2016

Choose a reason for hiding this comment

efidler Jun 2, 2016

Choose a reason for hiding this comment

coveralls commented Jun 1, 2016 • edited Loading

coveralls commented Jun 2, 2016 • edited Loading

efidler commented Jun 6, 2016

miloyip commented Jun 7, 2016

efidler commented Jun 7, 2016

efidler commented Jun 7, 2016

miloyip commented Jun 7, 2016

miloyip commented Jun 10, 2016

coveralls commented Jun 13, 2016 • edited Loading

coveralls commented Jun 13, 2016 • edited Loading

miloyip commented Jun 14, 2016

efidler commented Jun 14, 2016

coveralls commented Jun 14, 2016 • edited Loading

efidler commented Jun 14, 2016

miloyip commented Jun 15, 2016

coveralls commented May 31, 2016 •

edited

Loading

coveralls commented Jun 1, 2016 •

edited

Loading

coveralls commented Jun 2, 2016 •

edited

Loading

coveralls commented Jun 13, 2016 •

edited

Loading

coveralls commented Jun 13, 2016 •

edited

Loading

coveralls commented Jun 14, 2016 •

edited

Loading