From c78d8ff4cb83506413bb73833fc5c04885d0ece8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Tue, 10 Sep 2024 20:10:35 +0200 Subject: [PATCH 1/3] prevector: avoid GCC bogus warnings in insert method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When compiling with GCC 12.2, both `-Warray-bounds` and `-Wstringop-overflow` warnings were triggered in the `prevector::insert` method during CScript prevector operations. GCC incorrectly assumed that operator new could modify the state of class members, leading to false positives during the memmove operation. Following the approach in https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=cca06f0d6d76b0, we introduced local copies for the destination pointer in memmove operations. This prevents GCC from misinterpreting memory manipulation as unsafe. A minimal reproducer triggering this issue in GCC 12.2 and passing in GCC 12.3 can be found at https://godbolt.org/z/8r9TKKoxv. ------- Full error (with changes from the next commit as well): ``` In file included from /ci_container_base/src/script/script.h:11, from /ci_container_base/src/primitives/transaction.h:11, from /ci_container_base/src/primitives/block.h:9, from /ci_container_base/src/kernel/chainparams.h:11, from /ci_container_base/src/kernel/chainparams.cpp:6: In member function ‘void prevector::fill(T*, InputIterator, InputIterator) [with InputIterator = const unsigned char*; unsigned int N = 28; T = unsigned char; Size = unsigned int; Diff = int]’, inlined from ‘void prevector::insert(iterator, InputIterator, InputIterator) [with InputIterator = const unsigned char*; unsigned int N = 28; T = unsigned char; Size = unsigned int; Diff = int]’ at /ci_container_base/src/prevector.h:395:13, inlined from ‘void CScript::AppendData(const prevector<28, unsigned char>::value_type*, size_t)’ at /ci_container_base/src/script/script.h:439:15, inlined from ‘CScript& CScript::operator<<(std::span)’ at /ci_container_base/src/script/script.h:496:17, inlined from ‘CBlock CreateGenesisBlock(uint32_t, uint32_t, uint32_t, int32_t, const CAmount&)’ at /ci_container_base/src/kernel/chainparams.cpp:76:54: /ci_container_base/src/prevector.h:216:13: error: writing 65 bytes into a region of size 32 [-Werror=stringop-overflow=] 216 | new(static_cast(dst)) T(*first); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /ci_container_base/src/kernel/chainparams.cpp: In function ‘CBlock CreateGenesisBlock(uint32_t, uint32_t, uint32_t, int32_t, const CAmount&)’: /ci_container_base/src/kernel/chainparams.cpp:76:49: note: destination object ‘’ of size 32 76 | const CScript genesisOutputScript = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; | ^ In file included from /usr/lib/gcc/x86_64-w64-mingw32/12-posix/include/c++/cstring:42, from /ci_container_base/src/crypto/common.h:11, from /ci_container_base/src/uint256.h:9, from /ci_container_base/src/consensus/params.h:9, from /ci_container_base/src/kernel/chainparams.h:9: In function ‘void* memmove(void*, const void*, size_t)’, inlined from ‘void prevector::insert(iterator, InputIterator, InputIterator) [with InputIterator = const unsigned char*; unsigned int N = 28; T = unsigned char; Size = unsigned int; Diff = int]’ at /ci_container_base/src/prevector.h:393:16, inlined from ‘void CScript::AppendData(const prevector<28, unsigned char>::value_type*, size_t)’ at /ci_container_base/src/script/script.h:439:15, inlined from ‘CScript& CScript::operator<<(std::span)’ at /ci_container_base/src/script/script.h:496:17, inlined from ‘CBlock CreateGenesisBlock(uint32_t, uint32_t, uint32_t, int32_t, const CAmount&)’ at /ci_container_base/src/kernel/chainparams.cpp:76:54: /usr/share/mingw-w64/include/string.h:214:33: warning: ‘void* __builtin_memmove(void*, const void*, long long unsigned int)’ offset [65, 35] is out of the bounds [0, 32] of object ‘’ with type ‘CScript’ [-Warray-bounds] 214 | return __builtin___memmove_chk(__dst, __src, __n, __mingw_bos(__dst, 0)); | ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /ci_container_base/src/kernel/chainparams.cpp: In function ‘CBlock CreateGenesisBlock(uint32_t, uint32_t, uint32_t, int32_t, const CAmount&)’: /ci_container_base/src/kernel/chainparams.cpp:76:49: note: ‘’ declared here 76 | const CScript genesisOutputScript = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; | ^ ``` Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> Co-authored-by: Ryan Ofsky --- src/prevector.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/prevector.h b/src/prevector.h index 0c47137910b8d..d14e5f64e9dac 100644 --- a/src/prevector.h +++ b/src/prevector.h @@ -363,7 +363,8 @@ class prevector { change_capacity(new_size + (new_size >> 1)); } T* ptr = item_ptr(p); - memmove(ptr + 1, ptr, (size() - p) * sizeof(T)); + T* dst = ptr + 1; + memmove(dst, ptr, (size() - p) * sizeof(T)); _size++; new(static_cast(ptr)) T(value); return iterator(ptr); @@ -376,7 +377,8 @@ class prevector { change_capacity(new_size + (new_size >> 1)); } T* ptr = item_ptr(p); - memmove(ptr + count, ptr, (size() - p) * sizeof(T)); + T* dst = ptr + count; + memmove(dst, ptr, (size() - p) * sizeof(T)); _size += count; fill(item_ptr(p), count, value); } @@ -390,7 +392,8 @@ class prevector { change_capacity(new_size + (new_size >> 1)); } T* ptr = item_ptr(p); - memmove(ptr + count, ptr, (size() - p) * sizeof(T)); + T* dst = ptr + count; + memmove(dst, ptr, (size() - p) * sizeof(T)); _size += count; fill(ptr, first, last); } From cac846c2fbf6fc69bfc288fd387aa3f68d84d584 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 9 Sep 2024 21:08:08 +0200 Subject: [PATCH 2/3] Allow CScript's operator<< to accept spans, not just vectors Extracted existing serialization to append size & data in separate private methods to clarify that it does more than just a simple data insertion. * the C style casts were changed to static_cast * `unsigned char` and `uint8_t` were changed to value_type for forward compatibility * `data + sizeof(data)` was changed to `std::cend` * data insertion (in AppendData) relies on pointer arithmetic now to enable both `std::span` and `std::span` operators * use uint32_t for data size instead of size_t * used span instead of raw pointers in the new methods Co-authored-by: Ryan Ofsky Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com> --- src/script/script.h | 61 ++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/src/script/script.h b/src/script/script.h index e3119cbe05f96..f4579849803be 100644 --- a/src/script/script.h +++ b/src/script/script.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -412,6 +413,32 @@ bool GetScriptOp(CScriptBase::const_iterator& pc, CScriptBase::const_iterator en /** Serialized script, used inside transaction inputs and outputs */ class CScript : public CScriptBase { +private: + inline void AppendDataSize(const uint32_t size) + { + if (size < OP_PUSHDATA1) { + insert(end(), static_cast(size)); + } else if (size <= 0xff) { + insert(end(), OP_PUSHDATA1); + insert(end(), static_cast(size)); + } else if (size <= 0xffff) { + insert(end(), OP_PUSHDATA2); + value_type data[2]; + WriteLE16(data, size); + insert(end(), std::cbegin(data), std::cend(data)); + } else { + insert(end(), OP_PUSHDATA4); + value_type data[4]; + WriteLE32(data, size); + insert(end(), std::cbegin(data), std::cend(data)); + } + } + + void AppendData(std::span data) + { + insert(end(), data.begin(), data.end()); + } + protected: CScript& push_int64(int64_t n) { @@ -463,35 +490,19 @@ class CScript : public CScriptBase return *this; } - CScript& operator<<(const std::vector& b) LIFETIMEBOUND + CScript& operator<<(std::span b) LIFETIMEBOUND { - if (b.size() < OP_PUSHDATA1) - { - insert(end(), (unsigned char)b.size()); - } - else if (b.size() <= 0xff) - { - insert(end(), OP_PUSHDATA1); - insert(end(), (unsigned char)b.size()); - } - else if (b.size() <= 0xffff) - { - insert(end(), OP_PUSHDATA2); - uint8_t _data[2]; - WriteLE16(_data, b.size()); - insert(end(), _data, _data + sizeof(_data)); - } - else - { - insert(end(), OP_PUSHDATA4); - uint8_t _data[4]; - WriteLE32(_data, b.size()); - insert(end(), _data, _data + sizeof(_data)); - } - insert(end(), b.begin(), b.end()); + AppendDataSize(b.size()); + AppendData({reinterpret_cast(b.data()), b.size()}); return *this; } + // For compatibility reasons. In new code, prefer using std::byte instead of uint8_t. + CScript& operator<<(std::span b) LIFETIMEBOUND + { + return *this << std::as_bytes(b); + } + bool GetOp(const_iterator& pc, opcodetype& opcodeRet, std::vector& vchRet) const { return GetScriptOp(pc, end(), opcodeRet, &vchRet); From 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 9 Sep 2024 21:13:20 +0200 Subject: [PATCH 3/3] Replace CScript _hex_v_u8 appends with _hex This will skip vector conversion before serializing to the prevector in CScript. --- src/kernel/chainparams.cpp | 4 ++-- src/test/miner_tests.cpp | 2 +- src/test/script_tests.cpp | 9 ++++++++- src/test/transaction_tests.cpp | 18 +++++++++--------- src/wallet/test/ismine_tests.cpp | 4 ++-- 5 files changed, 22 insertions(+), 15 deletions(-) diff --git a/src/kernel/chainparams.cpp b/src/kernel/chainparams.cpp index 18c2026a884c1..0f128d4c56261 100644 --- a/src/kernel/chainparams.cpp +++ b/src/kernel/chainparams.cpp @@ -73,7 +73,7 @@ static CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesi static CBlock CreateGenesisBlock(uint32_t nTime, uint32_t nNonce, uint32_t nBits, int32_t nVersion, const CAmount& genesisReward) { const char* pszTimestamp = "The Times 03/Jan/2009 Chancellor on brink of second bailout for banks"; - const CScript genesisOutputScript = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex_v_u8 << OP_CHECKSIG; + const CScript genesisOutputScript = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; return CreateGenesisBlock(pszTimestamp, genesisOutputScript, nTime, nNonce, nBits, nVersion, genesisReward); } @@ -353,7 +353,7 @@ class CTestNet4Params : public CChainParams { m_assumed_chain_state_size = 0; const char* testnet4_genesis_msg = "03/May/2024 000000000000000000001ebd58c244970b3aa9d783bb001011fbe8ea8e98e00e"; - const CScript testnet4_genesis_script = CScript() << "000000000000000000000000000000000000000000000000000000000000000000"_hex_v_u8 << OP_CHECKSIG; + const CScript testnet4_genesis_script = CScript() << "000000000000000000000000000000000000000000000000000000000000000000"_hex << OP_CHECKSIG; genesis = CreateGenesisBlock(testnet4_genesis_msg, testnet4_genesis_script, 1714777860, diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 9f35690460602..6cf2757b2d56b 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -608,7 +608,7 @@ void MinerTestingSetup::TestPrioritisedMining(const CScript& scriptPubKey, const BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { // Note that by default, these tests run with size accounting enabled. - CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex_v_u8 << OP_CHECKSIG; + CScript scriptPubKey = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; std::unique_ptr pblocktemplate; CTxMemPool& tx_mempool{*m_node.mempool}; diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp index 0e2a1631ce4d0..8c3806d22f3eb 100644 --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -1368,6 +1368,13 @@ static CScript ScriptFromHex(const std::string& str) return ToScript(*Assert(TryParseHex(str))); } +BOOST_AUTO_TEST_CASE(script_byte_array_u8_vector_equivalence) +{ + const CScript scriptPubKey1 = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex_v_u8 << OP_CHECKSIG; + const CScript scriptPubKey2 = CScript() << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"_hex << OP_CHECKSIG; + BOOST_CHECK(scriptPubKey1 == scriptPubKey2); +} + BOOST_AUTO_TEST_CASE(script_FindAndDelete) { // Exercise the FindAndDelete functionality @@ -1421,7 +1428,7 @@ BOOST_AUTO_TEST_CASE(script_FindAndDelete) // prefix, leaving 02ff03 which is push-two-bytes: s = ToScript("0302ff030302ff03"_hex); d = ToScript("03"_hex); - expect = CScript() << "ff03"_hex_v_u8 << "ff03"_hex_v_u8; + expect = CScript() << "ff03"_hex << "ff03"_hex; BOOST_CHECK_EQUAL(FindAndDelete(s, d), 2); BOOST_CHECK(s == expect); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 462abd5222e80..3430a5bbfa0b2 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -852,24 +852,24 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) CheckIsNotStandard(t, "scriptpubkey"); // MAX_OP_RETURN_RELAY-byte TxoutType::NULL_DATA (standard) - t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY, t.vout[0].scriptPubKey.size()); CheckIsStandard(t); // MAX_OP_RETURN_RELAY+1-byte TxoutType::NULL_DATA (non-standard) - t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3800"_hex_v_u8; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3800"_hex; BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY + 1, t.vout[0].scriptPubKey.size()); CheckIsNotStandard(t, "scriptpubkey"); // Data payload can be encoded in any way... - t.vout[0].scriptPubKey = CScript() << OP_RETURN << ""_hex_v_u8; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << ""_hex; CheckIsStandard(t); - t.vout[0].scriptPubKey = CScript() << OP_RETURN << "00"_hex_v_u8 << "01"_hex_v_u8; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << "00"_hex << "01"_hex; CheckIsStandard(t); // OP_RESERVED *is* considered to be a PUSHDATA type opcode by IsPushOnly()! - t.vout[0].scriptPubKey = CScript() << OP_RETURN << OP_RESERVED << -1 << 0 << "01"_hex_v_u8 << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9 << 10 << 11 << 12 << 13 << 14 << 15 << 16; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << OP_RESERVED << -1 << 0 << "01"_hex << 2 << 3 << 4 << 5 << 6 << 7 << 8 << 9 << 10 << 11 << 12 << 13 << 14 << 15 << 16; CheckIsStandard(t); - t.vout[0].scriptPubKey = CScript() << OP_RETURN << 0 << "01"_hex_v_u8 << 2 << "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"_hex_v_u8; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << 0 << "01"_hex << 2 << "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"_hex; CheckIsStandard(t); // ...so long as it only contains PUSHDATA's @@ -883,13 +883,13 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) // Only one TxoutType::NULL_DATA permitted in all cases t.vout.resize(2); - t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; t.vout[0].nValue = 0; - t.vout[1].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8; + t.vout[1].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; t.vout[1].nValue = 0; CheckIsNotStandard(t, "multi-op-return"); - t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex_v_u8; + t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; t.vout[1].scriptPubKey = CScript() << OP_RETURN; CheckIsNotStandard(t, "multi-op-return"); diff --git a/src/wallet/test/ismine_tests.cpp b/src/wallet/test/ismine_tests.cpp index 8deab74facfc9..f6688ed30a030 100644 --- a/src/wallet/test/ismine_tests.cpp +++ b/src/wallet/test/ismine_tests.cpp @@ -684,7 +684,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); scriptPubKey.clear(); - scriptPubKey << OP_0 << "aabb"_hex_v_u8; + scriptPubKey << OP_0 << "aabb"_hex; result = keystore.GetLegacyScriptPubKeyMan()->IsMine(scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO); @@ -699,7 +699,7 @@ BOOST_AUTO_TEST_CASE(ismine_standard) BOOST_CHECK(keystore.GetLegacyScriptPubKeyMan()->AddKey(keys[0])); scriptPubKey.clear(); - scriptPubKey << OP_16 << "aabb"_hex_v_u8; + scriptPubKey << OP_16 << "aabb"_hex; result = keystore.GetLegacyScriptPubKeyMan()->IsMine(scriptPubKey); BOOST_CHECK_EQUAL(result, ISMINE_NO);