Skip to content

Commit f597c50

Browse files
committed
fix: resolve critical concurrency deadlocks and main-thread blocking
- SSH-1 (critical): replace DispatchSemaphore.wait() with withCheckedContinuation in HostKeyVerifier - SVC-3 (high): remove DispatchQueue.main.sync from SQLFormatterService, pass dialect as parameter - DB-3/PLG-1 (high): remove synchronous loadPendingPlugins() fallback, throw error instead - PLG-2 (medium): move SecStaticCodeCheckValidity to background path - SVC-1 (high): move SyncCoordinator storage I/O to Task.detached - APP-3.5 (high): protect waitForConnection continuation with OSAllocatedUnfairLock - APP-4.1 (high): consolidate AppState sync into single syncAppStateWithCurrentSession()
1 parent 8a47d9b commit f597c50

11 files changed

Lines changed: 234 additions & 205 deletions

TablePro/AppDelegate+ConnectionHandler.swift

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -329,13 +329,17 @@ extension AppDelegate {
329329
}
330330

331331
private func waitForConnection(timeout: Duration) async {
332+
let didResume = OSAllocatedUnfairLock(initialState: false)
332333
await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in
333-
var didResume = false
334334
var observer: NSObjectProtocol?
335335

336336
func resumeOnce() {
337-
guard !didResume else { return }
338-
didResume = true
337+
let shouldResume = didResume.withLock { alreadyResumed -> Bool in
338+
if alreadyResumed { return false }
339+
alreadyResumed = true
340+
return true
341+
}
342+
guard shouldResume else { return }
339343
if let obs = observer {
340344
NotificationCenter.default.removeObserver(obs)
341345
}

TablePro/ContentView.swift

Lines changed: 23 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,6 @@ struct ContentView: View {
9292
payload: payload
9393
)
9494
}
95-
AppState.shared.isConnected = true
96-
AppState.shared.safeModeLevel = session.connection.safeModeLevel
97-
AppState.shared.editorLanguage = PluginManager.shared.editorLanguage(for: session.connection.type)
98-
AppState.shared.currentDatabaseType = session.connection.type
99-
AppState.shared.supportsDatabaseSwitching = PluginManager.shared.supportsDatabaseSwitching(
100-
for: session.connection.type)
10195
}
10296
} else {
10397
currentSession = nil
@@ -130,20 +124,7 @@ struct ContentView: View {
130124
}()
131125
guard isOurWindow else { return }
132126

133-
if let session = DatabaseManager.shared.activeSessions[connectionId] {
134-
AppState.shared.isConnected = true
135-
AppState.shared.safeModeLevel = session.connection.safeModeLevel
136-
AppState.shared.editorLanguage = PluginManager.shared.editorLanguage(for: session.connection.type)
137-
AppState.shared.currentDatabaseType = session.connection.type
138-
AppState.shared.supportsDatabaseSwitching = PluginManager.shared.supportsDatabaseSwitching(
139-
for: session.connection.type)
140-
} else {
141-
AppState.shared.isConnected = false
142-
AppState.shared.safeModeLevel = .silent
143-
AppState.shared.editorLanguage = .sql
144-
AppState.shared.currentDatabaseType = nil
145-
AppState.shared.supportsDatabaseSwitching = true
146-
}
127+
syncAppStateWithCurrentSession()
147128
}
148129
.onChange(of: sessionState?.toolbarState.safeModeLevel) { _, newLevel in
149130
if let level = newLevel {
@@ -349,11 +330,7 @@ struct ContentView: View {
349330
sessionState = nil
350331
currentSession = nil
351332
columnVisibility = .detailOnly
352-
AppState.shared.isConnected = false
353-
AppState.shared.safeModeLevel = .silent
354-
AppState.shared.editorLanguage = .sql
355-
AppState.shared.currentDatabaseType = nil
356-
AppState.shared.supportsDatabaseSwitching = true
333+
syncAppStateWithCurrentSession()
357334

358335
let tabbingId = "com.TablePro.main.\(sid.uuidString)"
359336
DispatchQueue.main.async {
@@ -379,12 +356,27 @@ struct ContentView: View {
379356
payload: payload
380357
)
381358
}
382-
AppState.shared.isConnected = true
383-
AppState.shared.safeModeLevel = newSession.connection.safeModeLevel
384-
AppState.shared.editorLanguage = PluginManager.shared.editorLanguage(for: newSession.connection.type)
385-
AppState.shared.currentDatabaseType = newSession.connection.type
386-
AppState.shared.supportsDatabaseSwitching = PluginManager.shared.supportsDatabaseSwitching(
387-
for: newSession.connection.type)
359+
}
360+
361+
/// Single authoritative source for syncing AppState fields with the current session.
362+
/// Called from `windowDidBecomeKey` (the correct trigger for per-window AppState)
363+
/// and from `handleConnectionStatusChange` on disconnect cleanup.
364+
private func syncAppStateWithCurrentSession() {
365+
let connectionId = payload?.connectionId ?? currentSession?.id
366+
if let connectionId, let session = DatabaseManager.shared.activeSessions[connectionId] {
367+
AppState.shared.isConnected = true
368+
AppState.shared.safeModeLevel = session.connection.safeModeLevel
369+
AppState.shared.editorLanguage = PluginManager.shared.editorLanguage(for: session.connection.type)
370+
AppState.shared.currentDatabaseType = session.connection.type
371+
AppState.shared.supportsDatabaseSwitching = PluginManager.shared.supportsDatabaseSwitching(
372+
for: session.connection.type)
373+
} else {
374+
AppState.shared.isConnected = false
375+
AppState.shared.safeModeLevel = .silent
376+
AppState.shared.editorLanguage = .sql
377+
AppState.shared.currentDatabaseType = nil
378+
AppState.shared.supportsDatabaseSwitching = true
379+
}
388380
}
389381

390382
// MARK: - Actions

TablePro/Core/Database/DatabaseDriver.swift

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -317,13 +317,12 @@ extension DatabaseDriver {
317317
enum DatabaseDriverFactory {
318318
static func createDriver(for connection: DatabaseConnection) throws -> DatabaseDriver {
319319
let pluginId = connection.type.pluginTypeId
320-
// If the plugin isn't registered yet and background loading hasn't finished,
321-
// fall back to synchronous loading for this critical code path.
322-
if PluginManager.shared.driverPlugins[pluginId] == nil,
323-
!PluginManager.shared.hasFinishedInitialLoad {
324-
PluginManager.shared.loadPendingPlugins()
325-
}
326320
guard let plugin = PluginManager.shared.driverPlugins[pluginId] else {
321+
// If background loading hasn't finished yet, throw a specific error
322+
// instead of blocking the main thread with synchronous plugin loading.
323+
if !PluginManager.shared.hasFinishedInitialLoad {
324+
throw PluginError.pluginNotLoaded(pluginId)
325+
}
327326
if connection.type.isDownloadablePlugin {
328327
throw PluginError.pluginNotInstalled(connection.type.rawValue)
329328
}

TablePro/Core/Plugins/PluginError.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ enum PluginError: LocalizedError {
2020
case pluginNotInstalled(String)
2121
case incompatibleWithCurrentApp(minimumRequired: String)
2222
case invalidDescriptor(pluginId: String, reason: String)
23+
case pluginNotLoaded(String)
2324

2425
var errorDescription: String? {
2526
switch self {
@@ -51,6 +52,8 @@ enum PluginError: LocalizedError {
5152
return String(localized: "This plugin requires TablePro \(minimumRequired) or later")
5253
case .invalidDescriptor(let pluginId, let reason):
5354
return String(localized: "Plugin '\(pluginId)' has an invalid descriptor: \(reason)")
55+
case .pluginNotLoaded(let databaseType):
56+
return String(localized: "The \(databaseType) driver plugin is still loading. Please try again in a moment.")
5457
}
5558
}
5659
}

TablePro/Core/Plugins/PluginManager.swift

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,14 @@ final class PluginManager {
142142
logger.error("User plugin \(entry.url.lastPathComponent) has outdated PluginKit v\(pluginKitVersion)")
143143
continue
144144
}
145+
146+
// Verify code signature off the main thread (disk I/O in SecStaticCodeCheckValidity)
147+
do {
148+
try verifyCodeSignatureStatic(bundle: bundle)
149+
} catch {
150+
logger.error("Code signature verification failed for \(entry.url.lastPathComponent): \(error.localizedDescription)")
151+
continue
152+
}
145153
}
146154

147155
// Heavy I/O: dynamic linker resolution, C bridge library initialization
@@ -308,7 +316,8 @@ final class PluginManager {
308316
current: pluginKitVersion
309317
)
310318
}
311-
try verifyCodeSignature(bundle: bundle)
319+
// Code signature verification is deferred to loadBundlesOffMain()
320+
// to avoid blocking the main thread with SecStaticCodeCheckValidity disk I/O.
312321
}
313322

314323
pendingPluginURLs.append((url: url, source: source))
@@ -1048,13 +1057,21 @@ final class PluginManager {
10481057
private static let signingTeamId = "D7HJ5TFYCU"
10491058

10501059
private func createSigningRequirement() -> SecRequirement? {
1060+
Self.createSigningRequirementStatic()
1061+
}
1062+
1063+
nonisolated private static func createSigningRequirementStatic() -> SecRequirement? {
10511064
var requirement: SecRequirement?
1052-
let requirementString = "anchor apple generic and certificate leaf[subject.OU] = \"\(Self.signingTeamId)\"" as CFString
1065+
let requirementString = "anchor apple generic and certificate leaf[subject.OU] = \"\(signingTeamId)\"" as CFString
10531066
SecRequirementCreateWithString(requirementString, SecCSFlags(), &requirement)
10541067
return requirement
10551068
}
10561069

10571070
private func verifyCodeSignature(bundle: Bundle) throws {
1071+
try Self.verifyCodeSignatureStatic(bundle: bundle)
1072+
}
1073+
1074+
nonisolated private static func verifyCodeSignatureStatic(bundle: Bundle) throws {
10581075
var staticCode: SecStaticCode?
10591076
let createStatus = SecStaticCodeCreateWithPath(
10601077
bundle.bundleURL as CFURL,
@@ -1064,11 +1081,11 @@ final class PluginManager {
10641081

10651082
guard createStatus == errSecSuccess, let code = staticCode else {
10661083
throw PluginError.signatureInvalid(
1067-
detail: Self.describeOSStatus(createStatus)
1084+
detail: describeOSStatus(createStatus)
10681085
)
10691086
}
10701087

1071-
let requirement = createSigningRequirement()
1088+
let requirement = createSigningRequirementStatic()
10721089

10731090
let checkStatus = SecStaticCodeCheckValidity(
10741091
code,
@@ -1078,12 +1095,12 @@ final class PluginManager {
10781095

10791096
guard checkStatus == errSecSuccess else {
10801097
throw PluginError.signatureInvalid(
1081-
detail: Self.describeOSStatus(checkStatus)
1098+
detail: describeOSStatus(checkStatus)
10821099
)
10831100
}
10841101
}
10851102

1086-
private static func describeOSStatus(_ status: OSStatus) -> String {
1103+
nonisolated private static func describeOSStatus(_ status: OSStatus) -> String {
10871104
switch status {
10881105
case -67_062: return "bundle is not signed"
10891106
case -67_061: return "code signature is invalid"

TablePro/Core/SSH/HostKeyVerifier.swift

Lines changed: 38 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@ internal enum HostKeyVerifier {
1515
private static let logger = Logger(subsystem: "com.TablePro", category: "HostKeyVerifier")
1616

1717
/// Verify the host key, prompting the user if needed.
18-
/// This method blocks the calling thread while showing UI prompts.
19-
/// Must be called from a background thread.
18+
/// Uses `withCheckedContinuation` to await UI prompts without blocking the cooperative thread pool.
2019
/// - Parameters:
2120
/// - keyData: The raw host key bytes from the SSH session
2221
/// - keyType: The key type string (e.g. "ssh-rsa", "ssh-ed25519")
@@ -28,7 +27,7 @@ internal enum HostKeyVerifier {
2827
keyType: String,
2928
hostname: String,
3029
port: Int
31-
) throws {
30+
) async throws {
3231
let result = HostKeyStore.shared.verify(
3332
keyData: keyData,
3433
keyType: keyType,
@@ -43,7 +42,7 @@ internal enum HostKeyVerifier {
4342

4443
case .unknown(let fingerprint, let keyType):
4544
logger.info("Unknown host key for [\(hostname)]:\(port), prompting user")
46-
let accepted = promptUnknownHost(
45+
let accepted = await promptUnknownHost(
4746
hostname: hostname,
4847
port: port,
4948
fingerprint: fingerprint,
@@ -62,7 +61,7 @@ internal enum HostKeyVerifier {
6261

6362
case .mismatch(let expected, let actual):
6463
logger.warning("Host key mismatch for [\(hostname)]:\(port)")
65-
let accepted = promptHostKeyMismatch(
64+
let accepted = await promptHostKeyMismatch(
6665
hostname: hostname,
6766
port: port,
6867
expected: expected,
@@ -83,17 +82,14 @@ internal enum HostKeyVerifier {
8382

8483
// MARK: - UI Prompts
8584

86-
/// Show a dialog asking the user whether to trust an unknown host
87-
/// Blocks the calling thread until the user responds.
85+
/// Show a dialog asking the user whether to trust an unknown host.
86+
/// Suspends until the user responds, without blocking any thread.
8887
private static func promptUnknownHost(
8988
hostname: String,
9089
port: Int,
9190
fingerprint: String,
9291
keyType: String
93-
) -> Bool {
94-
let semaphore = DispatchSemaphore(value: 0)
95-
var accepted = false
96-
92+
) async -> Bool {
9793
let hostDisplay = "[\(hostname)]:\(port)"
9894
let title = String(localized: "Unknown SSH Host")
9995
let message = String(localized: """
@@ -105,34 +101,29 @@ internal enum HostKeyVerifier {
105101
Are you sure you want to continue connecting?
106102
""")
107103

108-
DispatchQueue.main.async {
109-
let alert = NSAlert()
110-
alert.messageText = title
111-
alert.informativeText = message
112-
alert.alertStyle = .informational
113-
alert.addButton(withTitle: String(localized: "Trust"))
114-
alert.addButton(withTitle: String(localized: "Cancel"))
115-
116-
let response = alert.runModal()
117-
accepted = (response == .alertFirstButtonReturn)
118-
semaphore.signal()
104+
return await withCheckedContinuation { continuation in
105+
DispatchQueue.main.async {
106+
let alert = NSAlert()
107+
alert.messageText = title
108+
alert.informativeText = message
109+
alert.alertStyle = .informational
110+
alert.addButton(withTitle: String(localized: "Trust"))
111+
alert.addButton(withTitle: String(localized: "Cancel"))
112+
113+
let response = alert.runModal()
114+
continuation.resume(returning: response == .alertFirstButtonReturn)
115+
}
119116
}
120-
121-
semaphore.wait()
122-
return accepted
123117
}
124118

125-
/// Show a warning dialog about a changed host key (potential MITM attack)
126-
/// Blocks the calling thread until the user responds.
119+
/// Show a warning dialog about a changed host key (potential MITM attack).
120+
/// Suspends until the user responds, without blocking any thread.
127121
private static func promptHostKeyMismatch(
128122
hostname: String,
129123
port: Int,
130124
expected: String,
131125
actual: String
132-
) -> Bool {
133-
let semaphore = DispatchSemaphore(value: 0)
134-
var accepted = false
135-
126+
) async -> Bool {
136127
let hostDisplay = "[\(hostname)]:\(port)"
137128
let title = String(localized: "SSH Host Key Changed")
138129
let message = String(localized: """
@@ -144,24 +135,22 @@ internal enum HostKeyVerifier {
144135
Current fingerprint: \(actual)
145136
""")
146137

147-
DispatchQueue.main.async {
148-
let alert = NSAlert()
149-
alert.messageText = title
150-
alert.informativeText = message
151-
alert.alertStyle = .critical
152-
alert.addButton(withTitle: String(localized: "Connect Anyway"))
153-
alert.addButton(withTitle: String(localized: "Disconnect"))
154-
155-
// Make "Disconnect" the default button (Return key) instead of "Connect Anyway"
156-
alert.buttons[1].keyEquivalent = "\r"
157-
alert.buttons[0].keyEquivalent = ""
158-
159-
let response = alert.runModal()
160-
accepted = (response == .alertFirstButtonReturn)
161-
semaphore.signal()
138+
return await withCheckedContinuation { continuation in
139+
DispatchQueue.main.async {
140+
let alert = NSAlert()
141+
alert.messageText = title
142+
alert.informativeText = message
143+
alert.alertStyle = .critical
144+
alert.addButton(withTitle: String(localized: "Connect Anyway"))
145+
alert.addButton(withTitle: String(localized: "Disconnect"))
146+
147+
// Make "Disconnect" the default button (Return key) instead of "Connect Anyway"
148+
alert.buttons[1].keyEquivalent = "\r"
149+
alert.buttons[0].keyEquivalent = ""
150+
151+
let response = alert.runModal()
152+
continuation.resume(returning: response == .alertFirstButtonReturn)
153+
}
162154
}
163-
164-
semaphore.wait()
165-
return accepted
166155
}
167156
}

0 commit comments

Comments
 (0)