From b9d06d5f7762d87323f98160a0b174316905165b Mon Sep 17 00:00:00 2001 From: Krystie Date: Tue, 7 Jul 2026 14:42:30 -0700 Subject: [PATCH] ci: fix sanitizer failures Replace undefined signed shifts in SPHlib SIMD FFT arithmetic with bounded multiplications, handle empty vectors in base64/base32/base58/hash/script paths, and skip the DoS_checkSig microbenchmark threshold under sanitizer instrumentation. Sanitizer ctest is now green locally, so make the GitHub sanitizer job blocking again. --- .github/workflows/build-all.yml | 6 ++--- notes/audit-progress.md | 21 ++++++++++++++++ src/base58.h | 2 ++ src/script.cpp | 8 +++--- src/simd.c | 43 ++++++++++++++++++--------------- src/test/DoS_tests.cpp | 7 ++++++ src/test/base58_tests.cpp | 2 +- src/util.cpp | 4 +++ src/util.h | 3 ++- 9 files changed, 68 insertions(+), 28 deletions(-) diff --git a/.github/workflows/build-all.yml b/.github/workflows/build-all.yml index 1de5515..4ca044a 100644 --- a/.github/workflows/build-all.yml +++ b/.github/workflows/build-all.yml @@ -88,11 +88,9 @@ jobs: run: cd build && ctest --output-on-failure || true test-linux-sanitizers: - # ASan + UBSan build of the daemon + unit tests. Allowed to fail until - # findings are triaged — see .github/workflows/lint.yml comment block. - # Once the test suite is clean under sanitizers, drop continue-on-error. + # ASan + UBSan build of the daemon + unit tests. This is a blocking + # signal: sanitizer regressions should fail the PR. runs-on: ubuntu-22.04 - continue-on-error: true env: # ASan: leak detection off by default (BDB and OpenSSL produce noise on shutdown). # Re-enable once we've quieted the legitimate suspects. diff --git a/notes/audit-progress.md b/notes/audit-progress.md index 16b834d..be40869 100644 --- a/notes/audit-progress.md +++ b/notes/audit-progress.md @@ -544,3 +544,24 @@ Full suite: 262/262 cases, 21713/21713 assertions. ctest: 4/4 green. Branch push ### PR #14 CI: ALL REAL JOBS GREEN Final CI run (run 28845879030 on f9a11fc) — every required job passes except the pre-existing simd.c sanitizer failure. PR #14 is merge-ready. + +## 2026-07-07 -- Krystie + +### Action taken: sanitizer lane fixed (branch fix/simd-ubsan-shift) + +Sami asked to fix the sanitizer failure after the release-infrastructure merge made all open PRs green except the known sanitizer issue. + +Root failures fixed: +- `src/simd.c`: SPHlib SIMD FFT macros performed signed left shifts on values that can be negative (`simd.c:265` in CI). Replaced the signed arithmetic shifts with equivalent bounded multiplications by powers of two. This preserves intended arithmetic while removing C undefined behavior. +- `src/util.cpp`: `DecodeBase32(std::string)` and `DecodeBase64(std::string)` took `&vchRet[0]` on empty decoded vectors. Added empty-return guards. +- `src/base58.h` + `src/test/base58_tests.cpp`: `EncodeBase58(vector)` and its test harness took `&vch[0]` for empty vectors. Added an empty-vector guard and routed the test through the vector overload. +- `src/util.h`: `Hash160(vector)` took `&vch[0]` for empty vectors. Switched to the existing pblank/length-0 pattern used by `Hash()` helpers. +- `src/script.cpp`: OP_RIPEMD160 / OP_SHA1 / OP_SHA256 used `&vch[0]` for empty stack data. Added pblank/length-0 handling; OP_HASH160 already routes through `Hash160`. +- `src/test/DoS_tests.cpp`: sanitizer instrumentation made the signature microbenchmark threshold false-fire. Kept all signature correctness checks, but skips the perf threshold under ASan builds. +- `.github/workflows/build-all.yml`: removed `continue-on-error: true` from `test-linux-sanitizers`; sanitizer regressions are blocking again. + +Verification: +- Local sanitizer build with CI flags: `ctest --output-on-failure` => 4/4 passed in build-san-local. +- Normal build/test: `ctest --output-on-failure` => 4/4 passed in build. + +This work intentionally does not touch production datadir `/root/.triangles/`, wallet files, consensus constants, or live daemon state. diff --git a/src/base58.h b/src/base58.h index c0edfbe..3e61722 100644 --- a/src/base58.h +++ b/src/base58.h @@ -67,6 +67,8 @@ inline std::string EncodeBase58(const unsigned char* pbegin, const unsigned char // Encode a byte vector as a base58-encoded string inline std::string EncodeBase58(const std::vector& vch) { + if (vch.empty()) + return std::string(); return EncodeBase58(&vch[0], &vch[0] + vch.size()); } diff --git a/src/script.cpp b/src/script.cpp index 431b13b..47bef66 100644 --- a/src/script.cpp +++ b/src/script.cpp @@ -971,17 +971,19 @@ bool EvalScript(vector >& stack, const CScript& script, co if (stack.size() < 1) return false; valtype& vch = stacktop(-1); + static unsigned char pblank[1]; + const unsigned char* pch = vch.empty() ? pblank : &vch[0]; valtype vchHash((opcode == OP_RIPEMD160 || opcode == OP_SHA1 || opcode == OP_HASH160) ? 20 : 32); if (opcode == OP_RIPEMD160) { TRI_OPENSSL_SUPPRESS_DEPRECATED_BEGIN - RIPEMD160(&vch[0], vch.size(), &vchHash[0]); + RIPEMD160(pch, vch.size(), &vchHash[0]); TRI_OPENSSL_SUPPRESS_DEPRECATED_END } else if (opcode == OP_SHA1) - SHA1(&vch[0], vch.size(), &vchHash[0]); + SHA1(pch, vch.size(), &vchHash[0]); else if (opcode == OP_SHA256) - SHA256(&vch[0], vch.size(), &vchHash[0]); + SHA256(pch, vch.size(), &vchHash[0]); else if (opcode == OP_HASH160) { uint160 hash160 = Hash160(vch); diff --git a/src/simd.c b/src/simd.c index 1e94d92..9229326 100644 --- a/src/simd.c +++ b/src/simd.c @@ -143,6 +143,11 @@ static const s32 alpha_tab[] = { * d5: min= -252 max= 4402 * d6: min=-4335 max= 4335 * d7: min=-4332 max= 322 + * + * Use multiplication, not signed left shift, for powers of two below. FFT + * values can be negative; left-shifting a negative signed integer is + * undefined in C, while these bounded multiplications are defined and + * preserve the intended arithmetic. */ #define FFT8(xb, xs, d) do { \ s32 x0 = x[(xb)]; \ @@ -150,13 +155,13 @@ static const s32 alpha_tab[] = { s32 x2 = x[(xb) + 2 * (xs)]; \ s32 x3 = x[(xb) + 3 * (xs)]; \ s32 a0 = x0 + x2; \ - s32 a1 = x0 + (x2 << 4); \ + s32 a1 = x0 + (x2 * 16); \ s32 a2 = x0 - x2; \ - s32 a3 = x0 - (x2 << 4); \ + s32 a3 = x0 - (x2 * 16); \ s32 b0 = x1 + x3; \ - s32 b1 = REDS1((x1 << 2) + (x3 << 6)); \ - s32 b2 = (x1 << 4) - (x3 << 4); \ - s32 b3 = REDS1((x1 << 6) + (x3 << 2)); \ + s32 b1 = REDS1((x1 * 4) + (x3 * 64)); \ + s32 b2 = (x1 * 16) - (x3 * 16); \ + s32 b3 = REDS1((x1 * 64) + (x3 * 4)); \ d ## 0 = a0 + b0; \ d ## 1 = a1 + b1; \ d ## 2 = a2 + b2; \ @@ -179,21 +184,21 @@ static const s32 alpha_tab[] = { FFT8(xb, (xs) << 1, d1_); \ FFT8((xb) + (xs), (xs) << 1, d2_); \ q[(rb) + 0] = d1_0 + d2_0; \ - q[(rb) + 1] = d1_1 + (d2_1 << 1); \ - q[(rb) + 2] = d1_2 + (d2_2 << 2); \ - q[(rb) + 3] = d1_3 + (d2_3 << 3); \ - q[(rb) + 4] = d1_4 + (d2_4 << 4); \ - q[(rb) + 5] = d1_5 + (d2_5 << 5); \ - q[(rb) + 6] = d1_6 + (d2_6 << 6); \ - q[(rb) + 7] = d1_7 + (d2_7 << 7); \ + q[(rb) + 1] = d1_1 + (d2_1 * 2); \ + q[(rb) + 2] = d1_2 + (d2_2 * 4); \ + q[(rb) + 3] = d1_3 + (d2_3 * 8); \ + q[(rb) + 4] = d1_4 + (d2_4 * 16); \ + q[(rb) + 5] = d1_5 + (d2_5 * 32); \ + q[(rb) + 6] = d1_6 + (d2_6 * 64); \ + q[(rb) + 7] = d1_7 + (d2_7 * 128); \ q[(rb) + 8] = d1_0 - d2_0; \ - q[(rb) + 9] = d1_1 - (d2_1 << 1); \ - q[(rb) + 10] = d1_2 - (d2_2 << 2); \ - q[(rb) + 11] = d1_3 - (d2_3 << 3); \ - q[(rb) + 12] = d1_4 - (d2_4 << 4); \ - q[(rb) + 13] = d1_5 - (d2_5 << 5); \ - q[(rb) + 14] = d1_6 - (d2_6 << 6); \ - q[(rb) + 15] = d1_7 - (d2_7 << 7); \ + q[(rb) + 9] = d1_1 - (d2_1 * 2); \ + q[(rb) + 10] = d1_2 - (d2_2 * 4); \ + q[(rb) + 11] = d1_3 - (d2_3 * 8); \ + q[(rb) + 12] = d1_4 - (d2_4 * 16); \ + q[(rb) + 13] = d1_5 - (d2_5 * 32); \ + q[(rb) + 14] = d1_6 - (d2_6 * 64); \ + q[(rb) + 15] = d1_7 - (d2_7 * 128); \ } while (0) /* diff --git a/src/test/DoS_tests.cpp b/src/test/DoS_tests.cpp index 24c82ac..55ecfbd 100644 --- a/src/test/DoS_tests.cpp +++ b/src/test/DoS_tests.cpp @@ -304,11 +304,18 @@ BOOST_AUTO_TEST_CASE(DoS_checkSig) // OpenSSL instead of libsecp256k1). Adjust if this false-fires on a // materially slower CI runner — the per-trial prints above make the // threshold-defining evidence reproducible. +#if defined(__SANITIZE_ADDRESS__) + // ASan/UBSan builds intentionally instrument every memory access and are + // not meaningful microbenchmark environments. Keep the correctness checks + // above and below, but do not enforce the perf threshold under sanitizers. + if (fDebug) printf("DoS_Checksig sanitizer build: skipping perf threshold (%ld ms)\n", nPerVerifyMs); +#else BOOST_CHECK_MESSAGE(nPerVerifyMs < 600, "Signature verify regression: " << nPerVerifyMs << "ms for 500 verifies (expected <600ms). " << "Cache is a no-op by design (see script.cpp CheckSig); " << "if this fires, an actual verify-path change has slowed it down."); +#endif // Empty a signature, validation should fail: CScript save = tx.vin[0].scriptSig; diff --git a/src/test/base58_tests.cpp b/src/test/base58_tests.cpp index e4c7bd4..9f670e7 100644 --- a/src/test/base58_tests.cpp +++ b/src/test/base58_tests.cpp @@ -26,7 +26,7 @@ BOOST_AUTO_TEST_CASE(base58_EncodeBase58) std::vector sourcedata = ParseHex(test[0].get_str()); std::string base58string = test[1].get_str(); BOOST_CHECK_MESSAGE( - EncodeBase58(&sourcedata[0], &sourcedata[sourcedata.size()]) == base58string, + EncodeBase58(sourcedata) == base58string, strTest); } } diff --git a/src/util.cpp b/src/util.cpp index d20b5ca..0cbed60 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -757,6 +757,8 @@ vector DecodeBase64(const char* p, bool* pfInvalid) string DecodeBase64(const string& str) { vector vchRet = DecodeBase64(str.c_str()); + if (vchRet.empty()) + return string(); return string((const char*)&vchRet[0], vchRet.size()); } @@ -944,6 +946,8 @@ vector DecodeBase32(const char* p, bool* pfInvalid) string DecodeBase32(const string& str) { vector vchRet = DecodeBase32(str.c_str()); + if (vchRet.empty()) + return string(); return string((const char*)&vchRet[0], vchRet.size()); } diff --git a/src/util.h b/src/util.h index 5e0c785..b44ddbf 100644 --- a/src/util.h +++ b/src/util.h @@ -603,8 +603,9 @@ uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL inline uint160 Hash160(const std::vector& vch) { + static unsigned char pblank[1]; uint256 hash1; - SHA256(&vch[0], vch.size(), (unsigned char*)&hash1); + SHA256(vch.empty() ? pblank : &vch[0], vch.size(), (unsigned char*)&hash1); uint160 hash2; TRI_OPENSSL_SUPPRESS_DEPRECATED_BEGIN RIPEMD160((unsigned char*)&hash1, sizeof(hash1), (unsigned char*)&hash2);