Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs.
In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison.
This was previously prevented by violations in leveldb which were fixed upstream and merged in #17398. You can test that by building this branch against: 22d11187ee3c7abfe9d43c9eb68f102498cc2b9a vs 75fb37ce68289eb7e00e2ccdd2ef7f9271332545
This is failing on the ARM64 Travis build:
CXX util/libbitcoin_util_a-asmap.o
CXX util/libbitcoin_util_a-bip32.o
util/asmap.cpp: In function ‘uint32_t Interpret(const std::vector<bool>&, const std::vector<bool>&)’:
util/asmap.cpp:82:26: error: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘std::ptrdiff_t’ {aka ‘int’} [-Werror=sign-compare]
if (jump >= endpos - pos) break;
~~~~~^~~~~~~~~~~~~~~
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Thanks for doing this! I'm confident that adding -Werror=sign-compare to --enable-werror will prevent a non-zero number of bugs hitting master in the future: strong concept ACK! :)
I've reviewed the code and my only suggestion is to add -Wsign-compare as part of our set of default diagnostics. That way developers will have a chance of resolving any sign-compare issues before PR submission (and thus --enable-werror scrunity in Travis).
Tested ACK 416f3be736faabddd06945557e726a28df9688ca modulo above suggestion
I'm confident that adding
-Werror=sign-compareto--enable-werrorwill prevent a non-zero number of bugs hittingmasterin the future: strong concept ACK! :)
FWIW -- CVE-2017-18350 was signedness related:
"CVE-2017-18350 is a buffer overflow vulnerability which allows a malicious
SOCKS proxy server to overwrite the program stack on systems with a signed
char type (including common 32-bit and 64-bit x86 PCs)."
ACK acee61494d50d2821003dde6c888b9ed3ed1ddce
Concept ACK.
76 | @@ -77,9 +77,10 @@ uint32_t Interpret(const std::vector<bool> &asmap, const std::vector<bool> &ip) 77 | return DecodeASN(pos, endpos); 78 | } else if (opcode == 1) { 79 | jump = DecodeJump(pos, endpos); 80 | + assert(pos + jump > pos);
Why this assert is needed here?
I mean for that to protect against the unlikely case of overflow of pos + jump overflowing - if an overflow occurs, pos + jump will be less than pos.
Is the failure condition possible (pos + jump <= pos)? If it is possible to reach I'm not certain aborting is the right thing to do :)
I don't think this should be an assert. It can be caused by incorrect input (an corrupt asmap file), not just developer errors. We shouldn't use assert for error handling. Don't we have other ways of reporting an error here?
Updated to break which results in returning 0, which is not a valid ASN.
Tested on macOS 10.15.3:
% clang --version | head -1
Apple clang version 11.0.0 (clang-1100.0.33.17)
% brew info boost | head -1
boost: stable 1.72.0 (bottled), HEAD
% make > /dev/null
/Library/Developer/CommandLineTools/usr/bin/ranlib: file: libbitcoin_util.a(libbitcoin_util_a-strnlen.o) has no symbols
/Library/Developer/CommandLineTools/usr/bin/ranlib: file: libbitcoin_util.a(libbitcoin_util_a-sync.o) has no symbols
/Library/Developer/CommandLineTools/usr/bin/ranlib: file: libbitcoin_util.a(libbitcoin_util_a-string.o) has no symbols
/Library/Developer/CommandLineTools/usr/bin/ranlib: file: libbitcoin_util.a(libbitcoin_util_a-strnlen.o) has no symbols
/Library/Developer/CommandLineTools/usr/bin/ranlib: file: libbitcoin_util.a(libbitcoin_util_a-sync.o) has no symbols
/Library/Developer/CommandLineTools/usr/bin/ranlib: file: libbitcoin_util.a(libbitcoin_util_a-string.o) has no symbols
ACK acee61494d50d2821003dde6c888b9ed3ed1ddce
ACK acee61494d50d2821003dde6c888b9ed3ed1ddce modulo assert - see @hebasto's question above which needs to be addressed :)
ACK acee6149
Can remove some of the lines in test/sanitizer_suppressions/ubsan now?
Could also split up the test changes to merge them in the fast track? The Core changes seem controversial for now.
ACK b8a6a2de910aed711026a8114be5391135beedb3 -- patch looks correct
345 | @@ -345,6 +346,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then 346 | AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]]) 347 | AX_CHECK_COMPILE_FLAG([-Wunused-variable],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunused-variable"],,[[$CXXFLAG_WERROR]]) 348 | AX_CHECK_COMPILE_FLAG([-Wdate-time],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wdate-time"],,[[$CXXFLAG_WERROR]]) 349 | + AX_CHECK_COMPILE_FLAG([-Wsign-compare],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-compare"],,[[$CXXFLAG_WERROR]])
It is not necessary to add -Wsign-compare because it is enabled by -Wall in gcc and by -Wextra in clang. Just a few lines above we enable both -Wall and -Wextra.
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wsign-compare https://clang.llvm.org/docs/DiagnosticsReference.html#wextra
This was explicitly requested by @practicalswift, but maybe he did not realize that it is already enabled in both gcc and clang?
Leaving as it seems harmless to me.
83 | if (bits == 0) break;
84 | if (ip[ip.size() - bits]) {
85 | - if (jump >= endpos - pos) break;
86 | + if (pos + jump >= endpos) break;
87 | pos += jump;
88 | }
Shouldn't the added check for overflow be inside if (ip[ip.size() - bits])? If we don't get inside that if then we don't care whether it overflows because we would not do the arithmetic that would overflow.
nit: only 2 spaces used for indentation, the rest of the code uses 4.
Fixed, thanks.
tACK b8a6a2de910aed711026a8114be5391135beedb3
Thanks for doing this, compiling on macOS will be without warnings again thanks to this (see https://traviszci.org/github/bitcoin/bitcoin/jobs/678210021#L2026 for example). Not sure why these only showed up on macOS and not on Linux previously.
utACK b8a6a2d
Some comments below, feel free to ignore.
utACK ba1b64a
It is good to expand the reach of -Werror=
@Empact Looks good, but this one needs to be fixed for the ARM job to succeed :)
util/asmap.cpp: In function ‘bool SanityCheckASMap(const std::vector<bool>&, int)’:
util/asmap.cpp:159:22: error: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘std::ptrdiff_t’ {aka ‘int’} [-Werror=sign-compare]
if (jump > endpos - pos) return false; // Jump out of range
~~~~~^~~~~~~~~~~~~~
@Empact Looks good, but this one needs to be fixed for the ARM job to succeed :)
util/asmap.cpp: In function ‘bool SanityCheckASMap(const std::vector<bool>&, int)’: util/asmap.cpp:159:22: error: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘std::ptrdiff_t’ {aka ‘int’} [-Werror=sign-compare] if (jump > endpos - pos) return false; // Jump out of range ~~~~~^~~~~~~~~~~~~~
Also #18686.
Explicitly add -Wsign-compare as well - not required for all compilers, as GCC activates it
under -Wall, but may impact clang, etc.
ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
As noted in earlier comments: I'm confident this change will help us catch a non-zero amount of bugs hitting master going forward :)
re-ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
I went ahead and reverted the asmap changes, because they rely on UB: #21802