-
Notifications
You must be signed in to change notification settings - Fork 55
fix(swift-sdk): handle ErrorShutdownIncomplete on PR #3954 #3957
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
Changes from all commits
99ac609
c8576ab
e16bfaa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -155,10 +155,47 @@ public class PlatformWalletManager: ObservableObject { | |||||
| if handle != NULL_HANDLE { | ||||||
| platform_wallet_manager_platform_address_sync_stop(handle).discard() | ||||||
| platform_wallet_manager_shielded_sync_stop(handle).discard() | ||||||
| platform_wallet_manager_destroy(handle).discard() | ||||||
| // `destroy` joins every coordinator OS thread with a 30 s | ||||||
| // deadline. On `errorShutdownIncomplete` one or more | ||||||
| // coordinators are still alive and still hold the | ||||||
| // `Arc<FFIPersister>` / `Arc<FFIEventHandler>` whose context | ||||||
| // pointer is `Unmanaged.passUnretained(handler).toOpaque()` | ||||||
| // for the Swift handler objects below — freeing those Swift | ||||||
| // objects now would dangle that pointer and the next | ||||||
| // callback would be a use-after-free. Stash them in a | ||||||
| // process-global leak slot so any final callback still | ||||||
| // sees valid memory. We accept the bounded leak (two small | ||||||
| // objects per non-clean shutdown); a clean shutdown returns | ||||||
| // success and the handlers are released as usual. | ||||||
| let destroyResult = PlatformWalletResult( | ||||||
| platform_wallet_manager_destroy(handle) | ||||||
| ) | ||||||
| if destroyResult.code == .errorShutdownIncomplete { | ||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Retention predicate should be The retention slot exists because any non-
Suggested change
source: ['claude', 'codex'] |
||||||
| PlatformWalletManager._leakedContextLock.lock() | ||||||
| if let h = persistenceHandler { | ||||||
| PlatformWalletManager._leakedContext.append(h) | ||||||
| } | ||||||
| if let h = eventHandler { | ||||||
| PlatformWalletManager._leakedContext.append(h) | ||||||
| } | ||||||
| PlatformWalletManager._leakedContextLock.unlock() | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
155
to
184
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: No test exercises the Two regressions this PR fixes are entirely uncovered: (a) the Swift mirror drift where source: ['claude', 'codex']
Comment on lines
155
to
184
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: No Swift coverage for the errorShutdownIncomplete mirror or the deinit retention path Two cross-boundary regressions this PR fixes remain entirely uncovered on the Swift side: (a) source: ['claude', 'codex'] |
||||||
|
|
||||||
| /// Lock guarding `_leakedContext`. Both are `nonisolated` so the | ||||||
| /// (implicitly nonisolated) `deinit` can append to the leak slot | ||||||
| /// without an isolation hop. | ||||||
| nonisolated static let _leakedContextLock = NSLock() | ||||||
| /// Process-global retention list for persister / event-handler | ||||||
| /// objects we cannot release at `deinit` because a lingering Rust | ||||||
| /// coordinator thread (`errorShutdownIncomplete` from `destroy`) | ||||||
| /// may still callback through their `Unmanaged.passUnretained` | ||||||
| /// context pointer. `AnyObject` keeps the existential opaque so | ||||||
| /// `PlatformWalletPersistenceHandler` / `PlatformWalletEventHandler` | ||||||
| /// don't need to be `Sendable` to be retained here. | ||||||
| nonisolated(unsafe) static var _leakedContext: [AnyObject] = [] | ||||||
|
|
||||||
| // MARK: - Configuration | ||||||
|
|
||||||
| /// Configure the manager with an SDK and an optional SwiftData | ||||||
|
|
||||||
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.
🟡 Suggestion: Retain on any non-success code, not just
.errorShutdownIncompleteThe retention branch matches exactly on
.errorShutdownIncomplete, but the Rust contract on the destroy boundary is broader: only aSuccessreturn guarantees no coordinator thread is still holdingArc<FFIPersister>/Arc<FFIEventHandler>whose context isUnmanaged.passUnretained(handler).toOpaque(). Today destroy only returnsSuccessorErrorShutdownIncomplete, so the current check works. However,PlatformWalletResultCode.init(ffi:)atPlatformWalletResult.swift:100-101maps unknown raw values to.errorUnknown, so if Rust ever grows another unclean-shutdown variant (e.g. a panicked-thread code) and Swift isn't regenerated in lockstep, the deinit will silently free the handler objects while a coordinator thread may still call back through them — re-opening the UAF. Inverting the check is the same one-line change but stays correct for any future destroy() return code.source: ['claude']