From 889c1a971cb18dea192f9be03a32c3921ded74b4 Mon Sep 17 00:00:00 2001 From: Jamil Date: Fri, 30 May 2025 13:54:13 -0700 Subject: [PATCH] fix(apple): Correctly handle stopTunnel and completionHandlers (#9308) This PR fixes two crashes related to lifetimes on Apple: - `completionHandler` was being called from within a Task executor context, which could be different from the one the IPC call was received on - The `getLogFolderSize` task could return and attempt to call `completionHandler` after the PacketTunnelProvider deinit'd - We were calling the completionHandler from `stopTunnel` manually. Apple explicitly says not to do this. Instead, we must call `cancelTunnelWithError(nil)` when we want to stop the tunnel from e.g. the `onDisconnect`. Apple with then call our `stopTunnel` override. The downside is that we have no control over the `NEProviderStopReason` received in this callback, but we don't use it anyway. Instead, we write the reason to a temporary file and read it from the GUI process when we detect a status change to `disconnected`. When that occurs, we're able to show a UI notification (macOS only - iOS can show this notification from the PacketTunnelProvider itself). --- .../FirezoneKit/Helpers/IPCClient.swift | 1 + .../FirezoneKit/Helpers/SharedAccess.swift | 11 ---- .../FirezoneNetworkExtension/Adapter.swift | 22 +++++--- .../PacketTunnelProvider.swift | 51 +++++-------------- website/src/components/Changelog/Apple.tsx | 4 ++ 5 files changed, 32 insertions(+), 57 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift index 41cee66c0..83e02e739 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift @@ -65,6 +65,7 @@ class IPCClient { func signOut() async throws { try await sendMessageWithoutResponse(ProviderMessage.signOut) + try stop() } func stop() throws { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/SharedAccess.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/SharedAccess.swift index 6a193de40..0550f2f44 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/SharedAccess.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/SharedAccess.swift @@ -7,17 +7,6 @@ import Foundation public struct SharedAccess { - public enum Error: Swift.Error { - case unableToWriteToFile(URL, Swift.Error) - - var localizedDescription: String { - switch self { - case .unableToWriteToFile(let url, let error): - return "Unable to write to \(url): \(error)" - } - } - } - public static var baseFolderURL: URL { guard let url = FileManager.default.containerURL( diff --git a/swift/apple/FirezoneNetworkExtension/Adapter.swift b/swift/apple/FirezoneNetworkExtension/Adapter.swift index a96e53d42..45cf357ee 100644 --- a/swift/apple/FirezoneNetworkExtension/Adapter.swift +++ b/swift/apple/FirezoneNetworkExtension/Adapter.swift @@ -425,16 +425,24 @@ extension Adapter: CallbackHandlerDelegate { workQueue.async { [weak self] in guard let self = self else { return } - // Set a default stop reason. In the future, we may have more to act upon in - // different ways. - var reason: NEProviderStopReason = .connectionFailed - + // If auth expired/is invalid, delete stored token and save the reason why so the GUI can act upon it. if error.isAuthenticationError() { - reason = .authenticationCanceled + do { + try Token.delete() + let reason: NEProviderStopReason = .authenticationCanceled + try String(reason.rawValue).write( + to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8) + } catch { + Log.error(error) + } +#if os(iOS) + // iOS notifications should be shown from the tunnel process + SessionNotification.showSignedOutNotificationiOS() +#endif } - // Start the process of telling the system to shut us down - self.packetTunnelProvider?.stopTunnel(with: reason) {} + // Tell the system to shut us down + self.packetTunnelProvider?.cancelTunnelWithError(nil) } } diff --git a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift index 4b4f139fc..b064449cd 100644 --- a/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift +++ b/swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift @@ -23,6 +23,8 @@ class PacketTunnelProvider: NEPacketTunnelProvider { case idle } + private var getLogFolderSizeTask: Task? + private var logExportState: LogExportState = .idle private var tunnelConfiguration: TunnelConfiguration? private let defaults = UserDefaults.standard @@ -37,6 +39,10 @@ class PacketTunnelProvider: NEPacketTunnelProvider { self.tunnelConfiguration = TunnelConfiguration.tryLoad() } + deinit { + getLogFolderSizeTask?.cancel() + } + override func startTunnel( options: [String: NSObject]?, completionHandler: @escaping (Error?) -> Void @@ -120,42 +126,16 @@ class PacketTunnelProvider: NEPacketTunnelProvider { ) { Log.log("stopTunnel: Reason: \(reason)") - do { - // There's no good way to send data like this from the - // Network Extension to the GUI, so save it to a file for the GUI to read upon - // either status change or the next launch. - try String(reason.rawValue).write( - to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8) - } catch { - Log.error( - SharedAccess.Error.unableToWriteToFile( - SharedAccess.providerStopReasonURL, - error - ) - ) - } - - if case .authenticationCanceled = reason { - // This was triggered from onDisconnect, so try to clear our token - do { try Token.delete() } catch { Log.error(error) } - -#if os(iOS) - // iOS notifications should be shown from the tunnel process - SessionNotification.showSignedOutNotificationiOS() -#endif - } - // handles both connlib-initiated and user-initiated stops adapter?.stop() - cancelTunnelWithError(nil) super.stopTunnel(with: reason, completionHandler: completionHandler) - completionHandler() } // It would be helpful to be able to encapsulate Errors here. To do that // we need to update ProviderMessage to encode/decode Result to and from Data. // TODO: Move to a more abstract IPC protocol + @MainActor override func handleAppMessage(_ message: Data, completionHandler: ((Data?) -> Void)? = nil) { do { let providerMessage = try PropertyListDecoder().decode(ProviderMessage.self, from: message) @@ -169,16 +149,7 @@ class PacketTunnelProvider: NEPacketTunnelProvider { completionHandler?(nil) case .signOut: - do { - try Token.delete() - Task { - await stopTunnel(with: .userInitiated) - completionHandler?(nil) - } - } catch { - Log.error(error) - completionHandler?(nil) - } + do { try Token.delete() } catch { Log.error(error) } case .getResourceList(let hash): guard let adapter = adapter else { @@ -239,11 +210,13 @@ class PacketTunnelProvider: NEPacketTunnelProvider { return } - Task { + getLogFolderSizeTask = Task { let size = await Log.size(of: logFolderURL) let data = withUnsafeBytes(of: size) { Data($0) } - completionHandler?(data) + // Ensure completionHandler is called on the same actor as handleAppMessage and isn't cancelled by deinit + if getLogFolderSizeTask?.isCancelled ?? true { return } + await MainActor.run { completionHandler?(data) } } } diff --git a/website/src/components/Changelog/Apple.tsx b/website/src/components/Changelog/Apple.tsx index 60af8e57e..51b1636dc 100644 --- a/website/src/components/Changelog/Apple.tsx +++ b/website/src/components/Changelog/Apple.tsx @@ -25,6 +25,10 @@ export default function Apple() { {/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */} + + Fixes a minor crash in the network extension that could occur when + viewing the Diagnostic Logs tab in app settings. + Fixes a rare bug that could prevent certain IPv6 DNS upstream resolvers from being used if they contained an interface scope