Merge pull request #17 from SamiAhmed7777/fix/simd-ubsan-shift
ci: fix sanitizer failures
This commit is contained in:
SamiAhmed7777
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<unsigned char>& vch)
|
||||
{
|
||||
if (vch.empty())
|
||||
return std::string();
|
||||
return EncodeBase58(&vch[0], &vch[0] + vch.size());
|
||||
}
|
||||
|
||||
|
||||
+5
-3
@@ -971,17 +971,19 @@ bool EvalScript(vector<vector<unsigned char> >& 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);
|
||||
|
||||
+24
-19
@@ -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)
|
||||
|
||||
/*
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -26,7 +26,7 @@ BOOST_AUTO_TEST_CASE(base58_EncodeBase58)
|
||||
std::vector<unsigned char> 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);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -757,6 +757,8 @@ vector<unsigned char> DecodeBase64(const char* p, bool* pfInvalid)
|
||||
string DecodeBase64(const string& str)
|
||||
{
|
||||
vector<unsigned char> vchRet = DecodeBase64(str.c_str());
|
||||
if (vchRet.empty())
|
||||
return string();
|
||||
return string((const char*)&vchRet[0], vchRet.size());
|
||||
}
|
||||
|
||||
@@ -944,6 +946,8 @@ vector<unsigned char> DecodeBase32(const char* p, bool* pfInvalid)
|
||||
string DecodeBase32(const string& str)
|
||||
{
|
||||
vector<unsigned char> vchRet = DecodeBase32(str.c_str());
|
||||
if (vchRet.empty())
|
||||
return string();
|
||||
return string((const char*)&vchRet[0], vchRet.size());
|
||||
}
|
||||
|
||||
|
||||
+2
-1
@@ -603,8 +603,9 @@ uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL
|
||||
|
||||
inline uint160 Hash160(const std::vector<unsigned char>& 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);
|
||||
|
||||
Reference in New Issue
Block a user