Repository: bitcoin/bitcoin (language: cpp)

Issue:
ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc
master @ 65c05db660b2ca1d0076b0d8573a6760b3228068.
Running `time FILE_ENV="./ci/test/00_setup_env_native_fuzz.sh" ./ci/test_run_all.sh` on Rawhide aarch64.

```bash
Run coincontrol with args ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')]crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0x52d000000406: note: pointer points here
 b9 c5 22 00 01 01  1a 6c 65 76 65 6c 64 62  2e 42 79 74 65 77 69 73  65 43 6f 6d 70 61 72 61  74 6f
             ^ 
    #0 0xaaaab69963e4 in crc32c::ExtendArm64(unsigned int, unsigned char const*, unsigned long) src/crc32c/src/crc32c_arm64.cc:101:26
    #1 0xaaaab68cd024 in leveldb::crc32c::Value(char const*, unsigned long) src/./leveldb/util/crc32c.h:20:60
    #2 0xaaaab68cd024 in leveldb::log::Reader::ReadPhysicalRecord(leveldb::Slice*) src/leveldb/db/log_reader.cc:246:29
    #3 0xaaaab68cb594 in leveldb::log::Reader::ReadRecord(leveldb::Slice*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*) src/leveldb/db/log_reader.cc:72:38
    #4 0xaaaab68fb1b0 in leveldb::VersionSet::Recover(bool*) src/leveldb/db/version_set.cc:910:19
    #5 0xaaaab6878208 in leveldb::DBImpl::Recover(leveldb::VersionEdit*, bool*) src/leveldb/db/db_impl.cc:320:18
    #6 0xaaaab68a9ec8 in leveldb::DB::Open(leveldb::Options const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, leveldb::DB**) src/leveldb/db/db_impl.cc:1484:20
    #7 0xaaaab4ca3858 in CDBWrapper::CDBWrapper(DBParams const&) src/dbwrapper.cpp:247:30
    #8 0xaaaab4b53fb0 in kernel::BlockTreeDB::BlockTreeDB(DBParams const&) src/./node/blockstorage.h:53:23
    #9 0xaaaab4b53fb0 in std::__detail::_MakeUniq<kernel::BlockTreeDB>::__single_object std::make_unique<kernel::BlockTreeDB, DBParams>(DBParams&&) /usr/bin/../lib/gcc/aarch64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:1070:34
    #10 0xaaaab4b53fb0 in ChainTestingSetup::ChainTestingSetup(ChainType, std::vector<char const*, std::allocator<char const*>> const&) src/test/util/setup_common.cpp:198:51
    #11 0xaaaab4b5932c in TestingSetup::TestingSetup(ChainType, std::vector<char const*, std::allocator<char const*>> const&, bool, bool) src/test/util/setup_common.cpp:250:7
    #12 0xaaaab44ae3b8 in std::__detail::_MakeUniq<TestingSetup const>::__single_object std::make_unique<TestingSetup const, ChainType const&, std::vector<char const*, std::allocator<char const*>> const&>(ChainType const&, std::vector<char const*, std::allocator<char const*>> const&) /usr/bin/../lib/gcc/aarch64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:1070:34
    #13 0xaaaab44ae3b8 in std::unique_ptr<TestingSetup const, std::default_delete<TestingSetup const>> MakeNoLogFileContext<TestingSetup const>(ChainType, std::vector<char const*, std::allocator<char const*>> const&) src/./test/util/setup_common.h:225:12
    #14 0xaaaab44a44e8 in wallet::(anonymous namespace)::initialize_coincontrol() src/wallet/test/fuzz/coincontrol.cpp:19:39
    #15 0xaaaab4ba0f94 in std::function<void ()>::operator()() const /usr/bin/../lib/gcc/aarch64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    #16 0xaaaab4ba0f94 in initialize() src/test/fuzz/fuzz.cpp:130:5
    #17 0xaaaab4ba2fc4 in LLVMFuzzerInitialize src/test/fuzz/fuzz.cpp:186:5
    #18 0xaaaab43962d4 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x19862d4) (BuildId: 289091bbf42eee6985a3c25d293425265dca4365)
    #19 0xaaaab43c0cc8 in main (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x19b0cc8) (BuildId: 289091bbf42eee6985a3c25d293425265dca4365)
    #20 0xffffaab57580  (/lib/aarch64-linux-gnu/libc.so.6+0x27580) (BuildId: ee37e53ec1958e9bebf5b3d5f81a3039cf0a1851)
    #21 0xffffaab57654 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x27654) (BuildId: ee37e53ec1958e9bebf5b3d5f81a3039cf0a1851)
    #22 0xaaaab438e26c in _start (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x197e26c) (BuildId: 289091bbf42eee6985a3c25d293425265dca4365)

SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use crc32c/src/crc32c_arm64.cc:101:26 in 

crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0x52d000000406 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0x52d000000406: note: pointer points here
 b9 c5 22 00 01 01  1a 6c 65 76 65 6c 64 62  2e 42 79 74 65 77 69 73  65 43 6f 6d 70 61 72 61  74 6f
             ^ 
    #0 0xaaaab69963e4 in crc32c::ExtendArm64(unsigned int, unsigned char const*, unsigned long) src/crc32c/src/crc32c_arm64.cc:101:26
    #1 0xaaaab68cd024 in leveldb::crc32c::Value(char const*, unsigned long) src/./leveldb/util/crc32c.h:20:60
    #2 0xaaaab68cd024 in leveldb::log::Reader::ReadPhysicalRecord(leveldb::Slice*) src/leveldb/db/log_reader.cc:246:29
    #3 0xaaaab68cb594 in leveldb::log::Reader::ReadRecord(leveldb::Slice*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*) src/leveldb/db/log_reader.cc:72:38
    #4 0xaaaab68fb1b0 in leveldb::VersionSet::Recover(bool*) src/leveldb/db/version_set.cc:910:19
    #5 0xaaaab6878208 in leveldb::DBImpl::Recover(leveldb::VersionEdit*, bool*) src/leveldb/db/db_impl.cc:320:18
    #6 0xaaaab68a9ec8 in leveldb::DB::Open(leveldb::Options const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, leveldb::DB**) src/leveldb/db/db_impl.cc:1484:20
    #7 0xaaaab4ca3858 in CDBWrapper::CDBWrapper(DBParams const&) src/dbwrapper.cpp:247:30
    #8 0xaaaab4b53fb0 in kernel::BlockTreeDB::BlockTreeDB(DBParams const&) src/./node/blockstorage.h:53:23
    #9 0xaaaab4b53fb0 in std::__detail::_MakeUniq<kernel::BlockTreeDB>::__single_object std::make_unique<kernel::BlockTreeDB, DBParams>(DBParams&&) /usr/bin/../lib/gcc/aarch64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:1070:34
    #10 0xaaaab4b53fb0 in ChainTestingSetup::ChainTestingSetup(ChainType, std::vector<char const*, std::allocator<char const*>> const&) src/test/util/setup_common.cpp:198:51
    #11 0xaaaab4b5932c in TestingSetup::TestingSetup(ChainType, std::vector<char const*, std::allocator<char const*>> const&, bool, bool) src/test/util/setup_common.cpp:250:7
    #12 0xaaaab44ae3b8 in std::__detail::_MakeUniq<TestingSetup const>::__single_object std::make_unique<TestingSetup const, ChainType const&, std::vector<char const*, std::allocator<char const*>> const&>(ChainType const&, std::vector<char const*, std::allocator<char const*>> const&) /usr/bin/../lib/gcc/aarch64-linux-gnu/13/../../../../include/c++/13/bits/unique_ptr.h:1070:34
    #13 0xaaaab44ae3b8 in std::unique_ptr<TestingSetup const, std::default_delete<TestingSetup const>> MakeNoLogFileContext<TestingSetup const>(ChainType, std::vector<char const*, std::allocator<char const*>> const&) src/./test/util/setup_common.h:225:12
    #14 0xaaaab44a44e8 in wallet::(anonymous namespace)::initialize_coincontrol() src/wallet/test/fuzz/coincontrol.cpp:19:39
    #15 0xaaaab4ba0f94 in std::function<void ()>::operator()() const /usr/bin/../lib/gcc/aarch64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
    #16 0xaaaab4ba0f94 in initialize() src/test/fuzz/fuzz.cpp:130:5
    #17 0xaaaab4ba2fc4 in LLVMFuzzerInitialize src/test/fuzz/fuzz.cpp:186:5
    #18 0xaaaab43962d4 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x19862d4) (BuildId: 289091bbf42eee6985a3c25d293425265dca4365)
    #19 0xaaaab43c0cc8 in main (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x19b0cc8) (BuildId: 289091bbf42eee6985a3c25d293425265dca4365)
    #20 0xffffaab57580  (/lib/aarch64-linux-gnu/libc.so.6+0x27580) (BuildId: ee37e53ec1958e9bebf5b3d5f81a3039cf0a1851)
    #21 0xffffaab57654 in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x27654) (BuildId: ee37e53ec1958e9bebf5b3d5f81a3039cf0a1851)
    #22 0xaaaab438e26c in _start (/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz+0x197e26c) (BuildId: 289091bbf42eee6985a3c25d293425265dca4365)

SUMMARY: UndefinedBehaviorSanitizer: misaligned-pointer-use crc32c/src/crc32c_arm64.cc:101:26 in 

Target ['/ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/ci_container_base/ci/scratch/qa-assets/fuzz_seed_corpus/coincontrol')] failed with exit code 1
```

CPU Info:
```bash
cat /proc/cpuinfo
processor	: 0
BogoMIPS	: 50.00
Features	: fp asimd evtstrm aes pmull sha1 sha2 crc32 atomics fphp asimdhp cpuid asimdrdm lrcpc dcpop asimddp ssbs
CPU implementer	: 0x41
CPU architecture: 8
CPU variant	: 0x3
CPU part	: 0xd0c
CPU revision	: 1
```


Discussion / hints:
Same with `./ci/test/00_setup_env_native_asan.sh`.

I somehow assumed that this was fixed by:

* https://github.com/bitcoin/bitcoin/pull/27360
* https://github.com/bitcoin/bitcoin/pull/27298
* https://github.com/bitcoin/bitcoin/pull/27363
I re-tried commit 7b45d171f549595a831489827c28e8493f36c00c and at least Asan is passing. Fuzz and Tsan are failing for different reasons.
Should we use the [`-mstrict-align`](https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html#index-mstrict-align) option?
```
# git bisect bad
228d6a2969e4fcee573c9df7aad31550eab9c8d4 is the first bad commit
commit 228d6a2969e4fcee573c9df7aad31550eab9c8d4
Author: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Date:   Mon Nov 20 13:37:44 2023 +0000

    build: Fix regression in "ARMv8 CRC32 intrinsics" test
    
    The `vmull_p64` is a part of the Crypto extensions from the ACLE. They
    are optional extensions, so they get enabled with a `+crypto` for
    architecture flags.

 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

I can reproduce the issue on Debian 11, aarch64.
A possible solution suggested in https://github.com/bitcoin-core/crc32c-subtree/pull/6.
FWIW @hebasto's solution looks correct (and necessary) to me.

An alternative would be to use c++20's `std::assume_aligned` if we could guarantee the input's alignment, but looking at the usage that's definitely not the case.
@hebasto Can you update https://github.com/bitcoin-core/crc32c-subtree/pull/6 so that the commit includes a description of the problem, an explanation of the fix, and any further information. We should probably add a comment inline explaining why a memcpy is being added to that code.
It'd be worth taking upstream as well.
> It'd be worth taking upstream as well.

We could open a PR, but I'm not sure if we'll get any traction there. The last change upstream https://github.com/google/crc32c was ~ 3 years ago, and the readme recommends the use of https://github.com/google/highwayhash, which is actively maintained.
> Can you update [bitcoin-core/crc32c-subtree#6](https://github.com/bitcoin-core/crc32c-subtree/pull/6) so that the commit includes a description of the problem, an explanation of the fix, and any further information. We should probably add a comment inline explaining why a memcpy is being added to that code.

Thanks! Done.

Fix patch:
diff --git a/src/crc32c/src/crc32c_arm64.cc b/src/crc32c/src/crc32c_arm64.cc
index 1da04ed34a3c9..711616cd2f53c 100644
--- a/src/crc32c/src/crc32c_arm64.cc
+++ b/src/crc32c/src/crc32c_arm64.cc
@@ -12,6 +12,7 @@
 
 #include <cstddef>
 #include <cstdint>
+#include <cstring>
 
 #include "./crc32c_internal.h"
 #ifdef CRC32C_HAVE_CONFIG_H
@@ -29,14 +30,14 @@
 // compute 8bytes for each segment parallelly
 #define CRC32C32BYTES(P, IND)                                             \
   do {                                                                    \
-    crc1 = __crc32cd(                                                     \
-        crc1, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 1 + (IND))); \
-    crc2 = __crc32cd(                                                     \
-        crc2, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 2 + (IND))); \
-    crc3 = __crc32cd(                                                     \
-        crc3, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 3 + (IND))); \
-    crc0 = __crc32cd(                                                     \
-        crc0, *((const uint64_t *)(P) + (SEGMENTBYTES / 8) * 0 + (IND))); \
+    std::memcpy(&d64, (P) + SEGMENTBYTES * 1 + (IND) * 8, sizeof(d64));   \
+    crc1 = __crc32cd(crc1, d64);                                          \
+    std::memcpy(&d64, (P) + SEGMENTBYTES * 2 + (IND) * 8, sizeof(d64));   \
+    crc2 = __crc32cd(crc2, d64);                                          \
+    std::memcpy(&d64, (P) + SEGMENTBYTES * 3 + (IND) * 8, sizeof(d64));   \
+    crc3 = __crc32cd(crc3, d64);                                          \
+    std::memcpy(&d64, (P) + SEGMENTBYTES * 0 + (IND) * 8, sizeof(d64));   \
+    crc0 = __crc32cd(crc0, d64);                                          \
   } while (0);
 
 // compute 8*8 bytes for each segment parallelly
@@ -68,6 +69,9 @@ uint32_t ExtendArm64(uint32_t crc, const uint8_t *data, size_t size) {
   int64_t length = size;
   uint32_t crc0, crc1, crc2, crc3;
   uint64_t t0, t1, t2;
+  uint16_t d16;
+  uint32_t d32;
+  uint64_t d64;
 
   // k0=CRC(x^(3*SEGMENTBYTES*8)), k1=CRC(x^(2*SEGMENTBYTES*8)),
   // k2=CRC(x^(SEGMENTBYTES*8))
@@ -88,7 +92,8 @@ uint32_t ExtendArm64(uint32_t crc, const uint8_t *data, size_t size) {
     t2 = (uint64_t)vmull_p64(crc2, k2);
     t1 = (uint64_t)vmull_p64(crc1, k1);
     t0 = (uint64_t)vmull_p64(crc0, k0);
-    crc = __crc32cd(crc3, *(uint64_t *)data);
+    std::memcpy(&d64, data, sizeof(d64));
+    crc = __crc32cd(crc3, d64);
     data += sizeof(uint64_t);
     crc ^= __crc32cd(0, t2);
     crc ^= __crc32cd(0, t1);
@@ -98,18 +103,21 @@ uint32_t ExtendArm64(uint32_t crc, const uint8_t *data, size_t size) {
   }
 
   while (length >= 8) {
-    crc = __crc32cd(crc, *(uint64_t *)data);
+    std::memcpy(&d64, data, sizeof(d64));
+    crc = __crc32cd(crc, d64);
     data += 8;
     length -= 8;
   }
 
   if (length & 4) {
-    crc = __crc32cw(crc, *(uint32_t *)data);
+    std::memcpy(&d32, data, sizeof(d32));
+    crc = __crc32cw(crc, d32);
     data += 4;
   }
 
   if (length & 2) {
-    crc = __crc32ch(crc, *(uint16_t *)data);
+    std::memcpy(&d16, data, sizeof(d16));
+    crc = __crc32ch(crc, d16);
     data += 2;
   }
 


Test patch: