Repository navigation
fix: isolate native binding errors by OS thread - #135
PastaPastaPasta wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
✅ Final review complete — no blockers (commit 56a917a) · triage: normal · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of dbd61e6..ad17f0d confirms consistent thread-local error storage in both native bridges, OS-thread pinning through the owned error copy in all 12 Go entrypoints, and synchronous error copying in Rust; no in-scope defects were identified. The added ownership regressions exercise cross-thread isolation, and the diff whitespace check passed; no builds or tests were run locally, while the supplied exact-head CI snapshot reports successful native, Go, and Rust checks. The pre-existing Rust threshold identifier-length issue is confirmed and retained only as a separate, non-blocking follow-up.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The cross-language error-storage and OS-thread-pinning changes require concurrency and pointer-lifetime reasoning, but do not alter cryptographic algorithms, signatures, or key handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Validate identifier lengths in safe Rust threshold recovery — The safe threshold_recover methods in rust-bindings/bls-signatures/src/elements.rs:118-138 and :257-277 and src/private_key.rs:174-189 accept Vec identifiers and forward their pointers without checking lengths. The native bridge constructs 32-byte views in rust-bindings/bls-dash-sys/c-bindings/threshold.cpp:25-31, and src/threshold.cpp:211-213 reads 32 bytes per identifier once at least two shares are supplied. Undersized identifiers can therefore cause out-of-bounds native reads through a safe Rust API, potentially crashing the process or incorporating unrelated memory into recovery calculations. These paths are unchanged from the base revision and are not worsened by this PR.
- Follow-up: Track a separate issue or maintainer-requested PR to reject identifiers whose lengths are not exactly 32 bytes before crossing FFI, with malformed-length regression tests for all three safe recovery methods.
The Go and Rust C bridges stored the last native error in one process-global string. A failure on another thread could overwrite or race with it before the caller copied the borrowed pointer. Make gErrMsg thread_local in both bridges and document that the returned pointer is only valid on the producing thread until its next failure. Go goroutines can migrate between OS threads across cgo calls, so pin the goroutine with runtime.LockOSThread in each of the 12 entrypoints that read the error through errFromC(). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ad17f0d to
56a917a
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
Independently reviewed the complete diff and relevant callers at head 56a917a; no actionable in-scope defects were found. Both bridges consistently declare and define thread-local error storage, all 12 Go error-reading entrypoints remain OS-thread-pinned through the message copy, and Rust copies errors synchronously; the regression tests exercise cross-thread message isolation. Validation was static only, and git diff --check passed; the supplied exact-head CI snapshot reports successful library and binding checks, with publication jobs skipped, but does not establish TSan coverage or broader native-library thread safety.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change coordinates thread-local native error storage with OS-thread pinning across 12 Go entrypoints and two FFI bridges, requiring concurrency and pointer-lifetime reasoning but not changing cryptographic algorithms, signatures, or key handling. - Phase 1 reviewers: not run (skipped for throughput: 20 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Issue being fixed
The Go and Rust C bridges kept the last native error in one process-global
std::string gErrMsg, andGetLastErrorMsg()returns a borrowed pointer into it. When two threads failed concurrently, one thread's failure could overwrite the other's message, or race with it, before the first caller copied the pointer. Go adds another way for this to fail: a goroutine can move to a different OS thread between the failing cgo call and the cgo call that reads the error.What was done
gErrMsgthread_localin both bridges (go-bindings/andrust-bindings/bls-dash-sys/c-bindings/).runtime.LockOSThreadin each of the 12 Go entrypoints that read the error througherrFromC(), so the read runs on the thread that failed.The same patch is under review in Dash Core as PR 7825, against
src/dashbls/. This PR targetsdevelop, the library revision Dash Core currently imports.Compatibility
C function signatures are unchanged. Raw C callers must retrieve and copy an error on the thread that produced it, before another failure on that thread or before the thread exits. Direct C++ consumers of
gErrMsgmust use the matchingthread_localdeclaration and recompile the bridge objects together.How Has This Been Tested?
TestErrorMessageThreadOwnership. The first goroutine stays locked to its OS thread while a failure on a second thread happens, then checks that its own message is still intact.go vet ./...andgo testpass against a local CMake build of the library (macOS, Apple Silicon).error_message_thread_ownershipinbls-dash-sysmakes the same cross-thread check against the Rust bridge.No TSan result, reliable crash, or daemon impact is claimed.
Breaking Changes
None for the Go or Rust APIs. See Compatibility above for raw C/C++ consumers.
🤖 Generated with Claude Code