Add copy constructor and copy operator= to CScript to remove ubsan suppression

achow101 commented at 6:30 pm on November 18, 2019: member

As noted in #17156 (comment), the PSBT fuzzer is triggering a UBSan condition which is fixed by this PR. Additionally, by fixing this, some UBSan suppressions can be removed.

Split from #17156

Add copy constructor and copy operator= to CScript d86e9180ac

DrahtBot added the label Consensus on Nov 18, 2019

DrahtBot added the label Tests on Nov 18, 2019

in src/script/script.h:415 in d86e9180ac

411@@ -412,6 +412,9 @@ class CScript : public CScriptBase
412     CScript(std::vector<unsigned char>::const_iterator pbegin, std::vector<unsigned char>::const_iterator pend) : CScriptBase(pbegin, pend) { }
413     CScript(const unsigned char* pbegin, const unsigned char* pend) : CScriptBase(pbegin, pend) { }
414 
415+    // Define a copy constructor so that operator= is a copy instead of a move to avoid undefined behavior

sipa commented at 6:36 pm on November 18, 2019:

If this is needed, there is another problem. Why does a move cause UB? And why is a move being invoked in the first place with operator=?

achow101 commented at 7:44 pm on November 18, 2019:

No idea. Maybe @practicalswift can shed some light here?

Sjors commented at 10:58 am on November 19, 2019:

See also: #17156 (comment)

MarcoFalke removed the label Tests on Nov 18, 2019

MarcoFalke added the label Brainstorming on Nov 18, 2019

MarcoFalke commented at 9:06 pm on November 18, 2019: member

For reference, one tb is this:

 0vim $(pwd)/test/sanitizer_suppressions # Remove the prevector suppressions 
 1./configure  --with-sanitizers=address,undefined CC=clang CXX=clang++
 2make
 3export LSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/lsan"
 4export TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan"
 5export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1"
 6$ ./src/test/test_bitcoin -t denialofservice_tests
 7Running 6 test cases...
 8prevector.h:452:19: runtime error: reference binding to misaligned address 0x7ffdaaebe012 for type 'prevector<28, unsigned char, unsigned int, int>::size_type' (aka 'unsigned int'), which requires 4 byte alignment
 90x7ffdaaebe012: note: pointer points here
10 00 00  00 00 00 00 00 00 6c 00  00 00 20 9e 01 00 b0 60  00 00 00 00 00 00 00 00  00 00 00 00 00 00
11              ^ 
12    [#0](/bitcoin-bitcoin/0/) 0x56076599c81d in prevector<28u, unsigned char, unsigned int, int>::swap(prevector<28u, unsigned char, unsigned int, int>&) ./src/./prevector.h:452:9
13    [#1](/bitcoin-bitcoin/1/) 0x560765991f58 in prevector<28u, unsigned char, unsigned int, int>::operator=(prevector<28u, unsigned char, unsigned int, int>&&) ./src/./prevector.h:272:9
14    [#2](/bitcoin-bitcoin/2/) 0x560765991f58 in CScript::operator=(CScript&&) ./src/./script/script.h:390
15    [#3](/bitcoin-bitcoin/3/) 0x5607673635b5 in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) ./src/script/sign.cpp:240:23
16    [#4](/bitcoin-bitcoin/4/) 0x56076736766c in SignSignature(SigningProvider const&, CScript const&, CMutableTransaction&, unsigned int, long const&, int) ./src/script/sign.cpp:375:16
17    [#5](/bitcoin-bitcoin/5/) 0x5607673679bb in SignSignature(SigningProvider const&, CTransaction const&, CMutableTransaction&, unsigned int, int) ./src/script/sign.cpp:387:12
18    [#6](/bitcoin-bitcoin/6/) 0x560765d5c515 in denialofservice_tests::DoS_mapOrphans::test_method() ./src/test/denialofservice_tests.cpp:403:9
19    [#7](/bitcoin-bitcoin/7/) 0x560765d5a4bc in denialofservice_tests::DoS_mapOrphans_invoker() ./src/test/denialofservice_tests.cpp:369:1
20    [#8](/bitcoin-bitcoin/8/) 0x560765aa4cc5 in boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) /usr/include/boost/function/function_template.hpp:117:11
21    [#9](/bitcoin-bitcoin/9/) 0x7fa1a39b7581  (/lib64/libboost_unit_test_framework.so.1.69.0+0x30581)
22    [#10](/bitcoin-bitcoin/10/) 0x7fa1a39b65ec in boost::execution_monitor::catch_signals(boost::function<int ()> const&) (/lib64/libboost_unit_test_framework.so.1.69.0+0x2f5ec)
23    [#11](/bitcoin-bitcoin/11/) 0x7fa1a39b6677 in boost::execution_monitor::execute(boost::function<int ()> const&) (/lib64/libboost_unit_test_framework.so.1.69.0+0x2f677)
24    [#12](/bitcoin-bitcoin/12/) 0x7fa1a39b674d in boost::execution_monitor::vexecute(boost::function<void ()> const&) (/lib64/libboost_unit_test_framework.so.1.69.0+0x2f74d)
25    [#13](/bitcoin-bitcoin/13/) 0x7fa1a39e124e in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned int) (/lib64/libboost_unit_test_framework.so.1.69.0+0x5a24e)
26    [#14](/bitcoin-bitcoin/14/) 0x7fa1a39c667f  (/lib64/libboost_unit_test_framework.so.1.69.0+0x3f67f)
27    [#15](/bitcoin-bitcoin/15/) 0x7fa1a39c6913  (/lib64/libboost_unit_test_framework.so.1.69.0+0x3f913)
28    [#16](/bitcoin-bitcoin/16/) 0x7fa1a39c6913  (/lib64/libboost_unit_test_framework.so.1.69.0+0x3f913)
29    [#17](/bitcoin-bitcoin/17/) 0x7fa1a39bdc73 in boost::unit_test::framework::run(unsigned long, bool) (/lib64/libboost_unit_test_framework.so.1.69.0+0x36c73)
30    [#18](/bitcoin-bitcoin/18/) 0x7fa1a39e00d1 in boost::unit_test::unit_test_main(bool (*)(), int, char**) (/lib64/libboost_unit_test_framework.so.1.69.0+0x590d1)
31    [#19](/bitcoin-bitcoin/19/) 0x5607659a1526 in main /usr/include/boost/test/unit_test.hpp:63:12
32    [#20](/bitcoin-bitcoin/20/) 0x7fa1a2f8df42 in __libc_start_main (/lib64/libc.so.6+0x23f42)
33    [#21](/bitcoin-bitcoin/21/) 0x560765861f8d in _start (./src/test/test_bitcoin+0x2a59f8d)
34
35SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior prevector.h:452:19 in

DrahtBot commented at 9:47 pm on November 18, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17208 (Make all tests pass UBSan without using any UBSan suppressions by practicalswift)
#10785 (Serialization improvements 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.

laanwj commented at 8:00 am on November 19, 2019: member

If this is needed, there is another problem. Why does a move cause UB? And why is a move being invoked in the first place with operator=?

Agree. This is kind of scary, and working around it like this seems a bad idea if we don’t understand why it happens.

laanwj commented at 8:51 am on November 19, 2019: member

BTW, this reverts 2ddfcfd2d67bc2bd8aa4682ceaba6a59614e54d1 (#9349), with the difference that the new code invokes CScript instead of CScriptBase.

laanwj commented at 9:01 am on November 19, 2019: member

Could this pragma in prevector.h be causing alignment issues? (doesn’t it mean something like “ignore all padding requirements within the struct”)

0#pragma pack(push, 1)

MarcoFalke commented at 1:45 pm on November 19, 2019: member

Smaller code to reproduce:

 0struct Test {
 1    uint8_t a : 8;
 2    CScript script;
 3};
 4
 5
 6{
 7    const std::array<unsigned char, 5> D{1, 2, 3, 4, 5};
 8    Test t{0x0f, {}};
 9    t.script = CScript{D.begin(), D.end()};
10}

Result:

0prevector.h:452:19: runtime error: reference binding to misaligned address 0x7ffe6446c741 for type 'prevector<28, unsigned char, unsigned int, int>::size_type' (aka 'unsigned int'), which requires 4 byte alignment
10x7ffe6446c741: note: pointer points here
2 7f 00 00  0f 00 00 00 00 01 02 03  04 05 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00
3              ^ 
4    [#0](/bitcoin-bitcoin/0/) 0x555dd33f9a7d in prevector<28u, unsigned char, unsigned int, int>::swap(prevector<28u, unsigned char, unsigned int, int>&) /home/marco/workspace/btc_bitcoin_core/src/./prevector.h:452:9
5    [#1](/bitcoin-bitcoin/1/) 0x555dd33ef1b8 in prevector<28u, unsigned char, unsigned int, int>::operator=(prevector<28u, unsigned char, unsigned int, int>&&) /home/marco/workspace/btc_bitcoin_core/src/./prevector.h:272:9
6    [#2](/bitcoin-bitcoin/2/) 0x555dd33ef1b8 in CScript::operator=(CScript&&) /home/marco/workspace/btc_bitcoin_core/src/./script/script.h:390
7    [#3](/bitcoin-bitcoin/3/) 0x555dd37a0aaa in a() /tmp/a.cpp:79:11

laanwj commented at 1:50 pm on November 19, 2019: member

Does it go away if you remove the pragma?

MarcoFalke commented at 2:13 pm on November 19, 2019: member

Yes, with this diff it goes away:

 0diff --git a/src/prevector.h b/src/prevector.h
 1index d307495fbe..a55bcb50fe 100644
 2--- a/src/prevector.h
 3+++ b/src/prevector.h
 4@@ -14,7 +14,6 @@
 5 #include <cstddef>
 6 #include <type_traits>
 7 
 8-#pragma pack(push, 1)
 9 /** Implements a drop-in replacement for std::vector<T> which stores up to N
10  *  elements directly (without heap allocation). The types Size and Diff are
11  *  used to store element counts, and can be any unsigned + signed type.
12@@ -522,6 +521,5 @@ public:
13         return item_ptr(0);
14     }
15 };
16-#pragma pack(pop)
17 
18 #endif // BITCOIN_PREVECTOR_H

laanwj commented at 2:43 pm on November 19, 2019: member

I would prefer that solution, then. I’m not sure in how far unaligned integers are really UB in C++ (versus architecture/implementation defined), but it’d be better to avoid that for architectures that don’t support (or have slow fallback paths for) unaligned reads/writes. (E.g. SiFive U540 traps into the kernel/bootloader to handle unaligned reads and writes, with a function, in software. Wouldn’t be surprised if it’s 1000 times slower)

practicalswift commented at 5:26 pm on November 19, 2019: contributor

@laanwj John Regehr and Pascal Cuoq’s excellent post “Undefined Behavior in 2017” has a nice section on Alignment Violations. Highly recommended reading!

laanwj referenced this in commit 7c6dd71664 on Nov 20, 2019

achow101 closed this on Nov 20, 2019

ajtowns referenced this in commit 9d933ef919 on Dec 10, 2019

HashUnlimited referenced this in commit 54a433a126 on Apr 17, 2020

backpacker69 referenced this in commit a3e85ad096 on Mar 28, 2021

MarcoFalke locked this on Dec 16, 2021

Add copy constructor and copy operator= to CScript to remove ubsan suppression #17510

Conflicts