Read ARP timing properties from DeviceConfig appropriately.

Overload NetworkStackUtils.getDeviceConfigPropertyInt API to check if the
value of property read from DeviceConfig is valid, value in the range
of mininum and maximum would be acceptable, otherwise, return the default
value.

Bug: 128639898
Test: atest NetworkStackTests NetworkStackIntegrationTests

Change-Id: I07e3e9d1e1b7252d852d4665d2ea254f29a1c3eb

4 files changed, 130 insertions, 67 deletions

diff --git a/src/android/net/dhcp/DhcpClient.java b/src/android/net/dhcp/DhcpClient.java
index 982d8ce..662a640 100644
--- a/src/android/net/dhcp/DhcpClient.java
+++ b/src/android/net/dhcp/DhcpClient.java
@@ -154,34 +154,40 @@ public class DhcpClient extends StateMachine {
     // seconds to avoid excessive traffic, but it's too long).
     @VisibleForTesting
     public static final String DHCP_RESTART_CONFIG_DELAY = "dhcp_restart_configuration_delay";
-    private static final int DEFAULT_DHCP_RESTART_CONFIG_DELAY_SEC = 1 * SECONDS;
+    private static final int DEFAULT_DHCP_RESTART_CONFIG_DELAY_MS = 1 * SECONDS;
+    private static final int MAX_DHCP_CLIENT_RESTART_CONFIG_DELAY_MS = 10 * SECONDS;
 
     // Initial random delay before sending first ARP probe.
     @VisibleForTesting
     public static final String ARP_FIRST_PROBE_DELAY_MS = "arp_first_probe_delay";
     private static final int DEFAULT_ARP_FIRST_PROBE_DELAY_MS = 100;
+    private static final int MAX_ARP_FIRST_PROBE_DELAY_MS = 1 * SECONDS;
 
     // Minimum delay until retransmitting the probe. The probe will be retransmitted after a
     // random number of milliseconds in the range ARP_PROBE_MIN_MS and ARP_PROBE_MAX_MS.
     @VisibleForTesting
     public static final String ARP_PROBE_MIN_MS = "arp_probe_min";
     private static final int DEFAULT_ARP_PROBE_MIN_MS = 100;
+    private static final int MAX_ARP_PROBE_MIN_MS = 1 * SECONDS;
 
     // Maximum delay until retransmitting the probe.
     @VisibleForTesting
     public static final String ARP_PROBE_MAX_MS = "arp_probe_max";
     private static final int DEFAULT_ARP_PROBE_MAX_MS = 300;
+    private static final int MAX_ARP_PROBE_MAX_MS = 2 * SECONDS;
 
     // Initial random delay before sending first ARP Announcement after completing Probe packet
     // transmission.
     @VisibleForTesting
     public static final String ARP_FIRST_ANNOUNCE_DELAY_MS = "arp_first_announce_delay";
     private static final int DEFAULT_ARP_FIRST_ANNOUNCE_DELAY_MS = 100;
+    private static final int MAX_ARP_FIRST_ANNOUNCE_DELAY_MS = 2 * SECONDS;
 
     // Time between retransmitting ARP Announcement packets.
     @VisibleForTesting
     public static final String ARP_ANNOUNCE_INTERVAL_MS = "arp_announce_interval";
     private static final int DEFAULT_ARP_ANNOUNCE_INTERVAL_MS = 100;
+    private static final int MAX_ARP_ANNOUNCE_INTERVAL_MS = 2 * SECONDS;
 
     // Max conflict count before configuring interface with declined IP address anyway.
     private static final int MAX_CONFLICTS_COUNT = 2;
@@ -367,33 +373,10 @@ public class DhcpClient extends StateMachine {
         /**
          * Get the Integer value of relevant DeviceConfig properties of Connectivity namespace.
          */
-        public int getIntDeviceConfig(final String name) {
-            final int defaultValue;
-            switch (name) {
-                case DHCP_RESTART_CONFIG_DELAY:
-                    defaultValue = DEFAULT_DHCP_RESTART_CONFIG_DELAY_SEC;
-                    break;
-                case ARP_FIRST_PROBE_DELAY_MS:
-                    defaultValue = DEFAULT_ARP_FIRST_PROBE_DELAY_MS;
-                    break;
-                case ARP_PROBE_MIN_MS:
-                    defaultValue = DEFAULT_ARP_PROBE_MIN_MS;
-                    break;
-                case ARP_PROBE_MAX_MS:
-                    defaultValue = DEFAULT_ARP_PROBE_MAX_MS;
-                    break;
-                case ARP_FIRST_ANNOUNCE_DELAY_MS:
-                    defaultValue = DEFAULT_ARP_FIRST_ANNOUNCE_DELAY_MS;
-                    break;
-                case ARP_ANNOUNCE_INTERVAL_MS:
-                    defaultValue = DEFAULT_ARP_ANNOUNCE_INTERVAL_MS;
-                    break;
-                default:
-                    Log.e(TAG, "Invalid experiment flag: " + name);
-                    return 0;
-            }
-            return NetworkStackUtils.getDeviceConfigPropertyInt(NAMESPACE_CONNECTIVITY, name,
-                    defaultValue);
+        public int getIntDeviceConfig(final String name, int minimumValue, int maximumValue,
+                int defaultValue) {
+            return NetworkStackUtils.getDeviceConfigPropertyInt(NAMESPACE_CONNECTIVITY,
+                    name, minimumValue, maximumValue, defaultValue);
         }
 
         /**
@@ -1429,32 +1412,30 @@ public class DhcpClient extends StateMachine {
     //    address being probed for, and the packet's 'sender hardware address' is not the
     //    hardware address of any of the host's interfaces, then the host SHOULD similarly
     //    treat this as an address conflict.
-    private boolean checkArpSenderIpOrTargetIp(@NonNull ArpPacket packet,
+    private boolean packetHasIpAddressConflict(@NonNull ArpPacket packet,
             @NonNull Inet4Address targetIp) {
-        return ((!packet.senderIp.equals(INADDR_ANY) && packet.senderIp.equals(targetIp))
-                || (isArpProbe(packet) && packet.targetIp.equals(targetIp)));
+        return (((!packet.senderIp.equals(INADDR_ANY) && packet.senderIp.equals(targetIp))
+                || (isArpProbe(packet) && packet.targetIp.equals(targetIp)))
+                && !Arrays.equals(packet.senderHwAddress.toByteArray(), mHwAddr));
     }
 
     private boolean hasIpAddressConflict(@NonNull ArpPacket packet,
             @NonNull Inet4Address targetIp) {
-        if (checkArpSenderIpOrTargetIp(packet, targetIp)
-                && !Arrays.equals(packet.senderHwAddress.toByteArray(), mHwAddr)) {
-            if (DBG) {
-                final String senderIpString = packet.senderIp.getHostAddress();
-                final String targetIpString = packet.targetIp.getHostAddress();
-                final MacAddress senderMacAddress = packet.senderHwAddress;
-                final MacAddress hostMacAddress = MacAddress.fromBytes(mHwAddr);
-                Log.d(TAG, "IP address conflict detected:"
-                        + (packet.opCode == ARP_REQUEST ? "ARP Request" : "ARP Reply")
-                        + " ARP sender MAC: " + senderMacAddress.toString()
-                        + " host MAC: "       + hostMacAddress.toString()
-                        + " ARP sender IP: "  + senderIpString
-                        + " ARP target IP: "  + targetIpString
-                        + " host target IP: " + targetIp.getHostAddress());
-            }
-            return true;
+        if (!packetHasIpAddressConflict(packet, targetIp)) return false;
+        if (DBG) {
+            final String senderIpString = packet.senderIp.getHostAddress();
+            final String targetIpString = packet.targetIp.getHostAddress();
+            final MacAddress senderMacAddress = packet.senderHwAddress;
+            final MacAddress hostMacAddress = MacAddress.fromBytes(mHwAddr);
+            Log.d(TAG, "IP address conflict detected:"
+                    + (packet.opCode == ARP_REQUEST ? "ARP Request" : "ARP Reply")
+                    + " ARP sender MAC: " + senderMacAddress.toString()
+                    + " host MAC: "       + hostMacAddress.toString()
+                    + " ARP sender IP: "  + senderIpString
+                    + " ARP target IP: "  + targetIpString
+                    + " host target IP: " + targetIp.getHostAddress());
         }
-        return false;
+        return true;
     }
 
     class IpAddressConflictDetectingState extends LoggingState {
@@ -1492,13 +1473,7 @@ public class DhcpClient extends StateMachine {
             }
 
             // Read the customized parameters from DeviceConfig.
-            mArpFirstProbeDelayMs = mDependencies.getIntDeviceConfig(ARP_FIRST_PROBE_DELAY_MS);
-            mArpProbeMaxDelayMs = mDependencies.getIntDeviceConfig(ARP_PROBE_MAX_MS);
-            mArpProbeMinDelayMs = mDependencies.getIntDeviceConfig(ARP_PROBE_MIN_MS);
-            mArpAnnounceIntervalMs = mDependencies.getIntDeviceConfig(ARP_ANNOUNCE_INTERVAL_MS);
-            mArpFirstAnnounceDelayMs = mDependencies.getIntDeviceConfig(
-                    ARP_FIRST_ANNOUNCE_DELAY_MS);
-
+            readIpConflictParametersFromDeviceConfig();
             if (VDBG) {
                 Log.d(TAG, "ARP First Probe delay: "    + mArpFirstProbeDelayMs
                         + " ARP Probe Max delay: "      + mArpProbeMaxDelayMs
@@ -1577,6 +1552,33 @@ public class DhcpClient extends StateMachine {
             removeMessages(EVENT_IP_CONFLICT);
         }
 
+        // The following timing parameters are defined in RFC5227 as fixed constants, which
+        // are too long to adopt in the mobile network scenario, however more appropriate to
+        // reference these fixed value as maximumValue argument to restrict the upper bound,
+        // the minimum values of 10/20ms are used to avoid tight loops due to misconfiguration.
+        private void readIpConflictParametersFromDeviceConfig() {
+            // PROBE_WAIT
+            mArpFirstProbeDelayMs = mDependencies.getIntDeviceConfig(ARP_FIRST_PROBE_DELAY_MS,
+                    10, MAX_ARP_FIRST_PROBE_DELAY_MS, DEFAULT_ARP_FIRST_PROBE_DELAY_MS);
+
+            // PROBE_MIN
+            mArpProbeMinDelayMs = mDependencies.getIntDeviceConfig(ARP_PROBE_MIN_MS, 10,
+                    MAX_ARP_PROBE_MIN_MS, DEFAULT_ARP_PROBE_MIN_MS);
+
+            // PROBE_MAX
+            mArpProbeMaxDelayMs = Math.max(mArpProbeMinDelayMs + 1,
+                    mDependencies.getIntDeviceConfig(ARP_PROBE_MAX_MS, 20, MAX_ARP_PROBE_MAX_MS,
+                    DEFAULT_ARP_PROBE_MAX_MS));
+
+            // ANNOUNCE_WAIT
+            mArpFirstAnnounceDelayMs = mDependencies.getIntDeviceConfig(ARP_FIRST_ANNOUNCE_DELAY_MS,
+                    20, MAX_ARP_FIRST_ANNOUNCE_DELAY_MS, DEFAULT_ARP_FIRST_ANNOUNCE_DELAY_MS);
+
+            // ANNOUNCE_INTERVAL
+            mArpAnnounceIntervalMs = mDependencies.getIntDeviceConfig(ARP_ANNOUNCE_INTERVAL_MS, 20,
+                    MAX_ARP_ANNOUNCE_INTERVAL_MS, DEFAULT_ARP_ANNOUNCE_INTERVAL_MS);
+        }
+
         private boolean sendArpProbe() {
             return mIpConflictDetector.transmitPacket(mTargetIp /* target IP */,
                     INADDR_ANY /* sender IP */, mHwAddr, mInterfaceBroadcastAddr);
@@ -1781,7 +1783,8 @@ public class DhcpClient extends StateMachine {
                 return;
             }
 
-            mTimeout = mDependencies.getIntDeviceConfig(DHCP_RESTART_CONFIG_DELAY);
+            mTimeout = mDependencies.getIntDeviceConfig(DHCP_RESTART_CONFIG_DELAY, 100,
+                    MAX_DHCP_CLIENT_RESTART_CONFIG_DELAY_MS, DEFAULT_DHCP_RESTART_CONFIG_DELAY_MS);
             super.enter();
             sendPacket();
         }
diff --git a/src/android/net/util/NetworkStackUtils.java b/src/android/net/util/NetworkStackUtils.java
index 45ed367..d147b45 100644
--- a/src/android/net/util/NetworkStackUtils.java
+++ b/src/android/net/util/NetworkStackUtils.java
@@ -232,6 +232,23 @@ public class NetworkStackUtils {
      * Look up the value of a property for a particular namespace from {@link DeviceConfig}.
      * @param namespace The namespace containing the property to look up.
      * @param name The name of the property to look up.
+     * @param minimumValue The minimum value of a property.
+     * @param maximumValue The maximum value of a property.
+     * @param defaultValue The value to return if the property does not exist or its value is null.
+     * @return the corresponding value, or defaultValue if none exists or the fetched value is
+     *         greater than maximumValue.
+     */
+    public static int getDeviceConfigPropertyInt(@NonNull String namespace, @NonNull String name,
+            int minimumValue, int maximumValue, int defaultValue) {
+        int value = getDeviceConfigPropertyInt(namespace, name, defaultValue);
+        if (value < minimumValue || value > maximumValue) return defaultValue;
+        return value;
+    }
+
+    /**
+     * Look up the value of a property for a particular namespace from {@link DeviceConfig}.
+     * @param namespace The namespace containing the property to look up.
+     * @param name The name of the property to look up.
      * @param defaultValue The value to return if the property does not exist or its value is null.
      * @return the corresponding value, or defaultValue if none exists.
      */
diff --git a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
index 2ec2efa..8679a9d 100644
--- a/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
+++ b/tests/integration/src/android/net/ip/IpClientIntegrationTest.java
@@ -311,7 +311,8 @@ public class IpClientIntegrationTest {
                 }
 
                 @Override
-                public int getIntDeviceConfig(final String name) {
+                public int getIntDeviceConfig(final String name, int minimumValue,
+                        int maximumValue, int defaultValue) {
                     return getDeviceConfigPropertyInt(name, 0 /* default value */);
                 }
 
diff --git a/tests/unit/src/android/net/util/NetworkStackUtilsTest.java b/tests/unit/src/android/net/util/NetworkStackUtilsTest.java
index 806054d..9dd91ab 100644
--- a/tests/unit/src/android/net/util/NetworkStackUtilsTest.java
+++ b/tests/unit/src/android/net/util/NetworkStackUtilsTest.java
@@ -54,9 +54,11 @@ import org.mockito.MockitoSession;
 public class NetworkStackUtilsTest {
     private static final String TEST_NAME_SPACE = "connectivity";
     private static final String TEST_EXPERIMENT_FLAG = "experiment_flag";
-    private static final int TEST_FLAG_VERSION = 28;
-    private static final String TEST_FLAG_VERSION_STRING = "28";
-    private static final int TEST_DEFAULT_FLAG_VERSION = 0;
+    private static final int TEST_FLAG_VALUE = 28;
+    private static final String TEST_FLAG_VALUE_STRING = "28";
+    private static final int TEST_DEFAULT_FLAG_VALUE = 0;
+    private static final int TEST_MAX_FLAG_VALUE = 1000;
+    private static final int TEST_MIN_FLAG_VALUE = 100;
     private static final long TEST_PACKAGE_VERSION = 290000000;
     private static final String TEST_PACKAGE_NAME = "NetworkStackUtilsTest";
     private MockitoSession mSession;
@@ -87,18 +89,58 @@ public class NetworkStackUtilsTest {
     public void testGetDeviceConfigPropertyInt_Null() {
         doReturn(null).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
                 eq(TEST_EXPERIMENT_FLAG)));
-        assertEquals(TEST_DEFAULT_FLAG_VERSION, NetworkStackUtils.getDeviceConfigPropertyInt(
+        assertEquals(TEST_DEFAULT_FLAG_VALUE, NetworkStackUtils.getDeviceConfigPropertyInt(
                 TEST_NAME_SPACE, TEST_EXPERIMENT_FLAG,
-                TEST_DEFAULT_FLAG_VERSION /* default value */));
+                TEST_DEFAULT_FLAG_VALUE /* default value */));
     }
 
     @Test
     public void testGetDeviceConfigPropertyInt_NotNull() {
-        doReturn(TEST_FLAG_VERSION_STRING).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
+        doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
                 eq(TEST_EXPERIMENT_FLAG)));
-        assertEquals(TEST_FLAG_VERSION, NetworkStackUtils.getDeviceConfigPropertyInt(
+        assertEquals(TEST_FLAG_VALUE, NetworkStackUtils.getDeviceConfigPropertyInt(
                 TEST_NAME_SPACE, TEST_EXPERIMENT_FLAG,
-                TEST_DEFAULT_FLAG_VERSION /* default value */));
+                TEST_DEFAULT_FLAG_VALUE /* default value */));
+    }
+
+    @Test
+    public void testGetDeviceConfigPropertyInt_NormalValue() {
+        doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
+                eq(TEST_EXPERIMENT_FLAG)));
+        assertEquals(TEST_FLAG_VALUE, NetworkStackUtils.getDeviceConfigPropertyInt(
+                TEST_NAME_SPACE, TEST_EXPERIMENT_FLAG, 0 /* minimum value */,
+                TEST_MAX_FLAG_VALUE /* maximum value */,
+                TEST_DEFAULT_FLAG_VALUE /* default value */));
+    }
+
+    @Test
+    public void testGetDeviceConfigPropertyInt_NullValue() {
+        doReturn(null).when(() -> DeviceConfig.getProperty(
+                eq(TEST_NAME_SPACE), eq(TEST_EXPERIMENT_FLAG)));
+        assertEquals(TEST_DEFAULT_FLAG_VALUE, NetworkStackUtils.getDeviceConfigPropertyInt(
+                TEST_NAME_SPACE, TEST_EXPERIMENT_FLAG, 0 /* minimum value */,
+                TEST_MAX_FLAG_VALUE /* maximum value */,
+                TEST_DEFAULT_FLAG_VALUE /* default value */));
+    }
+
+    @Test
+    public void testGetDeviceConfigPropertyInt_OverMaximumValue() {
+        doReturn(Integer.toString(TEST_MAX_FLAG_VALUE + 10)).when(() -> DeviceConfig.getProperty(
+                eq(TEST_NAME_SPACE), eq(TEST_EXPERIMENT_FLAG)));
+        assertEquals(TEST_DEFAULT_FLAG_VALUE, NetworkStackUtils.getDeviceConfigPropertyInt(
+                TEST_NAME_SPACE, TEST_EXPERIMENT_FLAG, TEST_MIN_FLAG_VALUE /* minimum value */,
+                TEST_MAX_FLAG_VALUE /* maximum value */,
+                TEST_DEFAULT_FLAG_VALUE /* default value */));
+    }
+
+    @Test
+    public void testGetDeviceConfigPropertyInt_BelowMinimumValue() {
+        doReturn(Integer.toString(TEST_MIN_FLAG_VALUE - 10)).when(() -> DeviceConfig.getProperty(
+                eq(TEST_NAME_SPACE), eq(TEST_EXPERIMENT_FLAG)));
+        assertEquals(TEST_DEFAULT_FLAG_VALUE, NetworkStackUtils.getDeviceConfigPropertyInt(
+                TEST_NAME_SPACE, TEST_EXPERIMENT_FLAG, TEST_MIN_FLAG_VALUE /* minimum value */,
+                TEST_MAX_FLAG_VALUE /* maximum value */,
+                TEST_DEFAULT_FLAG_VALUE /* default value */));
     }
 
     @Test
@@ -121,7 +163,7 @@ public class NetworkStackUtilsTest {
 
     @Test
     public void testFeatureIsEnabledWithExceptionsEnabled() {
-        doReturn(TEST_FLAG_VERSION_STRING).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
+        doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
                 eq(TEST_EXPERIMENT_FLAG)));
         assertTrue(NetworkStackUtils.isFeatureEnabled(mContext, TEST_NAME_SPACE,
                 TEST_EXPERIMENT_FLAG));
@@ -137,7 +179,7 @@ public class NetworkStackUtilsTest {
 
     @Test
     public void testFeatureIsEnabledWithException() throws Exception {
-        doReturn(TEST_FLAG_VERSION_STRING).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
+        doReturn(TEST_FLAG_VALUE_STRING).when(() -> DeviceConfig.getProperty(eq(TEST_NAME_SPACE),
                 eq(TEST_EXPERIMENT_FLAG)));
         doThrow(NameNotFoundException.class).when(mPm).getPackageInfo(anyString(), anyInt());
         assertFalse(NetworkStackUtils.isFeatureEnabled(mContext, TEST_NAME_SPACE,