-
Notifications
You must be signed in to change notification settings - Fork 1
fix: VSS race condition causing memory corruption during wallet restore #318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…etup.Add VssSetupCoordinator actor to ensure thread-safe VSS client setup. The race condition occurred when multiple concurrent tasks called awaitSetup() simultaneously, causing double initialization of the Rust FFI and memory corruption
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical race condition in the VSS backup client that caused memory corruption during concurrent wallet restore operations. The issue occurred when multiple tasks simultaneously called awaitSetup(), leading to double initialization of the Rust FFI layer.
Changes:
- Introduced
VssSetupCoordinatoractor to ensure thread-safe, single-instance setup coordination - Refactored
awaitSetup()to delegate setup orchestration to the coordinator - Updated
reset()method to work asynchronously with the new actor-based coordination
|
Makes perfect sense to use actor here. The reason I didn't in Android is because Kotlin doesn't have it at all and the But the proper way to mirror that code in Swift is likely with actor. |
|
OBS: The crash only happened to me one time, couldn't reproduce it again |
|
Note: Observed a crash today myself (not on this branch ofc, but also couldn't reproduce) and problem report analysis shown: Hopefully this will fix the problem 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utAck
Testing (sanity migration check)
pwltr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, looks good
|
@jvsena42 what are "integration-tests" and why they keep failing? 🤔 |
ovitrif
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tAck
Description
Problem
Memory corruption crash during wallet restore:
malloc: Incorrect checksum for freed object 0x1013da8d8: probably modified after being freed.The crash occurs when multiple concurrent tasks call VssBackupClient.awaitSetup() simultaneously.
Root Cause
The awaitSetup() method has a race condition. When multiple tasks call it concurrently:
Task A checks if let existingSetup = isSetup → nil → proceeds to create setup task
Task B checks if let existingSetup = isSetup → nil (Task A hasn't assigned yet) → also proceeds
Both tasks create separate setupTask instances
Both call setup() → double initialization of Rust FFI → memory corruption
The logs show VSS setup being called 7 times concurrently:
Solution:
Use a Swift actor to ensure thread-safe setup coordination. The actor isolates the setupTask state, ensuring only one setup runs at a time.
Linked Issues/Tasks
N/A
Screenshot / Video
To test: