Fix various things -fsanitize complains about #9512

sipa · 2017-01-10T23:17:38Z

The ReadLE32 etc functions in crypto/common.h where dereferencing type-punned pointers, which is undefined behaviour (and can cause consistent failure on some platforms due to misaligned reads). Fix this by memcpy()ing into a local variable first (as accessing a type's representation through a char* is always allowed).
The scriptnum tests were testing running several arithmetic operations on large integers, triggering signed integer overflow, which is undefined. Fix the tests by using integers limited to +/- 2^40 (larger than anything actually supported in script).
Fix a memory leak in the wallet tests: CWalletTx objects that weren't freed - wrap them in a unique_ptr.
Fix a memory leak in the net tests: a CNode object wasn't being freed - wrap them in a unique_ptr.
Fix a memory leak in the rpc auth: a buffer that wasn't being freed - switch it to a stack-allocated array.
The REST code for getutxos was using boost::dynamic_bitset, which apparently issues a rightshift by a negative amount. Fix this by replacing it with normal bit/byte vectors.

Now all unit tests and rpc tests run succesfully with -fsanitize=address -fsanitize=undefined -fsanitize=leak in GCC 6.2.0.

Thanks to @kcc for pointing me to these tools.

dcousens · 2017-01-11T00:17:23Z

src/test/scriptnum_tests.cpp

@@ -13,7 +13,7 @@
 BOOST_FIXTURE_TEST_SUITE(scriptnum_tests, BasicTestingSetup)

 static const int64_t values[] = \
-{ 0, 1, CHAR_MIN, CHAR_MAX, UCHAR_MAX, SHRT_MIN, USHRT_MAX, INT_MIN, INT_MAX, UINT_MAX, LONG_MIN, LONG_MAX };
+{ 0, 1, CHAR_MIN, CHAR_MAX, UCHAR_MAX, SHRT_MIN, USHRT_MAX, INT_MIN, INT_MAX, UINT_MAX, (1LL << 40), (1LL << 40) - 1, -(1LL << 40), 1 - (1LL << 40)};


maybe define these constants ourselves rather than inlining them?

I've just replaced them with integer constants and added a comment.

laanwj · 2017-01-11T12:47:51Z

src/crypto/common.h

 }

 uint64_t static inline ReadLE64(const unsigned char* ptr)
 {
-    return le64toh(*((uint64_t*)ptr));
+    return ((uint64_t)(ptr[0])) |


Interesting. This would also have failed on CPUs that don't support non-aligned reads (though I know of no such instances reported).

Seems ARM supports unaligned reads pretty much since the architecture we support.

laanwj · 2017-01-11T12:49:41Z

src/test/scriptnum_tests.cpp

@@ -13,7 +13,7 @@
 BOOST_FIXTURE_TEST_SUITE(scriptnum_tests, BasicTestingSetup)

 static const int64_t values[] = \
-{ 0, 1, CHAR_MIN, CHAR_MAX, UCHAR_MAX, SHRT_MIN, USHRT_MAX, INT_MIN, INT_MAX, UINT_MAX, LONG_MIN, LONG_MAX };
+{ 0, 1, CHAR_MIN, CHAR_MAX, UCHAR_MAX, SHRT_MIN, USHRT_MAX, INT_MIN, INT_MAX, UINT_MAX, (1LL << 40), (1LL << 40) - 1, -(1LL << 40), 1 - (1LL << 40)};


What is the problem that sanitize reports here? I don't understand the rationale for using "40" here.

Overflow of signed integers is undefined behaviour. These numbers are added/subtracted from eachother, so that behaviour triggers. However, it doesn't matter in practice, as we only use scriptnums up to 4 bytes (which can turn into 5 bytes after arithmetic).

sipa · 2017-01-11T18:19:23Z

I measure a 1.5% slowdown from these changes for the SHA256_32b benchmark (i7-6820HQ CPU @ 2.70GHz).

JeremyRubin

utACK.

It seems there are some extra bitmasks in the write procedures; I've marked where they are. Maybe the optimized missed these and that's your 1.5% ;)

JeremyRubin · 2017-01-11T20:11:55Z

src/crypto/common.h

 }

 void static inline WriteLE16(unsigned char* ptr, uint16_t x)
 {
-    *((uint16_t*)ptr) = htole16(x);
+    ptr[0] = x & 0xFF;
+    ptr[1] = (x >> 8) & 0xFF;
