Undefined behaviour in bench_dash

[OlegGirko]

Expected behavior

./src/bench/bench_dash -sanity-check -priority-level=high successfully run to completion.

Actual behavior

Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
/usr/include/c++/15/bits/stl_bvector.h:1158: constexpr std::vector<bool, _Alloc>::reference std::vector<bool, _Alloc>::operator[](size_type) [with _Alloc = std::allocator<bool>; reference = std::vector<bool>::reference; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
Aborted                    (core dumped) ./src/bench/bench_dash -sanity-check -priority-level=high

To reproduce

Build bench_dash with GCC 15 and the following C++ flags:

-O2  -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1  -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -mtls-dialect=gnu2 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer

These are standard flags used to build packages for Fedora.

System information

Version 23.0.0 built from sources downloaded from Github.

Building on regular x86_64 PC running Fedora Linux 43.

The bug was introduces in commit 61bba13.

This line calls BuildTestVectors with first argument (count) equal to 1, but second argument (invalidCount) is still 10:

dash/src/bench/bls.cpp

Line 258 in 61bba13

BuildTestVectors(bench.output() ? 1000 : 1, 10, pubKeys, secKeys, sigs, msgHashes, invalid);

As a result, invalid vector is resized to size 1 here:

dash/src/bench/bls.cpp

Line 23 in 61bba13

invalid.resize(count);

and later out-of bound access is performed there:

dash/src/bench/bls.cpp

Line 25 in 61bba13

invalid[i] = true;

[PastaPastaPasta]

I believe this was fixed here: #6977

@coderabbitai please confirm

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR #6977 details
gh pr view 6977 --json title,body,state,url,mergedAt,files

# Get the diff for src/bench/bls.cpp from PR #6977
gh pr diff 6977 -- src/bench/bls.cpp

Length of output: 130

---

🏁 Script executed:

#!/bin/bash
# Check if the file exists and view the problematic line
cat src/bench/bls.cpp | head -n 270 | tail -n 20

# Search for recent commits that modified this file
git log --oneline --all -20 -- src/bench/bls.cpp

# Try to find PR #6977 in commit messages
git log --all --grep="6977" --oneline

# Check if there's a fix for the BuildTestVectors call
grep -n "BuildTestVectors" src/bench/bls.cpp

Length of output: 8828

---

🏁 Script executed:

#!/bin/bash
# Get the diff from the merge commit for PR #6977
git show 2f44cf91fe -- src/bench/bls.cpp

# Also check the commit just before the merge
git log --oneline -5 2f44cf91fe

# Check the specific commit that introduced the bug
git show 61bba13bc2 -- src/bench/bls.cpp | grep -A5 -B5 "BuildTestVectors"

Length of output: 3216

---

🏁 Script executed:

#!/bin/bash
# Check the BuildTestVectors function to understand the issue better
sed -n '13,30p' src/bench/bls.cpp

# Search for commits after PR #6977 that might have fixed this
git log --oneline --all --since="2024-11-01" -- src/bench/bls.cpp

# Check if there are any recent changes to the invalidCount parameter
git log -p --all --since="2024-11-01" -- src/bench/bls.cpp | grep -A2 -B2 "invalidCount"

Length of output: 1062

---

@PastaPastaPasta Unfortunately, PR #6977 did NOT fix this issue. The bug described in #7011 is still present in the codebase.

Here's what I found:

The Problem:
In src/bench/bls.cpp, the BuildTestVectors function at line 23 resizes the invalid vector to count:

invalid.resize(count);

Then at lines 24-26, it tries to access indices from 0 to invalidCount:

for (size_t i = 0; i < invalidCount; i++) {
    invalid[i] = true;
}

What PR #6977 Did:
PR #6977 changed line 258 from:

BuildTestVectors(1000, 10, ...)

to:

BuildTestVectors(bench.output() ? 1000 : 1, 10, ...)

The Issue:
When running with -sanity-check, bench.output() returns false, so:

• count = 1 (first parameter)
• invalidCount = 10 (second parameter - unchanged)

This creates the exact out-of-bounds condition: the invalid vector has size 1, but the code tries to access invalid[0] through invalid[9].

The Fix Needed:
The second parameter should also be adjusted. It should be:

BuildTestVectors(bench.output() ? 1000 : 1, bench.output() ? 10 : 1, ...)

The same issue exists at line 314 in BLS_Verify_BatchedParallel.

---

🧠 Learnings used

Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.

[OlegGirko]

I believe this was fixed here: #6977

@coderabbitai please confirm

My natural intelligence also confirms that the change in #6977 has nothing to do with out-of-bound access I described here.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the current state of the problematic lines in src/bench/bls.cpp
echo "=== Checking line 258 (BLS_Verify_Batched) ==="
sed -n '250,265p' src/bench/bls.cpp

echo -e "\n=== Checking line 314 (BLS_Verify_BatchedParallel) ==="
sed -n '306,320p' src/bench/bls.cpp

echo -e "\n=== Checking BuildTestVectors function ==="
sed -n '13,30p' src/bench/bls.cpp

Length of output: 1842

---

@OlegGirko Thank you for confirming! You're absolutely right—the change in #6977 only made the first parameter (count) conditional but left the second parameter (invalidCount) hardcoded to 10, which causes the out-of-bounds access when running with -sanity-check.

The bug is confirmed to still exist at both locations:

• Line 258 in BLS_Verify_Batched
• Line 314 in BLS_Verify_BatchedParallel

Both need the same fix:

BuildTestVectors(bench.output() ? 1000 : 1, bench.output() ? 10 : 1, ...)

This ensures that when count=1, invalidCount is also reduced to 1, preventing the vector from being accessed beyond its bounds.

Would you like to submit a PR to fix this, or would you like me to help outline the changes needed?

---

🧠 Learnings used

Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.

---

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

---

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

[OlegGirko]

Would you like to submit a PR to fix this, or would you like me to help outline the changes needed?

I don't know this part of Dash code well enough to prepare a PR myself. Let somebody from Dash Core Team fix this issue.

In the meantime, I have a few options for packaging Dash for Fedora right now:

• add a patch reverting offending commit completely,
• don't run bench_dash in tests completely.

[PastaPastaPasta]

Should be resolved by #7013