Fixing multithreading issues in Ethernet Factory

IP client callbacks could be executed updating the state of an
ethernet interface even if they were no longer the callbacks for the
currently active interface. This can happen as IP client callbacks
were being called from a thread separate from ethernet.

Bug: 224890356
Test: atest EthernetServiceTests
atest CtsNetTestCasesLatestSdk
:android.net.cts.EthernetManagerTest --iterations 30

Change-Id: I238cb75caa01472bccc79db5bafa82bccdaeba52

service-t/src/com/android/server/ethernet/EthernetNetworkFactory.java

@@ -439,24 +439,44 @@ public class EthernetNetworkFactory extends NetworkFactory {
                 mIpClientShutdownCv.block();
             }
 
+            // At the time IpClient is stopped, an IpClient event may have already been posted on
+            // the back of the handler and is awaiting execution. Once that event is executed, the
+            // associated callback object may not be valid anymore
+            // (NetworkInterfaceState#mIpClientCallback points to a different object / null).
+            private boolean isCurrentCallback() {
+                return this == mIpClientCallback;
+            }
+
+            private void handleIpEvent(final @NonNull Runnable r) {
+                mHandler.post(() -> {
+                    if (!isCurrentCallback()) {
+                        Log.i(TAG, "Ignoring stale IpClientCallbacks " + this);
+                        return;
+                    }
+                    r.run();
+                });
+            }
+
             @Override
             public void onProvisioningSuccess(LinkProperties newLp) {
-                mHandler.post(() -> onIpLayerStarted(newLp, mNetworkManagementListener));
+                handleIpEvent(() -> onIpLayerStarted(newLp, mNetworkManagementListener));
             }
 
             @Override
             public void onProvisioningFailure(LinkProperties newLp) {
-                mHandler.post(() -> onIpLayerStopped(mNetworkManagementListener));
+                // This cannot happen due to provisioning timeout, because our timeout is 0. It can
+                // happen due to errors while provisioning or on provisioning loss.
+                handleIpEvent(() -> onIpLayerStopped(mNetworkManagementListener));
             }
 
             @Override
             public void onLinkPropertiesChange(LinkProperties newLp) {
-                mHandler.post(() -> updateLinkProperties(newLp));
+                handleIpEvent(() -> updateLinkProperties(newLp));
             }
 
             @Override
             public void onReachabilityLost(String logMsg) {
-                mHandler.post(() -> updateNeighborLostEvent(logMsg));
+                handleIpEvent(() -> updateNeighborLostEvent(logMsg));
             }
 
             @Override
@@ -558,14 +578,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
 
         void onIpLayerStarted(@NonNull final LinkProperties linkProperties,
                 @Nullable final INetworkInterfaceOutcomeReceiver listener) {
-            if(mIpClient == null) {
-                // This call comes from a message posted on the handler thread, but the IpClient has
-                // since been stopped such as may be the case if updateInterfaceLinkState() is
-                // queued on the handler thread prior. As network management callbacks are sent in
-                // stop(), there is no need to send them again here.
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             if (mNetworkAgent != null) {
                 Log.e(TAG, "Already have a NetworkAgent - aborting new request");
                 stop();
@@ -601,12 +613,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
         }
 
         void onIpLayerStopped(@Nullable final INetworkInterfaceOutcomeReceiver listener) {
-            // This cannot happen due to provisioning timeout, because our timeout is 0. It can
-            // happen due to errors while provisioning or on provisioning loss.
-            if(mIpClient == null) {
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             // There is no point in continuing if the interface is gone as stop() will be triggered
             // by removeInterface() when processed on the handler thread and start() won't
             // work for a non-existent interface.
@@ -648,10 +654,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
         }
 
         void updateLinkProperties(LinkProperties linkProperties) {
-            if(mIpClient == null) {
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             mLinkProperties = linkProperties;
             if (mNetworkAgent != null) {
                 mNetworkAgent.sendLinkPropertiesImpl(linkProperties);
@@ -659,10 +661,6 @@ public class EthernetNetworkFactory extends NetworkFactory {
         }
 
         void updateNeighborLostEvent(String logMsg) {
-            if(mIpClient == null) {
-                if (DBG) Log.d(TAG, "IpClient is not initialized.");
-                return;
-            }
             Log.i(TAG, "updateNeighborLostEvent " + logMsg);
             // Reachability lost will be seen only if the gateway is not reachable.
             // Since ethernet FW doesn't have the mechanism to scan for new networks

tests/ethernet/java/com/android/server/ethernet/EthernetNetworkFactoryTest.java

@@ -21,6 +21,7 @@ import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
@@ -543,68 +544,59 @@ public class EthernetNetworkFactoryTest {
         verifyRestart(createDefaultIpConfig());
     }
 
-    @Test
+    private IpClientCallbacks getStaleIpClientCallbacks() throws Exception {
-    public void testIgnoreOnIpLayerStartedCallbackAfterIpClientHasStopped() throws Exception {
-        initEthernetNetworkFactory();
         createAndVerifyProvisionedInterface(TEST_IFACE);
-        mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
+        final IpClientCallbacks staleIpClientCallbacks = mIpClientCallbacks;
-        mIpClientCallbacks.onProvisioningSuccess(new LinkProperties());
+        mNetFactory.removeInterface(TEST_IFACE);
-        mLooper.dispatchAll();
         verifyStop();
+        assertNotSame(mIpClientCallbacks, staleIpClientCallbacks);
+        return staleIpClientCallbacks;
+    }
+
+    @Test
+    public void testIgnoreOnIpLayerStartedCallbackForStaleCallback() throws Exception {
+        initEthernetNetworkFactory();
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
+
+        staleIpClientCallbacks.onProvisioningSuccess(new LinkProperties());
+        mLooper.dispatchAll();
 
-        // ipClient has been shut down first, we should not retry
         verify(mIpClient, never()).startProvisioning(any());
         verify(mNetworkAgent, never()).register();
     }
 
     @Test
-    public void testIgnoreOnIpLayerStoppedCallbackAfterIpClientHasStopped() throws Exception {
+    public void testIgnoreOnIpLayerStoppedCallbackForStaleCallback() throws Exception {
         initEthernetNetworkFactory();
-        createAndVerifyProvisionedInterface(TEST_IFACE);
         when(mDeps.getNetworkInterfaceByName(TEST_IFACE)).thenReturn(mInterfaceParams);
-        mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
-        mIpClientCallbacks.onProvisioningFailure(new LinkProperties());
-        mLooper.dispatchAll();
-        verifyStop();
 
-        // ipClient has been shut down first, we should not retry
+        staleIpClientCallbacks.onProvisioningFailure(new LinkProperties());
-        verify(mIpClient).startProvisioning(any());
+        mLooper.dispatchAll();
+
+        verify(mIpClient, never()).startProvisioning(any());
     }
 
     @Test
-    public void testIgnoreLinkPropertiesCallbackAfterIpClientHasStopped() throws Exception {
+    public void testIgnoreLinkPropertiesCallbackForStaleCallback() throws Exception {
         initEthernetNetworkFactory();
-        createAndVerifyProvisionedInterface(TEST_IFACE);
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
-        LinkProperties lp = new LinkProperties();
+        final LinkProperties lp = new LinkProperties();
 
-        // The test requires the two proceeding methods to happen one after the other in ENF and
+        staleIpClientCallbacks.onLinkPropertiesChange(lp);
-        // verifies onLinkPropertiesChange doesn't complete execution for a downed interface.
-        // Posting is necessary as updateInterfaceLinkState with false will set mIpClientCallbacks
-        // to null which will throw an NPE in the test if executed synchronously.
-        mHandler.post(() -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER));
-        mIpClientCallbacks.onLinkPropertiesChange(lp);
         mLooper.dispatchAll();
 
-        verifyStop();
+        verify(mNetworkAgent, never()).sendLinkPropertiesImpl(eq(lp));
-        // ipClient has been shut down first, we should not update
-        verify(mNetworkAgent, never()).sendLinkPropertiesImpl(same(lp));
     }
 
     @Test
-    public void testIgnoreNeighborLossCallbackAfterIpClientHasStopped() throws Exception {
+    public void testIgnoreNeighborLossCallbackForStaleCallback() throws Exception {
         initEthernetNetworkFactory();
-        createAndVerifyProvisionedInterface(TEST_IFACE);
+        final IpClientCallbacks staleIpClientCallbacks = getStaleIpClientCallbacks();
 
-        // The test requires the two proceeding methods to happen one after the other in ENF and
+        staleIpClientCallbacks.onReachabilityLost("Neighbor Lost");
-        // verifies onReachabilityLost doesn't complete execution for a downed interface.
-        // Posting is necessary as updateInterfaceLinkState with false will set mIpClientCallbacks
-        // to null which will throw an NPE in the test if executed synchronously.
-        mHandler.post(() -> mNetFactory.updateInterfaceLinkState(TEST_IFACE, false, NULL_LISTENER));
-        mIpClientCallbacks.onReachabilityLost("Neighbor Lost");
         mLooper.dispatchAll();
 
-        verifyStop();
-        // ipClient has been shut down first, we should not update
         verify(mIpClient, never()).startProvisioning(any());
         verify(mNetworkAgent, never()).register();
     }