mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-09-13 06:04:42 +02:00
5223cf1795
fabafd91f1refactor: Use u64 over size_t for all cache sizes to fix a 32-bit overflow (MarcoFalke) Pull request description: This is a refactor on 64-bit systems, because size_t is equal to u64. However, on 32-bit systems, it fixes an integer overflow while calculating the cache sizes: ``` src/node/caches.cpp:71:49: runtime error: unsigned integer overflow: 471859200 * 10 cannot be represented in type size_t (aka "unsigned int") ``` This happens while multiplying the default cache size (450MiB) by 10: ``` index_sizes.tx_index = std::min(total_cache * 10 / 100, ...) ^^^^^^^^^^^^^^^^ ``` The issue was introduced in commitd06dabf26b. ---- This change follows similar changed one in the past, like3789215f73,ac76d94117, or28a523fb94. Generally, using fixed sized integer types for calculations is beneficial, because all platforms behave exactly the same way. With platform-dependent types there is a risk that the same calculation yields different results. This has several resulting benefits: * Easier review, because there is no need to review the same code several times for each supported platform. * Easier quality assurance, because there is less need to run the same code several times in sanitizers for each supported platform, which is [tedious](https://github.com/bitcoin/bitcoin/issues/32375#issuecomment-4825318068). There are also no downsides, because there is no measurable overhead on 32-bit for u64 calculations that are done only once in the lifetime of the program. Also, there is no measurable memory overhead when a few fields on 32-bit store some extra zero bytes. ---- As said, testing is only possible by picking one of the tedious options: * Apply a diff on 64-bit arch and compile with `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=integer` ```diff diff --git a/src/node/caches.cpp b/src/node/caches.cpp index c98b8ce604..cfd49b60d3 100644 --- a/src/node/caches.cpp +++ b/src/node/caches.cpp @@ -58,3 +58,3 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes) { - size_t total_cache{CalculateDbCacheBytes(args)}; + uint32_t total_cache(CalculateDbCacheBytes(args)); @@ -72,6 +72,6 @@ CacheSizes CalculateCacheSizes(const ArgsManager& args, size_t n_indexes) IndexCacheSizes index_sizes; - index_sizes.tx_index = std::min(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0); - index_sizes.txospender_index = std::min(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0); + index_sizes.tx_index = std::min<uint32_t>(total_cache * 10 / 100, args.GetBoolArg("-txindex", DEFAULT_TXINDEX) ? MAX_TX_INDEX_CACHE : 0); + index_sizes.txospender_index = std::min<uint32_t>(total_cache * 5 / 100, args.GetBoolArg("-txospenderindex", DEFAULT_TXOSPENDERINDEX) ? MAX_TXOSPENDER_INDEX_CACHE : 0); if (n_indexes > 0) { - size_t max_cache = std::min(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE); + size_t max_cache = std::min<uint32_t>(total_cache * 5 / 100, MAX_FILTER_INDEX_CACHE); index_sizes.filter_index = max_cache / n_indexes; ``` This will give a roughly similar error: ``` sh-5.3$ echo 'Bw==' | base64 -d > /tmp/blob sh-5.3$ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob ./src/node/caches.cpp:73:59: runtime error: unsigned integer overflow: 1073741824 * 10 cannot be represented in type 'uint32_t' (aka 'unsigned int') #0 0x55ca78da5c47 in node::CalculateCacheSizes(ArgsManager const&, unsigned long) ./src/node/caches.cpp:73:59 ``` * Alternatively, to reproduce in a fresh `podman run -it --rm --platform linux/i386 debian:unstable`: ``` export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev mold -y && cmake -B ./bld-cmake -DAPPEND_CXXFLAGS='-O3 -g2' -DAPPEND_CFLAGS='-O3 -g2' -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXE_LINKER_FLAGS=-fuse-ld=mold -DCMAKE_C_COMPILER='clang;-ftrivial-auto-var-init=pattern' -DCMAKE_CXX_COMPILER='clang++;-ftrivial-auto-var-init=pattern' -DSANITIZERS=address,float-divide-by-zero,integer,undefined --preset=dev-mode && cmake --build ./bld-cmake --parallel $(nproc) echo 'Bw==' | base64 -d > /tmp/blob UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=block_index_tree ./bld-cmake/bin/fuzz /tmp/blob # or: UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ASAN_OPTIONS="detect_leaks=0" ./bld-cmake/bin/test_bitcoin-qt ``` ACKs for top commit: l0rinc: ACKfabafd91f1sedited: Re-ACKfabafd91f1theStack: re-ACKfabafd91f1Tree-SHA512: cebbc29636b4074917c96cf3af8fcc176dcd328821d5032dc8609475e18778dfda4e287ded889c024ab29b79d81004bc9a8e57a88eaa0a5eaefe5f8bba1462ab
src/node/
The src/node/ directory contains code that needs to access node state
(state in CChain, CBlockIndex, CCoinsView, CTxMemPool, and similar
classes).
Code in src/node/ is meant to be segregated from code in
src/wallet/ and src/qt/, to ensure wallet and GUI
code changes don't interfere with node operation, to allow wallet and GUI code
to run in separate processes, and to perhaps eventually allow wallet and GUI
code to be maintained in separate source repositories.
As a rule of thumb, code in one of the src/node/,
src/wallet/, or src/qt/ directories should avoid
calling code in the other directories directly, and only invoke it indirectly
through the more limited src/interfaces/ classes.
This directory is at the moment
sparsely populated. Eventually more substantial files like
src/validation.cpp and
src/txmempool.cpp might be moved there.