Merge pull request #338 from teleportr-app/bugfix/dealloc-race-condition

Fix dealloc race condition

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 ### Thanks to:  
 - @p4checo
+- @connorpower
 
 ### Changed
 - `init()` methods now `throw`
@@ -15,6 +16,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Renamed error cases to start with lowercase
 ### Added
 - Allow configuring the notification `DispatchQueue`, which was previously hardcoded to `DispatchQueue.main`. It is now an optional, which if set to `nil` will use the notifier's internal queue to fire notifications. The default is still `.main`
+### Fixed
+- Fixed a crash which could occur if Reachability was deallocated at the same time a system thread was calling back into Reachability
 
 ## [4.3.1] - 2018-10-18
 ### Fixed 

Sources/Reachability.swift

@@ -172,12 +172,37 @@ public extension Reachability {
         let callback: SCNetworkReachabilityCallBack = { (reachability, flags, info) in
             guard let info = info else { return }
 
-            let reachability = Unmanaged<Reachability>.fromOpaque(info).takeUnretainedValue()
-            reachability.flags = flags
+            // `weakifiedReachability` is guaranteed to exist by virtue of our
+            // retain/release callbacks which we provided to the `SCNetworkReachabilityContext`.
+            let weakifiedReachability = Unmanaged<ReachabilityWeakifier>.fromOpaque(info).takeUnretainedValue()
+
+            // The weak `reachability` _may_ no longer exist if the `Reachability`
+            // object has since been deallocated but a callback was already in flight.
+            weakifiedReachability.reachability?.flags = flags
         }
 
-        var context = SCNetworkReachabilityContext(version: 0, info: nil, retain: nil, release: nil, copyDescription: nil)
-        context.info = Unmanaged.passUnretained(self).toOpaque()
+        let weakifiedReachability = ReachabilityWeakifier(reachability: self)
+        let opaqueWeakifiedReachability = Unmanaged<ReachabilityWeakifier>.passUnretained(weakifiedReachability).toOpaque()
+
+        var context = SCNetworkReachabilityContext(
+            version: 0,
+            info: UnsafeMutableRawPointer(opaqueWeakifiedReachability),
+            retain: { (info: UnsafeRawPointer) -> UnsafeRawPointer in
+                let unmanagedWeakifiedReachability = Unmanaged<ReachabilityWeakifier>.fromOpaque(info)
+                _ = unmanagedWeakifiedReachability.retain()
+                return UnsafeRawPointer(unmanagedWeakifiedReachability.toOpaque())
+            },
+            release: { (info: UnsafeRawPointer) -> Void in
+                let unmanagedWeakifiedReachability = Unmanaged<ReachabilityWeakifier>.fromOpaque(info)
+                unmanagedWeakifiedReachability.release()
+            },
+            copyDescription: { (info: UnsafeRawPointer) -> Unmanaged<CFString> in
+                let unmanagedWeakifiedReachability = Unmanaged<ReachabilityWeakifier>.fromOpaque(info)
+                let weakifiedReachability = unmanagedWeakifiedReachability.takeUnretainedValue()
+                let description = weakifiedReachability.reachability?.description ?? "nil"
+                return Unmanaged.passRetained(description as CFString)
+            }
+        )
 
         if !SCNetworkReachabilitySetCallback(reachabilityRef, callback, &context) {
             stopNotifier()
@@ -334,3 +359,44 @@ extension SCNetworkReachabilityFlags {
         return "\(W)\(R) \(c)\(t)\(i)\(C)\(D)\(l)\(d)"
     }
 }
+
+/**
+ `ReachabilityWeakifier` weakly wraps the `Reachability` class
+ in order to break retain cycles when interacting with CoreFoundation.
+
+ CoreFoundation callbacks expect a pair of retain/release whenever an
+ opaque `info` parameter is provided. These callbacks exist to guard
+ against memory management race conditions when invoking the callbacks.
+
+ #### Race Condition
+
+ If we passed `SCNetworkReachabilitySetCallback` a direct reference to our
+ `Reachability` class without also providing corresponding retain/release
+ callbacks, then a race condition can lead to crashes when:
+ - `Reachability` is deallocated on thread X
+ - A `SCNetworkReachability` callback(s) is already in flight on thread Y
+
+ #### Retain Cycle
+
+ If we pass `Reachability` to CoreFoundtion while also providing retain/
+ release callbacks, we would create a retain cycle once CoreFoundation
+ retains our `Reachability` class. This fixes the crashes and his how
+ CoreFoundation expects the API to be used, but doesn't play nicely with
+ Swift/ARC. This cycle would only be broken after manually calling
+ `stopNotifier()` — `deinit` would never be called.
+
+ #### ReachabilityWeakifier
+
+ By providing both retain/release callbacks and wrapping `Reachability` in
+ a weak wrapper, we:
+ - interact correctly with CoreFoundation, thereby avoiding a crash.
+ See "Memory Management Programming Guide for Core Foundation".
+ - don't alter the public API of `Reachability.swift` in any way
+ - still allow for automatic stopping of the notifier on `deinit`.
+ */
+private class ReachabilityWeakifier {
+    weak var reachability: Reachability?
+    init(reachability: Reachability) {
+        self.reachability = reachability
+    }
+}