Fix integer sanitizer suppressions in validation.cpp #24196

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2201-valInt changing 2 files +3 −3

MarcoFalke commented at 4:38 PM on January 28, 2022: member

It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.

Fix it with a refactor and remove the suppression.
MarcoFalke added the label Refactoring on Jan 28, 2022
DrahtBot commented at 4:00 AM on January 29, 2022: member



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



Conflicts

No conflicts as of last run.
luke-jr approved
luke-jr commented at 3:51 AM on January 31, 2022: member

int is only guaranteed to have a max value of 32768. In a 2 MB block, that's 61 bytes per input. Seems quite possible to hit it?

OTOH, I don't think we support/work on such platforms right now, so this probably isn't a real issue. So utACK anyway.

MarcoFalke commented at 7:13 AM on January 31, 2022: member

It wouldn't be possible to start Bitcoin Core if int max was 32768. See also:

src/compat/assumptions.h:static_assert(sizeof(short) == 2, "16-bit short assumed");
src/compat/assumptions.h:static_assert(sizeof(int) == 4, "32-bit int assumed");
src/compat/assumptions.h:static_assert(sizeof(unsigned) == 4, "32-bit unsigned assumed");
src/compat/assumptions.h:static_assert(sizeof(size_t) == 4 || sizeof(size_t) == 8, "size_t assumed to be 32-bit or 64-bit");

in src/validation.cpp:1790 in fa2d0c7008 outdated

1786 | @@ -1787,8 +1787,8 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
1787 |                  error("DisconnectBlock(): transaction and undo data inconsistent");
1788 |                  return DISCONNECT_FAILED;
1789 |              }
1790 | -            for (unsigned int j = tx.vin.size(); j-- > 0;) {
1791 | -                const COutPoint &out = tx.vin[j].prevout;
1792 | +            for (int j = int(tx.vin.size()); j-- > 0;) {

PastaPastaPasta commented at 5:15 AM on February 1, 2022:

nit: Is this equivalent to below? If so, is that more clear?

            for (int j = int(tx.vin.size()) - 1; j >= 0; j--) {

MarcoFalke commented at 2:32 PM on February 7, 2022:

pushed something else

PastaPastaPasta approved
PastaPastaPasta commented at 5:18 AM on February 1, 2022: contributor

utACK fa2d0c7008864384012c1bd84a602b85231f5983 modulo nit
MarcoFalke marked this as a draft on Feb 1, 2022
MarcoFalke force-pushed on Feb 1, 2022
Fix integer sanitizer suppressions in validation.cpp fac62056b5
MarcoFalke force-pushed on Feb 7, 2022
MarcoFalke marked this as ready for review on Feb 7, 2022
MarcoFalke commented at 2:33 PM on February 7, 2022: member

Changed to a pure refactor, that doesn't change any types. Also, it doesn't change the binary with clang++ -O2 on my system, though with gcc it does. It is the same refactor that was used in #https://github.com/bitcoin/bitcoin/pull/24227
hebasto approved
hebasto commented at 2:40 PM on February 7, 2022: member

ACK fac62056b56e0a28baf0b6f285752d83fbf96074, I have reviewed the code and it looks OK, I agree it can be merged.
unknown approved
unknown commented at 2:51 PM on February 7, 2022: none

Code Review ACK https://github.com/bitcoin/bitcoin/pull/24196/commits/fac62056b56e0a28baf0b6f285752d83fbf96074
MarcoFalke merged this on Feb 9, 2022
MarcoFalke closed this on Feb 9, 2022
MarcoFalke deleted the branch on Feb 9, 2022
sidhujag referenced this in commit 89a770bc17 on Feb 9, 2022
Fabcien referenced this in commit 295dde9e88 on Dec 9, 2022
DrahtBot locked this on Feb 9, 2023

Contributors

Labels

Fix integer sanitizer suppressions in validation.cpp #24196

Conflicts