This pull request has fixes to prevent bit shifting signed integers to the left, onto and past the sign bit.
While this currently does not cause problems, it is undefined behavior in C++11 and could cause problems in the future (i.e. so-called time bombs). Bitcoin in particular should be extra careful about these problems.
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.
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.
How did you test these changes?
For commit "4843b55 Make CNetAddr::GetHash() return an unsigned val.", I verified that there was only one user of CNetAddr::GetHash() by changing the return value to 'void' and seeing where the compilation broke.
In the single user of that function, I think the changes are relatively straightforward and the hash calculations more likely to be correct using uint64 than int64.
For commit "fe78c9a Don't overflow signed ints in CBigNum::setint64()", I wrote a small program that tested creating a few CBigNum vars, with positive and negative integers, to make sure the results were the ones expected.
These 2 bugs were detected using a tool that automatically detects some kinds of undefined behavior at run-time. After fixing them, no more errors were reported.
Furthermore, I let bitcoind run with that tool, along with another one which detects memory corruption problems (even more kinds of memory corruption than valgrind is able to detect), while confirming the latest ~150 blocks of the main net. It successfully confirmed them without any apparent problems.
Could you turn your small test program into unit tests in src/test/bignum_tests.cpp ? Comments on where to get your nifty testing tools/methods would be most welcome, too (in that does-not-exist-yet unit test file).
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.
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).
I added test cases for CBigNum::setint64(). It was a bit more complex than I thought because gcc (with -O2) was inlining the function and, due to the values being constants, optimizing it to the point that -ftrapv was not detecting the signed subtraction overflow when the function was given an INT64_MIN argument.
After the test case was added, it was aborting in one of the test cases, as expected, due to the mentioned bug.
The commit "5849bd4 Fix signed subtraction overflow in CBigNum::setint64()" then fixes the bug.
Note that this bug is also present in the mainline bitcoin code and will cause bitcoin to crash when a CBigNum var is initialized with an INT64_MIN value and the code was compiled with -ftrapv.
0 | @@ -0,0 +1,110 @@ 1 | +#include <boost/test/unit_test.hpp> 2 | +#include <climits>
The rest of the code uses #include <limits> and std::numeric_limits<type>::min()/max(), it'd be nice to be consistent if possible.
27 | +// stack buffer overruns, which valgrind can't currently detect. 28 | + 29 | +// Let's force this code not to be inlined, in order to actually 30 | +// test a generic version of the function. This increases the chance 31 | +// that -ftrapv will detect overflows. 32 | +void mysetint64(CBigNum& num, int64 n) __attribute__((noinline));
attribute is gcc only, we should try to stay compiler-independent as much as possible.
Thanks for the numeric limits suggestion, I was not aware of that (I'm a lot more experienced in C than C++, as you can probably guess). I have fixed that as you suggested.
As for the noinline attribute, it will work on any compiler that supports gcc extensions (it should work for gcc, clang and icc, at least), but I guess not for MSVC++.
As far as I could find out, there is no standard way of doing this, so we just have to use whatever syntax each compiler supports.
I have updated the code to define a NOINLINE macro which will conditionally use syntax for gcc-compatible compilers (gcc, clang, icc), MSVC++ or emit a warning if the code is being compiled with an unknown compiler.
I have re-tested everything with gcc and clang (with and without the fix, to see if the test still works), but I haven't tested with MSVC++ as I don't have access to such an environment at the moment.
This was causing test_bitcoin to abort on a 32-bit system likely due to -ftrapv.
42 | @@ -43,6 +43,23 @@ 43 | #define ARRAYLEN(array) (sizeof(array)/sizeof((array)[0])) 44 | #define printf OutputDebugStringF 45 | 46 | +// Unfortunately there's no standard way of preventing a function from being 47 | +// inlined, so we define a macro for it.
In my opinion, this mess should go in bignum_tests.cpp, since that's the only place it is used and I can't imagine we'll use it anyplace else.
I've moved the NOINLINE definition to the test rather than util.h, as suggested.
I have also fixed a bug which was causing the test_bitcoin program to abort on 32-bit systems.
ACK (needs a trivial rebase though).
So what is the process now?
Should I rebase? If so, can I do it in the same branch, and will the pull request preserve all existing information?
@wizeman Seems @gavinandresen did it for you.
For future reference: yes, you can just push to the branch on github where you originally pullrequested from, and the pull request will be updated accordingly (keeping relevant discussion, but changing commits as necessary).
Cool, thanks to both of you!
Hi luke-jr,
I added a comment to your pull request.
Cheers, Ricardo