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

1 files changed, 24 insertions, 26 deletions

diff --git a/java/com/android/server/ethernet/EthernetNetworkFactory.java b/java/com/android/server/ethernet/EthernetNetworkFactory.java
index 4f15355..9c3148b 100644
--- a/java/com/android/server/ethernet/EthernetNetworkFactory.java
+++ b/java/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