test, build: Enable -Werror=sign-compare #18216

Empact · 2020-02-27T23:43:14Z

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: 22d1118 vs 75fb37c

fanquake · 2020-02-28T00:44:08Z

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;
                     ~~~~~^~~~~~~~~~~~~~~

Empact · 2020-02-28T03:07:13Z

@fanquake Thanks, addressed with f35268d

DrahtBot · 2020-02-28T05:06:35Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: avoid repetitions when enabling warnings in configure.ac #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
Span improvements #18468 (Span improvements by sipa)
build: Enable -Wsuggest-override if available #16710 (build: Enable -Wsuggest-override if available by hebasto)
Wallet passive startup #15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)
Make script interpreter independent from storage type CScript #13062 (Make script interpreter independent from storage type CScript by sipa)

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.

practicalswift · 2020-02-29T20:12:56Z

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 416f3be modulo above suggestion

practicalswift · 2020-02-29T20:16:56Z

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! :)

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

practicalswift

ACK acee614

hebasto · 2020-03-10T15:23:48Z

Concept ACK.

hebasto · 2020-03-10T21:34:05Z

src/util/asmap.cpp

@@ -77,9 +77,10 @@ uint32_t Interpret(const std::vector<bool> &asmap, const std::vector<bool> &ip)
            return DecodeASN(pos, endpos);
        } else if (opcode == 1) {
            jump = DecodeJump(pos, endpos);
+            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.

hebasto · 2020-03-10T22:28:37Z

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

hebasto

ACK acee614

practicalswift · 2020-03-11T06:45:16Z

ACK acee614 modulo assert - see @hebasto's question above which needs to be addressed :)

jonatack

ACK acee614

maflcko · 2020-04-22T20:11:26Z

Can remove some of the lines in test/sanitizer_suppressions/ubsan now?

maflcko · 2020-04-22T21:25:00Z

Could also split up the test changes to merge them in the fast track? The Core changes seem controversial for now.

practicalswift · 2020-04-23T10:42:34Z

ACK b8a6a2d -- patch looks correct

Empact · 2020-05-08T18:22:35Z

Rebased for #18512

practicalswift · 2020-05-08T20:06:47Z

@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
                 ~~~~~^~~~~~~~~~~~~~

hebasto · 2020-05-09T03:35:19Z

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

practicalswift · 2020-05-09T10:03:48Z

ACK 6853727

As noted in earlier comments: I'm confident this change will help us catch a non-zero amount of bugs hitting master going forward :)

fjahr · 2020-05-10T13:49:18Z

re-ACK 6853727

Removes the unused function warning when building on macOS. Now that bitcoin#18216 is in, with this change we'll be back to warningless builds on macOS. random.cpp:259:13: warning: unused function 'GetDevURandom' [-Wunused-function] static void GetDevURandom(unsigned char *ent32) ^ 1 warning generated.

Summary: test: Fix outstanding -Wsign-compare errors refactor: Rework asmap Interpret to avoid ptrdiff_t This is a backport of Core [[bitcoin/bitcoin#18216 | PR18216]] It also includes a change in asmap.cpp that was missed in D9055 ([[bitcoin/bitcoin#18512 | PR18512]]) Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D9197

maflcko · 2021-04-29T06:56:51Z

I went ahead and reverted the asmap changes, because they rely on UB: #21802

6853727 build: Enable -Werror=sign-compare (Ben Woosley) eac6a30 refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley) df37377 test: Fix outstanding -Wsign-compare errors (Ben Woosley) Pull request description: 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 bitcoin#17398. You can test that by building this branch against: 22d1118 vs 75fb37c ACKs for top commit: fjahr: re-ACK 6853727 practicalswift: ACK 6853727 Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c

fanquake added Build system Tests labels Feb 27, 2020

Empact force-pushed the 2020-02-sign-compare branch 2 times, most recently from 7ab2e6c to 416f3be Compare February 28, 2020 01:40

DrahtBot mentioned this pull request Feb 28, 2020

build: add --enable-isystem and change --enable-werror to enable -Werror #18149

Closed

fanquake requested a review from practicalswift February 28, 2020 08:00

This was referenced Feb 28, 2020

build: Optionally enable -Wzero-as-null-pointer-constant #15112

Closed

Build: enable -Wdocumentation via isystem #14920

Closed

Empact force-pushed the 2020-02-sign-compare branch from 416f3be to acee614 Compare February 29, 2020 22:08

practicalswift approved these changes Feb 29, 2020

View reviewed changes

DrahtBot mentioned this pull request Mar 1, 2020

build: Enable -Wsuggest-override if available #16710

Merged

hebasto reviewed Mar 10, 2020

View reviewed changes

hebasto approved these changes Mar 10, 2020

View reviewed changes

jonatack reviewed Mar 16, 2020

View reviewed changes

This was referenced Mar 30, 2020

Span improvements #18468

Merged

Make script interpreter independent from storage type CScript #13062

Closed

Improve asmap checks and add sanity check #18512

Merged

DrahtBot mentioned this pull request Apr 9, 2020

[RFC] bitcoin-asmap utility #18573

Closed

Empact force-pushed the 2020-02-sign-compare branch from acee614 to b8a6a2d Compare April 23, 2020 02:09

This was referenced May 1, 2020

build: warn on potentially uninitialized reads #18843

Merged

The Zero Allocations project #18849

Closed

build: avoid repetitions when enabling warnings in configure.ac #18857

Closed

DrahtBot added the Needs rebase label May 6, 2020

test: Fix outstanding -Wsign-compare errors

df37377

Empact force-pushed the 2020-02-sign-compare branch from ba1b64a to ab36ecf Compare May 8, 2020 18:19

DrahtBot removed the Needs rebase label May 8, 2020

DrahtBot mentioned this pull request May 9, 2020

Wallet passive startup #15719

Closed

Empact force-pushed the 2020-02-sign-compare branch from ab36ecf to 054cc19 Compare May 9, 2020 07:16

Empact added 2 commits May 9, 2020 00:20

refactor: Rework asmap Interpret to avoid ptrdiff_t

eac6a30

build: Enable -Werror=sign-compare

6853727

Explicitly add -Wsign-compare as well - not required for all compilers, as GCC activates it under -Wall, but may impact clang, etc.

Empact force-pushed the 2020-02-sign-compare branch from 054cc19 to 6853727 Compare May 9, 2020 07:20

fanquake merged commit ec4d27f into bitcoin:master May 11, 2020

LarryRuane added a commit to LarryRuane/bitcoin that referenced this pull request Jul 13, 2020

fix CI failures, same changes as bitcoin#18216

ee42af0

Empact deleted the 2020-02-sign-compare branch April 13, 2021 16:49

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test, build: Enable -Werror=sign-compare #18216

test, build: Enable -Werror=sign-compare #18216

Empact commented Feb 27, 2020 •

edited

Loading

fanquake commented Feb 28, 2020

Empact commented Feb 28, 2020

DrahtBot commented Feb 28, 2020 •

edited

Loading

practicalswift commented Feb 29, 2020 •

edited

Loading

practicalswift commented Feb 29, 2020

practicalswift left a comment

hebasto commented Mar 10, 2020

hebasto Mar 10, 2020

Empact Mar 11, 2020

practicalswift Mar 11, 2020 •

edited

Loading

laanwj Apr 22, 2020 •

edited

Loading

Empact Apr 23, 2020

hebasto commented Mar 10, 2020

hebasto left a comment

practicalswift commented Mar 11, 2020

jonatack left a comment

maflcko commented Apr 22, 2020

maflcko commented Apr 22, 2020

practicalswift commented Apr 23, 2020

Empact commented May 8, 2020

practicalswift commented May 8, 2020

hebasto commented May 9, 2020

practicalswift commented May 9, 2020

fjahr commented May 10, 2020

maflcko commented Apr 29, 2021

test, build: Enable -Werror=sign-compare #18216

test, build: Enable -Werror=sign-compare #18216

Conversation

Empact commented Feb 27, 2020 • edited Loading

fanquake commented Feb 28, 2020

Empact commented Feb 28, 2020

DrahtBot commented Feb 28, 2020 • edited Loading

Conflicts

practicalswift commented Feb 29, 2020 • edited Loading

practicalswift commented Feb 29, 2020

practicalswift left a comment

Choose a reason for hiding this comment

hebasto commented Mar 10, 2020

hebasto Mar 10, 2020

Choose a reason for hiding this comment

Empact Mar 11, 2020

Choose a reason for hiding this comment

practicalswift Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

laanwj Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Empact Apr 23, 2020

Choose a reason for hiding this comment

hebasto commented Mar 10, 2020

hebasto left a comment

Choose a reason for hiding this comment

practicalswift commented Mar 11, 2020

jonatack left a comment

Choose a reason for hiding this comment

maflcko commented Apr 22, 2020

maflcko commented Apr 22, 2020

practicalswift commented Apr 23, 2020

Empact commented May 8, 2020

practicalswift commented May 8, 2020

hebasto commented May 9, 2020

practicalswift commented May 9, 2020

fjahr commented May 10, 2020

maflcko commented Apr 29, 2021

Empact commented Feb 27, 2020 •

edited

Loading

DrahtBot commented Feb 28, 2020 •

edited

Loading

practicalswift commented Feb 29, 2020 •

edited

Loading

practicalswift Mar 11, 2020 •

edited

Loading

laanwj Apr 22, 2020 •

edited

Loading