From 4843b55fd167c54d86eb9e1f19525ed363cfe72d Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Sun, 13 May 2012 21:31:59 +0200 Subject: [PATCH 1/8] Make CNetAddr::GetHash() return an unsigned val. This prevents an undefined operation in main.cpp, when shifting the hash value left by 32 bits. Shifting a signed int left into the sign bit is undefined in C++11. --- src/main.cpp | 2 +- src/netbase.cpp | 4 ++-- src/netbase.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 20bb56e96..f71702f0e 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2440,7 +2440,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv) static uint256 hashSalt; if (hashSalt == 0) RAND_bytes((unsigned char*)&hashSalt, sizeof(hashSalt)); - int64 hashAddr = addr.GetHash(); + uint64 hashAddr = addr.GetHash(); uint256 hashRand = hashSalt ^ (hashAddr<<32) ^ ((GetTime()+hashAddr)/(24*60*60)); hashRand = Hash(BEGIN(hashRand), END(hashRand)); multimap mapMix; diff --git a/src/netbase.cpp b/src/netbase.cpp index 2131bdf75..c237e2dc4 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -820,10 +820,10 @@ std::vector CNetAddr::GetGroup() const return vchRet; } -int64 CNetAddr::GetHash() const +uint64 CNetAddr::GetHash() const { uint256 hash = Hash(&ip[0], &ip[16]); - int64 nRet; + uint64 nRet; memcpy(&nRet, &hash, sizeof(nRet)); return nRet; } diff --git a/src/netbase.h b/src/netbase.h index 514a1ae95..544a66686 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -66,7 +66,7 @@ class CNetAddr std::string ToString() const; std::string ToStringIP() const; int GetByte(int n) const; - int64 GetHash() const; + uint64 GetHash() const; bool GetInAddr(struct in_addr* pipv4Addr) const; std::vector GetGroup() const; int GetReachabilityFrom(const CNetAddr *paddrPartner = NULL) const; From fe78c9ae8b4c9701cf6d84a0572a2d503c6ee424 Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Mon, 14 May 2012 02:50:01 +0200 Subject: [PATCH 2/8] Don't overflow signed ints in CBigNum::setint64(). CBigNum::setint64() does 'n <<= 8', where n is of type "long long". This leads to shifting onto and past the sign bit, which is undefined behavior in C++11 and can cause problems in the future. --- src/bignum.h | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/bignum.h b/src/bignum.h index f0971e885..3716c4965 100644 --- a/src/bignum.h +++ b/src/bignum.h @@ -122,16 +122,22 @@ public: return (n > (unsigned long)std::numeric_limits::max() ? std::numeric_limits::min() : -(int)n); } - void setint64(int64 n) + void setint64(int64 sn) { - unsigned char pch[sizeof(n) + 6]; + unsigned char pch[sizeof(sn) + 6]; unsigned char* p = pch + 4; - bool fNegative = false; - if (n < (int64)0) + bool fNegative; + uint64 n; + + if (sn < (int64)0) { - n = -n; + n = -sn; fNegative = true; + } else { + n = sn; + fNegative = false; } + bool fLeadingZeroes = true; for (int i = 0; i < 8; i++) { From 62e0453ce0ee0f03fca4626882263ec41dc1d64d Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Mon, 14 May 2012 21:15:27 +0200 Subject: [PATCH 3/8] Add test case for CBigNum::setint64(). One of the test cases currently aborts when using gcc's flag -ftrapv, due to negating an INT64_MIN int64 variable, which is an undefined operation. This will be fixed in a subsequent commit. --- src/test/bignum_tests.cpp | 110 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 src/test/bignum_tests.cpp diff --git a/src/test/bignum_tests.cpp b/src/test/bignum_tests.cpp new file mode 100644 index 000000000..ca17766d1 --- /dev/null +++ b/src/test/bignum_tests.cpp @@ -0,0 +1,110 @@ +#include +#include + +#include "bignum.h" + +BOOST_AUTO_TEST_SUITE(bignum_tests) + + +// For the following test case, it is useful to use additional tools. +// +// The simplest one to use is the compiler flag -ftrapv, which detects integer +// overflows and similar errors. However, due to optimizations and compilers +// taking advantage of undefined behavior sometimes it may not actually detect +// anything. +// +// You can also use compiler-based stack protection to possibly detect possible +// stack buffer overruns. +// +// For more accurate diagnostics, you can use an undefined arithmetic operation +// detector such as the clang-based tool: +// +// "IOC: An Integer Overflow Checker for C/C++" +// +// Available at: http://embed.cs.utah.edu/ioc/ +// +// It might also be useful to use Google's AddressSanitizer to detect +// stack buffer overruns, which valgrind can't currently detect. + +// Let's force this code not to be inlined, in order to actually +// test a generic version of the function. This increases the chance +// that -ftrapv will detect overflows. +void mysetint64(CBigNum& num, int64 n) __attribute__((noinline)); + +void mysetint64(CBigNum& num, int64 n) +{ + num.setint64(n); +} + +// For each number, we do 2 tests: one with inline code, then we reset the +// value to 0, then the second one with a non-inlined function. +BOOST_AUTO_TEST_CASE(bignum_setint64) +{ + int64 n; + + { + n = 0; + CBigNum num(n); + BOOST_CHECK(num.ToString() == "0"); + num.setulong(0); + BOOST_CHECK(num.ToString() == "0"); + mysetint64(num, n); + BOOST_CHECK(num.ToString() == "0"); + } + { + n = 1; + CBigNum num(n); + BOOST_CHECK(num.ToString() == "1"); + num.setulong(0); + BOOST_CHECK(num.ToString() == "0"); + mysetint64(num, n); + BOOST_CHECK(num.ToString() == "1"); + } + { + n = -1; + CBigNum num(n); + BOOST_CHECK(num.ToString() == "-1"); + num.setulong(0); + BOOST_CHECK(num.ToString() == "0"); + mysetint64(num, n); + BOOST_CHECK(num.ToString() == "-1"); + } + { + n = 5; + CBigNum num(n); + BOOST_CHECK(num.ToString() == "5"); + num.setulong(0); + BOOST_CHECK(num.ToString() == "0"); + mysetint64(num, n); + BOOST_CHECK(num.ToString() == "5"); + } + { + n = -5; + CBigNum num(n); + BOOST_CHECK(num.ToString() == "-5"); + num.setulong(0); + BOOST_CHECK(num.ToString() == "0"); + mysetint64(num, n); + BOOST_CHECK(num.ToString() == "-5"); + } + { + n = LLONG_MIN; + CBigNum num(n); + BOOST_CHECK(num.ToString() == "-9223372036854775808"); + num.setulong(0); + BOOST_CHECK(num.ToString() == "0"); + mysetint64(num, n); + BOOST_CHECK(num.ToString() == "-9223372036854775808"); + } + { + n = LLONG_MAX; + CBigNum num(n); + BOOST_CHECK(num.ToString() == "9223372036854775807"); + num.setulong(0); + BOOST_CHECK(num.ToString() == "0"); + mysetint64(num, n); + BOOST_CHECK(num.ToString() == "9223372036854775807"); + } +} + +BOOST_AUTO_TEST_SUITE_END() From 5849bd472a3a7296f91b887884946218897ca11f Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Mon, 14 May 2012 21:17:24 +0200 Subject: [PATCH 4/8] Fix signed subtraction overflow in CBigNum::setint64(). As noticed by sipa (Pieter Wuille), this can happen when CBigNum::setint64() is called with an integer value of INT64_MIN (-2^63). When compiled with -ftrapv, the program would crash. Otherwise, it would execute an undefined operation (although in practice, usually the correct one). --- src/bignum.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/bignum.h b/src/bignum.h index 3716c4965..5190c2f39 100644 --- a/src/bignum.h +++ b/src/bignum.h @@ -131,7 +131,15 @@ public: if (sn < (int64)0) { - n = -sn; + // We negate in 2 steps to avoid signed subtraction overflow, + // i.e. -(-2^63), which is an undefined operation and causes SIGILL + // when compiled with -ftrapv. + // + // Note that uint64_t n = sn, when sn is an int64_t, is a + // well-defined operation and n will be equal to sn + 2^64 when sn + // is negative. + n = sn; + n = -n; fNegative = true; } else { n = sn; From 78e851f94f67b56e9baa8a62d6266c6bf979ee9e Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Thu, 31 May 2012 19:12:01 +0200 Subject: [PATCH 5/8] Fix noinline definition so that it works for more compilers. --- src/test/bignum_tests.cpp | 5 ++--- src/util.h | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/test/bignum_tests.cpp b/src/test/bignum_tests.cpp index ca17766d1..16e4449e9 100644 --- a/src/test/bignum_tests.cpp +++ b/src/test/bignum_tests.cpp @@ -2,6 +2,7 @@ #include #include "bignum.h" +#include "util.h" BOOST_AUTO_TEST_SUITE(bignum_tests) @@ -29,9 +30,7 @@ BOOST_AUTO_TEST_SUITE(bignum_tests) // Let's force this code not to be inlined, in order to actually // test a generic version of the function. This increases the chance // that -ftrapv will detect overflows. -void mysetint64(CBigNum& num, int64 n) __attribute__((noinline)); - -void mysetint64(CBigNum& num, int64 n) +NOINLINE void mysetint64(CBigNum& num, int64 n) { num.setint64(n); } diff --git a/src/util.h b/src/util.h index 8e65fa786..0d5221c33 100644 --- a/src/util.h +++ b/src/util.h @@ -43,6 +43,23 @@ static const int64 CENT = 1000000; #define ARRAYLEN(array) (sizeof(array)/sizeof((array)[0])) #define printf OutputDebugStringF +// Unfortunately there's no standard way of preventing a function from being +// inlined, so we define a macro for it. +// +// You should use it like this: +// NOINLINE void function() {...} +#if defined(__GNUC__) +// This also works and will be defined for any compiler implementing gcc +// extensions, such as clang and icc. +#define NOINLINE __attribute__((noinline)) +#elif defined(_MSC_VER) +#define NOINLINE __declspec(noinline) +#else +// We give out a warning because it impacts the correctness of one bignum test. +#warning You should define NOINLINE for your compiler. +#define NOINLINE +#endif + #ifdef snprintf #undef snprintf #endif From 10b45b4c2e11994aa2aed966f83d84f83852d1e1 Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Thu, 31 May 2012 20:31:15 +0200 Subject: [PATCH 6/8] Use C++-style numeric limits instead of C-style. --- src/test/bignum_tests.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/bignum_tests.cpp b/src/test/bignum_tests.cpp index 16e4449e9..38c625bba 100644 --- a/src/test/bignum_tests.cpp +++ b/src/test/bignum_tests.cpp @@ -1,5 +1,5 @@ #include -#include +#include #include "bignum.h" #include "util.h" @@ -87,7 +87,7 @@ BOOST_AUTO_TEST_CASE(bignum_setint64) BOOST_CHECK(num.ToString() == "-5"); } { - n = LLONG_MIN; + n = std::numeric_limits::min(); CBigNum num(n); BOOST_CHECK(num.ToString() == "-9223372036854775808"); num.setulong(0); @@ -96,7 +96,7 @@ BOOST_AUTO_TEST_CASE(bignum_setint64) BOOST_CHECK(num.ToString() == "-9223372036854775808"); } { - n = LLONG_MAX; + n = std::numeric_limits::max(); CBigNum num(n); BOOST_CHECK(num.ToString() == "9223372036854775807"); num.setulong(0); From 43346904e15c3f1c3f92eee140dbf37b897c7c21 Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Thu, 7 Jun 2012 19:11:15 +0200 Subject: [PATCH 7/8] Don't overflow integer on 32-bit machines. This was causing test_bitcoin to abort on a 32-bit system likely due to -ftrapv. --- src/util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util.h b/src/util.h index 0d5221c33..cfc20b327 100644 --- a/src/util.h +++ b/src/util.h @@ -288,7 +288,7 @@ inline int64 GetPerformanceCounter() #else timeval t; gettimeofday(&t, NULL); - nCounter = t.tv_sec * 1000000 + t.tv_usec; + nCounter = (int64) t.tv_sec * 1000000 + t.tv_usec; #endif return nCounter; } From 31ac53fbdc2346876da201b9e1495565b38b46ba Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Thu, 7 Jun 2012 19:14:18 +0200 Subject: [PATCH 8/8] Move NOINLINE definition to test where it's used. --- src/test/bignum_tests.cpp | 16 ++++++++++++++++ src/util.h | 17 ----------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/test/bignum_tests.cpp b/src/test/bignum_tests.cpp index 38c625bba..8620f81f1 100644 --- a/src/test/bignum_tests.cpp +++ b/src/test/bignum_tests.cpp @@ -6,6 +6,22 @@ BOOST_AUTO_TEST_SUITE(bignum_tests) +// Unfortunately there's no standard way of preventing a function from being +// inlined, so we define a macro for it. +// +// You should use it like this: +// NOINLINE void function() {...} +#if defined(__GNUC__) +// This also works and will be defined for any compiler implementing gcc +// extensions, such as clang and icc. +#define NOINLINE __attribute__((noinline)) +#elif defined(_MSC_VER) +#define NOINLINE __declspec(noinline) +#else +// We give out a warning because it impacts the correctness of one bignum test. +#warning You should define NOINLINE for your compiler. +#define NOINLINE +#endif // For the following test case, it is useful to use additional tools. // diff --git a/src/util.h b/src/util.h index cfc20b327..b77eb43c9 100644 --- a/src/util.h +++ b/src/util.h @@ -43,23 +43,6 @@ static const int64 CENT = 1000000; #define ARRAYLEN(array) (sizeof(array)/sizeof((array)[0])) #define printf OutputDebugStringF -// Unfortunately there's no standard way of preventing a function from being -// inlined, so we define a macro for it. -// -// You should use it like this: -// NOINLINE void function() {...} -#if defined(__GNUC__) -// This also works and will be defined for any compiler implementing gcc -// extensions, such as clang and icc. -#define NOINLINE __attribute__((noinline)) -#elif defined(_MSC_VER) -#define NOINLINE __declspec(noinline) -#else -// We give out a warning because it impacts the correctness of one bignum test. -#warning You should define NOINLINE for your compiler. -#define NOINLINE -#endif - #ifdef snprintf #undef snprintf #endif