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

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

For reference, one tb is this:

vim $(pwd)/test/sanitizer_suppressions # Remove the prevector suppressions 
./configure  --with-sanitizers=address,undefined CC=clang CXX=clang++
make
export LSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/lsan"
export TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan"
export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1"
$ ./src/test/test_bitcoin -t denialofservice_tests
Running 6 test cases...
prevector.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
0x7ffdaaebe012: note: pointer points here
 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
              ^ 
    [#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
    [#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
    [#2](/bitcoin-bitcoin/2/) 0x560765991f58 in CScript::operator=(CScript&&) ./src/./script/script.h:390
    [#3](/bitcoin-bitcoin/3/) 0x5607673635b5 in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) ./src/script/sign.cpp:240:23
    [#4](/bitcoin-bitcoin/4/) 0x56076736766c in SignSignature(SigningProvider const&, CScript const&, CMutableTransaction&, unsigned int, long const&, int) ./src/script/sign.cpp:375:16
    [#5](/bitcoin-bitcoin/5/) 0x5607673679bb in SignSignature(SigningProvider const&, CTransaction const&, CMutableTransaction&, unsigned int, int) ./src/script/sign.cpp:387:12
    [#6](/bitcoin-bitcoin/6/) 0x560765d5c515 in denialofservice_tests::DoS_mapOrphans::test_method() ./src/test/denialofservice_tests.cpp:403:9
    [#7](/bitcoin-bitcoin/7/) 0x560765d5a4bc in denialofservice_tests::DoS_mapOrphans_invoker() ./src/test/denialofservice_tests.cpp:369:1
    [#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
    [#9](/bitcoin-bitcoin/9/) 0x7fa1a39b7581  (/lib64/libboost_unit_test_framework.so.1.69.0+0x30581)
    [#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)
    [#11](/bitcoin-bitcoin/11/) 0x7fa1a39b6677 in boost::execution_monitor::execute(boost::function<int ()> const&) (/lib64/libboost_unit_test_framework.so.1.69.0+0x2f677)
    [#12](/bitcoin-bitcoin/12/) 0x7fa1a39b674d in boost::execution_monitor::vexecute(boost::function<void ()> const&) (/lib64/libboost_unit_test_framework.so.1.69.0+0x2f74d)
    [#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)
    [#14](/bitcoin-bitcoin/14/) 0x7fa1a39c667f  (/lib64/libboost_unit_test_framework.so.1.69.0+0x3f67f)
    [#15](/bitcoin-bitcoin/15/) 0x7fa1a39c6913  (/lib64/libboost_unit_test_framework.so.1.69.0+0x3f913)
    [#16](/bitcoin-bitcoin/16/) 0x7fa1a39c6913  (/lib64/libboost_unit_test_framework.so.1.69.0+0x3f913)
    [#17](/bitcoin-bitcoin/17/) 0x7fa1a39bdc73 in boost::unit_test::framework::run(unsigned long, bool) (/lib64/libboost_unit_test_framework.so.1.69.0+0x36c73)
    [#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)
    [#19](/bitcoin-bitcoin/19/) 0x5607659a1526 in main /usr/include/boost/test/unit_test.hpp:63:12
    [#20](/bitcoin-bitcoin/20/) 0x7fa1a2f8df42 in __libc_start_main (/lib64/libc.so.6+0x23f42)
    [#21](/bitcoin-bitcoin/21/) 0x560765861f8d in _start (./src/test/test_bitcoin+0x2a59f8d)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior prevector.h:452:19 in 

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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.

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

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

#pragma pack(push, 1)

Smaller code to reproduce:

struct Test {
    uint8_t a : 8;
    CScript script;
};


{
    const std::array<unsigned char, 5> D{1, 2, 3, 4, 5};
    Test t{0x0f, {}};
    t.script = CScript{D.begin(), D.end()};
}

Result:

prevector.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
0x7ffe6446c741: note: pointer points here
 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
              ^ 
    [#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
    [#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
    [#2](/bitcoin-bitcoin/2/) 0x555dd33ef1b8 in CScript::operator=(CScript&&) /home/marco/workspace/btc_bitcoin_core/src/./script/script.h:390
    [#3](/bitcoin-bitcoin/3/) 0x555dd37a0aaa in a() /tmp/a.cpp:79:11

Does it go away if you remove the pragma?

Yes, with this diff it goes away:

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

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)

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