Fix Reachability Safety (Alamofire#3684)

### Issue Link 🔗 Alamofire#3676 ### Goals ⚽ This PR updates our integration of `SCNetworkReachabilityContext` to use a weak box class and fully integrate with the framework's retain and release closures. This follows the same logic as [Reachability.swift](https://github.com/ashleymills/Reachability.swift/blob/a81b7367f2c46875f29577e03a60c39cdfad0c8d/Sources/Reachability.swift#L190). ### Implementation Details 🚧 This PR adds a `WeakManager` type to wrap a weak reference to the `NetworkReachabilityManager` and updates the context memory management closures to properly retain and release the values. ### Testing Details 🔍 No tests added, wasn't able to find a way to trigger the reported crash. Since this logic matches that of the other popular reachability library I'm confident it's correct.
Mohamed-AbdulRaouf · Feb 7, 2023 · 337b29a · 337b29a
1 parent 728de0e
commit 337b29a
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 17 deletions.
diff --git a/.swiftformat b/.swiftformat
@@ -24,6 +24,6 @@
 --nospaceoperators ..<, ...
 --selfrequired validate
 --stripunusedargs closure-only
---wraparguments after-first
---wrapcollections after-first
---wrapparameters after-first
+--wraparguments preserve
+--wrapcollections preserve
+--wrapparameters preserve
diff --git a/Source/NetworkReachabilityManager.swift b/Source/NetworkReachabilityManager.swift
@@ -173,16 +173,33 @@ open class NetworkReachabilityManager {
             state.listener = listener
         }
 
-        var context = SCNetworkReachabilityContext(version: 0,
-                                                   info: Unmanaged.passUnretained(self).toOpaque(),
-                                                   retain: nil,
-                                                   release: nil,
-                                                   copyDescription: nil)
+        let weakManager = WeakManager(manager: self)
+
+        var context = SCNetworkReachabilityContext(
+            version: 0,
+            info: Unmanaged.passUnretained(weakManager).toOpaque(),
+            retain: { info in
+                let unmanaged = Unmanaged<WeakManager>.fromOpaque(info)
+                _ = unmanaged.retain()
+
+                return UnsafeRawPointer(unmanaged.toOpaque())
+            },
+            release: { info in
+                let unmanaged = Unmanaged<WeakManager>.fromOpaque(info)
+                unmanaged.release()
+            },
+            copyDescription: { info in
+                let unmanaged = Unmanaged<WeakManager>.fromOpaque(info)
+                let weakManager = unmanaged.takeUnretainedValue()
+                let description = weakManager.manager?.flags?.readableDescription ?? "nil"
+
+                return Unmanaged.passRetained(description as CFString)
+            })
         let callback: SCNetworkReachabilityCallBack = { _, flags, info in
             guard let info = info else { return }
 
-            let instance = Unmanaged<NetworkReachabilityManager>.fromOpaque(info).takeUnretainedValue()
-            instance.notifyListener(flags)
+            let weakManager = Unmanaged<WeakManager>.fromOpaque(info).takeUnretainedValue()
+            weakManager.manager?.notifyListener(flags)
         }
 
         let queueAdded = SCNetworkReachabilitySetDispatchQueue(reachability, reachabilityQueue)
@@ -228,6 +245,14 @@ open class NetworkReachabilityManager {
             state.listenerQueue?.async { listener?(newStatus) }
         }
     }
+
+    private final class WeakManager {
+        weak var manager: NetworkReachabilityManager?
+
+        init(manager: NetworkReachabilityManager?) {
+            self.manager = manager
+        }
+    }
 }
 
 // MARK: -

diff --git a/Source/RetryPolicy.swift b/Source/RetryPolicy.swift
@@ -56,7 +56,7 @@ open class RetryPolicy: RequestInterceptor {
     ]
 
     /// The default URL error codes to retry.
-    public static let defaultRetryableURLErrorCodes: Set<URLError.Code> = [// [Security] App Transport Security disallowed a connection because there is no secure network connection.
+    public static let defaultRetryableURLErrorCodes: Set<URLError.Code> = [ // [Security] App Transport Security disallowed a connection because there is no secure network connection.
         //   - [Disabled] ATS settings do not change at runtime.
         // .appTransportSecurityRequiresSecureConnection,
 

diff --git a/Tests/ParameterEncodingTests.swift b/Tests/ParameterEncodingTests.swift
@@ -236,7 +236,7 @@ final class URLParameterEncodingTestCase: ParameterEncodingTestCase {
     func testURLParameterLiteralBoolEncodingWorksAndDoesNotAffectNumbers() throws {
         // Given
         let encoding = URLEncoding(boolEncoding: .literal)
-        let parameters: [String: Any] = [// Must still encode to numbers
+        let parameters: [String: Any] = [ // Must still encode to numbers
             "a": 1,
             "b": 0,
             "c": 1.0,
@@ -250,7 +250,8 @@ final class URLParameterEncodingTestCase: ParameterEncodingTestCase {
             "i": true,
             "j": false,
             "k": NSNumber(value: true),
-            "l": NSNumber(value: false)]
+            "l": NSNumber(value: false)
+        ]
 
         // When
         let urlRequest = try encoding.encode(urlRequest, with: parameters)

diff --git a/Tests/ValidationTests.swift b/Tests/ValidationTests.swift
@@ -257,7 +257,7 @@ final class ContentTypeValidationTestCase: BaseTestCase {
         var requestError: AFError?
         var downloadError: AFError?
 
-        let acceptableContentTypes = [// Sorted in a random order, not alphabetically
+        let acceptableContentTypes = [ // Sorted in a random order, not alphabetically
             "application/octet-stream",
             "image/gif",
             "image/x-xbitmap",
@@ -271,7 +271,8 @@ final class ContentTypeValidationTestCase: BaseTestCase {
             "image/ico",
             "image/bmp",
             "image/x-ms-bmp",
-            "image/x-win-bitmap"]
+            "image/x-win-bitmap"
+        ]
 
         // When
         AF.request(endpoint)
@@ -294,7 +295,7 @@ final class ContentTypeValidationTestCase: BaseTestCase {
         XCTAssertNotNil(requestError)
         XCTAssertNotNil(downloadError)
 
-        let expectedAcceptableContentTypes = [// Sorted in a specific order, alphabetically
+        let expectedAcceptableContentTypes = [ // Sorted in a specific order, alphabetically
             "application/octet-stream",
             "image/bmp",
             "image/gif",
@@ -308,7 +309,8 @@ final class ContentTypeValidationTestCase: BaseTestCase {
             "image/x-icon",
             "image/x-ms-bmp",
             "image/x-win-bitmap",
-            "image/x-xbitmap"]
+            "image/x-xbitmap"
+        ]
 
         for error in [requestError, downloadError] {
             XCTAssertEqual(error?.isUnacceptableContentType, true)