llmq/signing_shares: guard quorumMember indexing with Assume() to avoid OOB writes; improve ToInvString() safety #6866
Conversation
…id OOB writes; improve ToInvString() safety
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughAdds defensive bounds checks around index-based writes when marking sigShares as announced, batched, or known in src/llmq/signing_shares.cpp. Direct assignments to inv.inv and session.knows.inv are replaced with conditional writes guarded by Assume(...) and explicit index checks to prevent out-of-bounds access. Core logic for building/merging invocation strings and session state is preserved, with state mutations now conditional on valid indices. Also adds an include for util/check.h. No new public interfaces or signature changes. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)src/llmq/signing_shares.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llmq/signing_shares.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/signing_shares.cpp
🧬 Code graph analysis (1)
src/llmq/signing_shares.cpp (1)
src/test/util_tests.cpp (1)
Assume(77-82)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/signing_shares.cpp
[error] 17-17: Clang format differences found: expected changes detected by clang-format-diff.py. Please apply formatting (e.g., run the formatter or re-run the diff) to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (3)
src/llmq/signing_shares.cpp (3)
21-21: LGTM! Include added for Assume() macro.The include is correctly placed and necessary for the bounds-checking guards introduced in this PR.
97-99: LGTM! Bounds check prevents OOB writes.The Assume() guard correctly prevents out-of-bounds access to
inv.inv. In debug builds, this will crash if the condition fails; in release builds, the write is skipped, avoiding undefined behavior.
1094-1110: LGTM! Comprehensive bounds checks prevent OOB access.The multiple Assume() guards correctly protect all vector accesses using
quorumMemberas an index:
- Line 1094: Guards read access to
session.knows.inv- Lines 1105-1107: Guards write access to
inv.inv- Lines 1108-1110: Guards write access to
session.knows.invThis defensive approach prevents undefined behavior from out-of-bounds access while allowing the code to skip invalid operations in release builds.
| auto& session = nodeState.GetOrCreateSessionFromShare(*sigShare); | ||
|
|
||
| if (session.knows.inv[quorumMember]) { | ||
| if (Assume(quorumMember < session.knows.inv.size()) && session.knows.inv[quorumMember]) { |
There was a problem hiding this comment.
Not sure how this can ever happen because invalid sigshare received from another peer shouldn't get this far. We check it in ProcessMessageSigShare:
if (sigShare.getQuorumMember() >= quorum->members.size()) {
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- quorumMember out of bounds\n", __func__);
BanNode(fromId, peerman);
return;
}
We shouldn't create invalid sigshares ourselves either. But anyway... I think we should continue on Assume failure regardless of session.knows.inv. Should probably also drop the invalid session. And maybe also log it so that it wouldn't be a completely silent failure on release nodes. Smth like
| if (Assume(quorumMember < session.knows.inv.size()) && session.knows.inv[quorumMember]) { | |
| if (!Assume(quorumMember < session.knows.inv.size())) { | |
| LogPrintf("%s -- BUG! quorumMember >= session.knows.inv.size(), %d >= %d\n", __func__, quorumMember, | |
| session.knows.inv.size()); | |
| nodeState.RemoveSession(session.signHash.Get()); | |
| continue; | |
| } | |
| if (session.knows.inv[quorumMember]) { |
There was a problem hiding this comment.
Now that I'm looking at it again; what if we wrap these raw datastructs like session.knows.inv in some structure or class and then just expose a method; and internally, it'll either crash or log on out of bounds attempts, and just return false.
Would this be better?
There was a problem hiding this comment.
Internal logic won't be able to clean things up though so we could end up in a half-processed state which can potentially cause issues down the line I guess.
| if (Assume(quorumMember < inv.inv.size())) { | ||
| inv.inv[quorumMember] = true; | ||
| } |
There was a problem hiding this comment.
Same here. Would rather drop all potentially invalid data and continue:
| if (Assume(quorumMember < inv.inv.size())) { | |
| inv.inv[quorumMember] = true; | |
| } | |
| if (!Assume(quorumMember < inv.inv.size())) { | |
| LogPrintf("%s -- BUG! quorumMember >= inv.inv.size(), %d >= %d\n", __func__, quorumMember, inv.inv.size()); | |
| nodeState.RemoveSession(session.signHash.Get()); | |
| sigSharesToAnnounce[nodeId].erase(signHash); | |
| continue; | |
| } | |
| inv.inv[quorumMember] = true; |
| if (Assume(quorumMember < session.knows.inv.size())) { | ||
| session.knows.inv[quorumMember] = true; | ||
| } |
There was a problem hiding this comment.
We just checked it above, no extra Assume is needed
| if (Assume(quorumMember < session.knows.inv.size())) { | |
| session.knows.inv[quorumMember] = true; | |
| } | |
| session.knows.inv[quorumMember] = true; |
2 participants
Issue being fixed or feature implemented
Avoid potentially undefined behavior in llmq logic by using Assume to crash in debug, and skip in release
What was done?
Describe your changes in detail
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.
Breaking Changes
Please describe any breaking changes your code introduces
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.