Fix getNetworkID race condition

Ensures reachability status is valid before GoPsiStart is
called so getNetworkID returns the correct network ID
instead of UNKNOWN.

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/Network/Reachability/DefaultRouteMonitor.m

@@ -49,10 +49,10 @@
 
 @implementation DefaultRouteMonitor {
     nw_path_monitor_t monitor;
-    dispatch_queue_t workQueue;
+    dispatch_queue_t nwPathMonitorQueue;
+    dispatch_queue_t notifQueue;
     NetworkReachability status;
     NetworkPathState* state;
-    BOOL emitReachabilityUpdates;
 
     void (^logger) (NSString *_Nonnull);
 }
@@ -60,28 +60,8 @@
 - (void)initialize API_AVAILABLE(macos(10.14), ios(12.0), watchos(5.0), tvos(12.0)) {
     self->state = nil;
     self->status = NetworkReachabilityNotReachable;
-    self->emitReachabilityUpdates = FALSE;
-    self->monitor = nw_path_monitor_create();
-    self->workQueue = dispatch_queue_create("com.psiphon3.library.NWInterfaceNWPathMonitorQueue", DISPATCH_QUEUE_SERIAL);
-
-    nw_path_monitor_set_queue(self->monitor, self->workQueue);
-    __block dispatch_group_t group = dispatch_group_create();
-    dispatch_group_enter(group);
-
-    nw_path_monitor_set_update_handler(self->monitor, ^(nw_path_t  _Nonnull path) {
-        @synchronized (self) {
-            [self pathUpdateHandler:path];
-            if (group != NULL) {
-                dispatch_group_leave(group);
-                group = NULL;
-            }
-        }
-    });
-    nw_path_monitor_start(self->monitor);
-
-    // Wait for the current path to be emitted before returning to ensure this instance is populated
-    // with the current network state before use. PsiphonTunnel depends on this guarantee.
-    dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+    self->nwPathMonitorQueue = dispatch_queue_create("com.psiphon3.library.DefaultRouteMonitorNWPathMonitorQueue", DISPATCH_QUEUE_SERIAL);
+    self->notifQueue = dispatch_queue_create("com.psiphon3.library.DefaultRouteMonitorNotificationQueue", DISPATCH_QUEUE_SERIAL);
 }
 
 - (instancetype)init {
@@ -92,13 +72,6 @@
     return self;
 }
 
-- (void)dealloc API_AVAILABLE(macos(10.14), ios(12.0), watchos(5.0), tvos(12.0)) {
-    // Note: this monitor cannot be used after being cancelled. Its update handler will not
-    // fire again and cannot be restarted with nw_path_monitor_start. A new monitor must be
-    // created and started.
-    nw_path_monitor_cancel(self->monitor);
-}
-
 - (instancetype)initWithLogger:(void (^__nonnull)(NSString *_Nonnull))logger {
     self = [super init];
     if (self) {
@@ -150,13 +123,38 @@ NetworkReachability nw_interface_type_network_reachability(nw_interface_type_t i
 
 - (void)start API_AVAILABLE(macos(10.14), ios(12.0), watchos(5.0), tvos(12.0)) {
     @synchronized (self) {
-        self->emitReachabilityUpdates = TRUE;
+        // Ensure previous monitor cancelled
+        if (self->monitor != nil) {
+            nw_path_monitor_cancel(self->monitor);
+        }
+        self->monitor = nw_path_monitor_create();
+
+        nw_path_monitor_set_queue(self->monitor, self->nwPathMonitorQueue);
+        __block dispatch_group_t group = dispatch_group_create();
+        dispatch_group_enter(group);
+
+        nw_path_monitor_set_update_handler(self->monitor, ^(nw_path_t  _Nonnull path) {
+            [self pathUpdateHandler:path];
+            if (group != NULL) {
+                dispatch_group_leave(group);
+                group = NULL;
+            }
+        });
+        nw_path_monitor_start(self->monitor);
+
+        // Wait for the current path to be emitted before returning to ensure this instance is
+        // populated with the current network state. PsiphonTunnel depends on this guarantee.
+        dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
     }
 }
 
 - (void)stop API_AVAILABLE(macos(10.14), ios(12.0), watchos(5.0), tvos(12.0)) {
     @synchronized (self) {
-        self->emitReachabilityUpdates = FALSE;
+        // Note: this monitor cannot be used after being cancelled. Its update handler will not
+        // fire again and cannot be restarted with nw_path_monitor_start. A new monitor must be
+        // created and started.
+        nw_path_monitor_cancel(self->monitor);
+        self->monitor = nil;
     }
 }
 
@@ -227,17 +225,17 @@ NetworkReachability nw_interface_type_network_reachability(nw_interface_type_t i
     }
     self->state = newPathState;
 
-    if (self->emitReachabilityUpdates) {
-        // Backwards compatibility with Reachability
-        ReachabilityChangedNotification *notif =
-            [[ReachabilityChangedNotification alloc]
-             initWithReachabilityStatus:self->status
-             curDefaultActiveInterfaceName:newPathState.defaultActiveInterfaceName
-             prevDefaultActiveInterfaceName:prevDefaultActiveInterfaceName];
+    // Backwards compatibility with Reachability
+    ReachabilityChangedNotification *notif =
+        [[ReachabilityChangedNotification alloc]
+         initWithReachabilityStatus:self->status
+         curDefaultActiveInterfaceName:newPathState.defaultActiveInterfaceName
+         prevDefaultActiveInterfaceName:prevDefaultActiveInterfaceName];
+    dispatch_async(self->notifQueue, ^{
         [[NSNotificationCenter defaultCenter]
          postNotificationName:[DefaultRouteMonitor reachabilityChangedNotification]
          object:notif];
-    }
+    });
 }
 
 + (NSString*)pathDebugInfo:(nw_path_t)path API_AVAILABLE(macos(10.14), ios(12.0), watchos(5.0), tvos(12.0)) {
@@ -313,6 +311,8 @@ NetworkReachability nw_interface_type_network_reachability(nw_interface_type_t i
 }
 
 - (NetworkReachability)reachabilityStatus {
+    // Note: alternatively we could initialize a temporary NWPathMonitor instance and sample the
+    // reachability state by synchronously waiting for its initial update.
     return self->status;
 }
 

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m

@@ -340,6 +340,8 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
 
         [self changeConnectionStateTo:PsiphonConnectionStateConnecting evenIfSameState:NO];
 
+        [self startInternetReachabilityMonitoring];
+
         @try {
             NSError *e = nil;
 
@@ -376,8 +378,6 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
             return FALSE;
         }
 
-        [self startInternetReachabilityMonitoring];
-
         [self logMessage:@"Psiphon library started"];
         
         return TRUE;
@@ -1432,10 +1432,15 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
 // time for the tunnel to notice the network is gone (depending on attempts to
 // use the tunnel).
 - (void)startInternetReachabilityMonitoring {
-    atomic_store(&self->currentNetworkStatus, [self->reachability reachabilityStatus]);
-
-    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(internetReachabilityChanged:) name:kReachabilityChangedNotification object:nil];
+    // (1) Start notifier and then (2) sample the reachability status to bootstrap current network
+    // status. This ordering is required to ensure we do not miss any reachability status updates.
+    // Note: this function must complete execution before any reachability changed notifications are
+    // processed to ensure ordering; otherwise (2) may overwrite the current network status with a
+    // stale value in the unlikely event where a reachability changed notification is emitted
+    // immediately after (1).
+    [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(internetReachabilityChanged:) name:kReachabilityChangedNotification object :nil];
     [self->reachability startNotifier];
+    atomic_store(&self->currentNetworkStatus, [self->reachability reachabilityStatus]);
 }
 
 - (void)stopInternetReachabilityMonitoring {
@@ -1444,45 +1449,49 @@ typedef NS_ERROR_ENUM(PsiphonTunnelErrorDomain, PsiphonTunnelErrorCode) {
 }
 
 - (void)internetReachabilityChanged:(NSNotification *)note {
-    // Invalidate initialDNSCache.
-    atomic_store(&self->useInitialDNS, FALSE);
-
-    NetworkReachability networkStatus;
-    NetworkReachability previousNetworkStatus;
-    BOOL interfaceChanged = FALSE;
-
-    // Pass current reachability through to the delegate
-    // as soon as a network reachability change is detected
-    if (@available(iOS 12.0, *)) {
-        ReachabilityChangedNotification *notif = [note object];
-        networkStatus = notif.reachabilityStatus;
-        if (notif.prevDefaultActiveInterfaceName == nil && notif.curDefaultActiveInterfaceName == nil) {
-            // no interface change
-        } else if (notif.prevDefaultActiveInterfaceName == nil || notif.curDefaultActiveInterfaceName == nil) {
-            // interface appeared or disappeared
-            interfaceChanged = TRUE;
-        } else if (![notif.prevDefaultActiveInterfaceName isEqualToString:notif.curDefaultActiveInterfaceName]) {
-            // active interface changed
-            interfaceChanged = TRUE;
+    // Ensure notifications are not processed until current network status is
+    // bootstrapped. See comment in startInternetReachabilityMonitoring.
+    @synchronized (PsiphonTunnel.self) {
+        // Invalidate initialDNSCache.
+        atomic_store(&self->useInitialDNS, FALSE);
+
+        NetworkReachability networkStatus;
+        NetworkReachability previousNetworkStatus;
+        BOOL interfaceChanged = FALSE;
+
+        // Pass current reachability through to the delegate
+        // as soon as a network reachability change is detected
+        if (@available(iOS 12.0, *)) {
+            ReachabilityChangedNotification *notif = [note object];
+            networkStatus = notif.reachabilityStatus;
+            if (notif.prevDefaultActiveInterfaceName == nil && notif.curDefaultActiveInterfaceName == nil) {
+                // no interface change
+            } else if (notif.prevDefaultActiveInterfaceName == nil || notif.curDefaultActiveInterfaceName == nil) {
+                // interface appeared or disappeared
+                interfaceChanged = TRUE;
+            } else if (![notif.prevDefaultActiveInterfaceName isEqualToString:notif.curDefaultActiveInterfaceName]) {
+                // active interface changed
+                interfaceChanged = TRUE;
+            }
+        } else {
+            Reachability* currentReachability = [note object];
+            networkStatus = [currentReachability reachabilityStatus];
         }
-    } else {
-        Reachability* currentReachability = [note object];
-        networkStatus = [currentReachability reachabilityStatus];
-    }
 
-    if ([self.tunneledAppDelegate respondsToSelector:@selector(onInternetReachabilityChanged:)]) {
-        dispatch_sync(self->callbackQueue, ^{
-            [self.tunneledAppDelegate onInternetReachabilityChanged:networkStatus];
-        });
-    }
+        if ([self.tunneledAppDelegate respondsToSelector:@selector(onInternetReachabilityChanged:)]) {
+            dispatch_sync(self->callbackQueue, ^{
+                [self.tunneledAppDelegate onInternetReachabilityChanged:networkStatus];
+            });
+        }
 
-    previousNetworkStatus = atomic_exchange(&self->currentNetworkStatus, networkStatus);
+        previousNetworkStatus = atomic_exchange(&self->currentNetworkStatus, networkStatus);
 
-    // Restart if the network status or interface has changed, unless the previous status was
-    // NetworkReachabilityNotReachable, because the tunnel should be waiting for connectivity in
-    // that case.
-    if ((networkStatus != previousNetworkStatus || interfaceChanged) && previousNetworkStatus != NetworkReachabilityNotReachable) {
-        GoPsiReconnectTunnel();
+        // Restart if the network status or interface has changed, unless the previous status was
+        // NetworkReachabilityNotReachable, because the tunnel should be waiting for connectivity in
+        // that case.
+        if ((networkStatus != previousNetworkStatus || interfaceChanged) && previousNetworkStatus != NetworkReachabilityNotReachable) {
+            GoPsiReconnectTunnel();
+        }
     }
 }