rpc: Fix implicit-integer-sign-change in verifychain #24347

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2202-rpcInt changing 2 files +2 −2
  1. MarcoFalke commented at 10:13 am on February 15, 2022: member

    It doesn’t really make sense to treat DEFAULT_CHECKLEVEL as unsigned as long as VerifyDB accepts a signed integer.

    Making it signed also avoids a cast round trip from signed->unsigned->signed in the RPC.

  2. rpc: Fix implicit-integer-sign-change in verifychain fa8dad0e07
  3. MarcoFalke force-pushed on Feb 15, 2022
  4. MarcoFalke added the label Refactoring on Feb 15, 2022
  5. MarcoFalke commented at 10:41 am on February 15, 2022: member

    Test plan (with ubsan)

    Fuzz

    0echo 'dmVyaWZ5Y2hhaW5c//////9wc2J0/wAAAAAAfv/i4uL///////9/BQX/////Cg==' | base64 --decode > /tmp/crash
1UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=rpc ./src/test/fuzz/fuzz /tmp/crash

     0rpc/blockchain.cpp:1420:77: runtime error: implicit conversion from type 'int' of value -30 (32-bit, signed) to type 'unsigned int' changed the value to 4294967266 (32-bit, unsigned)
 1    [#0](/bitcoin-bitcoin/0/) 0x55df91a807d6 in verifychain()::$_20::operator()(RPCHelpMan const&, JSONRPCRequest const&) const src/rpc/blockchain.cpp:1420:77
 2    [#1](/bitcoin-bitcoin/1/) 0x55df91a807d6 in std::_Function_handler<UniValue (RPCHelpMan const&, JSONRPCRequest const&), verifychain()::$_20>::_M_invoke(std::_Any_data const&, RPCHelpMan const&, JSONRPCRequest const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
 3    [#2](/bitcoin-bitcoin/2/) 0x55df91f1acaf in std::function<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
 4    [#3](/bitcoin-bitcoin/3/) 0x55df91f18910 in RPCHelpMan::HandleRequest(JSONRPCRequest const&) const src/rpc/util.cpp:576:26
 5    [#4](/bitcoin-bitcoin/4/) 0x55df91a4679a in CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const src/./rpc/server.h:109:91
 6    [#5](/bitcoin-bitcoin/5/) 0x55df91a4636f in std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
 7    [#6](/bitcoin-bitcoin/6/) 0x55df91886507 in std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
 8    [#7](/bitcoin-bitcoin/7/) 0x55df91bcf43a in ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) src/rpc/server.cpp:480:20
 9    [#8](/bitcoin-bitcoin/8/) 0x55df91bc9b39 in ExecuteCommands(std::vector<CRPCCommand const*, std::allocator<CRPCCommand const*> > const&, JSONRPCRequest const&, UniValue&) src/rpc/server.cpp:444:13
10    [#9](/bitcoin-bitcoin/9/) 0x55df91bc96a7 in CRPCTable::execute(JSONRPCRequest const&) const src/rpc/server.cpp:464:13
11    [#10](/bitcoin-bitcoin/10/) 0x55df913e561a in (anonymous namespace)::RPCFuzzTestingSetup::CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) src/test/fuzz/rpc.cpp:54:18
12    [#11](/bitcoin-bitcoin/11/) 0x55df913e561a in rpc_fuzz_target(Span<unsigned char const>) src/test/fuzz/rpc.cpp:361:28
13    [#12](/bitcoin-bitcoin/12/) 0x55df911bd208 in std::_Function_handler<void (Span<unsigned char const>), void (*)(Span<unsigned char const>)>::_M_invoke(std::_Any_data const&, Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:300:2
14    [#13](/bitcoin-bitcoin/13/) 0x55df92170a9d in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
15    [#14](/bitcoin-bitcoin/14/) 0x55df92170748 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:119:5
16    [#15](/bitcoin-bitcoin/15/) 0x55df910c9f93 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x14c0f93) (BuildId: cf5bb89b801a533254c4db373c3faadf43035367)
17    [#16](/bitcoin-bitcoin/16/) 0x55df910b433f in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x14ab33f) (BuildId: cf5bb89b801a533254c4db373c3faadf43035367)
18    [#17](/bitcoin-bitcoin/17/) 0x55df910ba056 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x14b1056) (BuildId: cf5bb89b801a533254c4db373c3faadf43035367)
19    [#18](/bitcoin-bitcoin/18/) 0x55df910e36d2 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x14da6d2) (BuildId: cf5bb89b801a533254c4db373c3faadf43035367)
20    [#19](/bitcoin-bitcoin/19/) 0x7f474c8d10b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
21    [#20](/bitcoin-bitcoin/20/) 0x55df910aeacd in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x14a5acd) (BuildId: cf5bb89b801a533254c4db373c3faadf43035367)
22
23SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change rpc/blockchain.cpp:1420:77 in

    bitcoind/bitcoin-qt

    verifychain -1 1

  6. LarryRuane commented at 2:43 pm on February 16, 2022: contributor
    I can’t reproduce the problem using fuzz. I’m using Ubuntu 21.10 and clang 14.0.0, in case that matters. The only unusual thing in my configuration is that it’s build without optimization (built using -O0).
  7. MarcoFalke commented at 2:56 pm on February 16, 2022: member
    You’ll need to enable ubsan. What is the configure summary?
  8. LarryRuane commented at 3:14 pm on February 16, 2022: contributor

    Is there a way to just print the config? Here’s how I did the build:

    0./autogen.sh && ./configure CXXFLAGS='-O0 -g' CC=clang-14 CXX=clang++-14 --enable-fuzz --with-sanitizers=address,fuzzer,undefined && make
  9. MarcoFalke commented at 7:22 am on February 17, 2022: member
    I can also reproduce with -O0. Please share the exact steps to reproduce
  10. Meru852 commented at 7:30 am on February 17, 2022: none

    I can’t reproduce the problem using fuzz. I’m using Ubuntu 21.10 and clang 14.0.0, in case that matters. The only unusual thing in my configuration is that it’s build without optimization (built using -O0).

    install for more command to the ubuntu 2110 and get more note for that ubuntu type

  11. LarryRuane commented at 9:25 pm on February 18, 2022: contributor

    Please share the exact steps to reproduce

    Some things about my environment:

     0$ cat /etc/os-release # (I'll abbreviate the output of just this command slightly)
 1PRETTY_NAME="Ubuntu 21.10"
 2NAME="Ubuntu"
 3VERSION_ID="21.10"
 4VERSION="21.10 (Impish Indri)"
 5VERSION_CODENAME=impish
 6ID=ubuntu
 7ID_LIKE=debian
 8$ uname -a
 9Linux larry-VirtualBox 5.13.0-28-generic [#31](/bitcoin-bitcoin/31/)-Ubuntu SMP Thu Jan 13 17:41:06 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
10$ clang++-14 --version
11Ubuntu clang version 14.0.0-++20220216033948+c561bf0daa95-1~exp1~20220216153959.57
12Target: x86_64-pc-linux-gnu
13Thread model: posix
14InstalledDir: /usr/bin
15$

    Build, this time without -O0, starting in a fresh clone of master:

    0$ git checkout MarcoFalke/2202-rpcInt 
1$ git checkout HEAD~ # back up one commit so that we don't have the fix
2Previous HEAD position was fa8dad0e0 rpc: Fix implicit-integer-sign-change in verifychain
3HEAD is now at 7164e00e1 Merge bitcoin/bitcoin#24324: test: refactor: remove unneeded bytes<->hex conversions in `byte_to_base58`
4$ ./autogen.sh && ./configure CC=clang-14 CXX=clang++-14 --enable-fuzz --with-sanitizers=address,fuzzer,undefined --without-miniupnpc --disable-wallet --without-gui && make -C src test/fuzz/fuzz
5$

    Run the test

     0$ echo 'dmVyaWZ5Y2hhaW5c//////9wc2J0/wAAAAAAfv/i4uL///////9/BQX/////Cg==' | base64 --decode > /tmp/crash
 1$ UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=rpc ./src/test/fuzz/fuzz /tmp/crash
 2INFO: Running with entropic power schedule (0xFF, 100).
 3INFO: Seed: 302550202
 4INFO: Loaded 1 modules   (229102 inline 8-bit counters): 229102 [0x5621d0f1b2a0, 0x5621d0f5318e), 
 5INFO: Loaded 1 PC tables (229102 PCs): 229102 [0x5621d0f53190,0x5621d12d2070), 
 6./src/test/fuzz/fuzz: Running 1 inputs 1 time(s) each.
 7Running: /tmp/crash
 8Executed /tmp/crash in 1 ms
 9***
10*** NOTE: fuzzing was not performed, you have only
11***       executed the target code on a fixed set of inputs.
12***
13$
  12. luke-jr approved
  13. luke-jr commented at 11:51 pm on February 18, 2022: member
    utACK fa8dad0e078c577d740a9667636733957586c035
  14. MarcoFalke commented at 1:21 pm on February 19, 2022: member
    I can reproduce, but this looks like an upstream bug, unrelated to the changes here, so let’s continue discussion in #24394.
  15. MarcoFalke commented at 7:14 pm on February 19, 2022: member
    Oh, can you try again with integer sanitizer? --with-sanitizers=integer,fuzzer,undefined
  16. theStack approved
  17. theStack commented at 6:41 pm on February 20, 2022: member

    Code-review ACK fa8dad0e078c577d740a9667636733957586c035

    Related problem: could also change the check_level parameter of VerifyLoadedChainstate from unsigned int to int, in order to avoid another cast roundtrip from signed->unsigned->signed in the init procedure (AppInitMain), either in this PR or a follow-up. The same probably also for the check_blocks parameter (didn’t check this one in detail though).

  18. MarcoFalke merged this on Feb 21, 2022
  19. MarcoFalke closed this on Feb 21, 2022
  20. MarcoFalke deleted the branch on Feb 21, 2022
  21. MarcoFalke commented at 9:32 am on February 21, 2022: member
    Thanks, done in #24403
  22. sidhujag referenced this in commit c166d85cf0 on Feb 22, 2022
  23. DrahtBot locked this on Feb 21, 2023


MarcoFalke LarryRuane Meru852 luke-jr theStack

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me