Fix various things -fsanitize complains about

sipa commented at 11:17 pm on January 10, 2017: member

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.

fanquake added the label Refactoring on Jan 10, 2017

in src/test/scriptnum_tests.cpp: in 4fde6a8b8b outdated

12@@ -13,7 +13,7 @@
13 BOOST_FIXTURE_TEST_SUITE(scriptnum_tests, BasicTestingSetup)
14 
15 static const int64_t values[] = \
16-{ 0, 1, CHAR_MIN, CHAR_MAX, UCHAR_MAX, SHRT_MIN, USHRT_MAX, INT_MIN, INT_MAX, UINT_MAX, LONG_MIN, LONG_MAX };
17+{ 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)};

dcousens commented at 0:17 am on January 11, 2017:

maybe define these constants ourselves rather than inlining them?

laanwj commented at 12:49 pm on January 11, 2017:

What is the problem that sanitize reports here? I don’t understand the rationale for using “40” here.

sipa commented at 5:05 pm on January 11, 2017:

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

sipa commented at 5:07 pm on January 11, 2017:

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).

dcousens approved

in src/crypto/common.h: in 4fde6a8b8b outdated

30 }
31 
32 uint64_t static inline ReadLE64(const unsigned char* ptr)
33 {
34-    return le64toh(*((uint64_t*)ptr));
35+    return ((uint64_t)(ptr[0])) |

laanwj commented at 12:47 pm on January 11, 2017:

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

sipa commented at 5:05 pm on January 11, 2017:

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

sipa force-pushed on Jan 11, 2017

sipa commented at 6:19 pm on January 11, 2017: member

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