S- not to crash on NetworkInfo(null) or setDetailedState(null) am: dece0d805f

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/1892101

Change-Id: Idfb993a40d43a4ead862c229c93b8cb680b7bc1a

framework/Android.bp

@@ -80,6 +80,9 @@ java_sdk_library {
         "framework-wifi.stubs.module_lib",
         "net-utils-device-common",
     ],
+    static_libs: [
+        "modules-utils-build",
+    ],
     libs: [
         "unsupportedappusage",
     ],

framework/jarjar-rules.txt

@@ -1,2 +1,3 @@
 rule com.android.net.module.util.** android.net.connectivity.framework.util.@1
+rule com.android.modules.utils.** android.net.connectivity.framework.modules.utils.@1
 rule android.net.NetworkFactory* android.net.connectivity.framework.NetworkFactory@1

framework/src/android/net/NetworkInfo.java

@@ -24,6 +24,7 @@ import android.os.Parcelable;
 import android.text.TextUtils;
 
 import com.android.internal.annotations.VisibleForTesting;
+import com.android.modules.utils.build.SdkLevel;
 
 import java.util.EnumMap;
 
@@ -180,6 +181,10 @@ public class NetworkInfo implements Parcelable {
     /** {@hide} */
     @UnsupportedAppUsage
     public NetworkInfo(@NonNull NetworkInfo source) {
+        // S- didn't use to crash when passing null. This plants a timebomb where mState and
+        // some other fields are null, but there may be existing code that relies on this behavior
+        // and doesn't trip the timebomb, so on SdkLevel < T, keep the old behavior. b/145972387
+        if (null == source && !SdkLevel.isAtLeastT()) return;
         synchronized (source) {
             mNetworkType = source.mNetworkType;
             mSubtype = source.mSubtype;
@@ -490,8 +495,9 @@ public class NetworkInfo implements Parcelable {
             this.mReason = reason;
             this.mExtraInfo = extraInfo;
             // Catch both the case where detailedState is null and the case where it's some
-            // unknown value
-            if (null == mState) {
+            // unknown value. This is clearly incorrect usage, but S- didn't use to crash (at
+            // least immediately) so keep the old behavior on older frameworks for safety.
+            if (null == mState && SdkLevel.isAtLeastT()) {
                 throw new NullPointerException("Unknown DetailedState : " + detailedState);
             }
         }

tests/cts/net/src/android/net/cts/NetworkInfoTest.kt

@@ -26,16 +26,19 @@ import android.telephony.TelephonyManager
 import androidx.test.filters.SmallTest
 import androidx.test.platform.app.InstrumentationRegistry
 import androidx.test.runner.AndroidJUnit4
+import com.android.modules.utils.build.SdkLevel
 import com.android.testutils.DevSdkIgnoreRule
 import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
-import org.junit.Assert.fail
 import org.junit.Rule
 import org.junit.runner.RunWith
 import org.junit.Test
+import kotlin.reflect.jvm.isAccessible
+import kotlin.test.assertFails
+import kotlin.test.assertFailsWith
 
 const val TYPE_MOBILE = ConnectivityManager.TYPE_MOBILE
 const val TYPE_WIFI = ConnectivityManager.TYPE_WIFI
@@ -97,12 +100,16 @@ class NetworkInfoTest {
         assertNull(networkInfo.reason)
         assertNull(networkInfo.extraInfo)
 
-        try {
+        assertFailsWith<IllegalArgumentException> {
             NetworkInfo(ConnectivityManager.MAX_NETWORK_TYPE + 1,
                     TelephonyManager.NETWORK_TYPE_LTE, MOBILE_TYPE_NAME, LTE_SUBTYPE_NAME)
-            fail("Unexpected behavior. Network type is invalid.")
-        } catch (e: IllegalArgumentException) {
-            // Expected behavior.
+        }
+
+        if (SdkLevel.isAtLeastT()) {
+            assertFailsWith<NullPointerException> { NetworkInfo(null) }
+        } else {
+            // Doesn't immediately crash on S-
+            NetworkInfo(null)
         }
     }
 
@@ -118,5 +125,29 @@ class NetworkInfoTest {
         assertEquals(State.CONNECTED, networkInfo.state)
         assertEquals(reason, networkInfo.reason)
         assertEquals(extraReason, networkInfo.extraInfo)
+
+        // Create an incorrect enum value by calling the default constructor of the enum
+        val constructor = DetailedState::class.java.declaredConstructors.first {
+            it.parameters.size == 2
+        }
+        constructor.isAccessible = true
+        val incorrectDetailedState = constructor.newInstance("any", 200) as DetailedState
+        if (SdkLevel.isAtLeastT()) {
+            assertFailsWith<NullPointerException> {
+                NetworkInfo(null)
+            }
+            assertFailsWith<NullPointerException> {
+                networkInfo.setDetailedState(null, "reason", "extraInfo")
+            }
+            // This actually throws ArrayOutOfBoundsException because of the implementation of
+            // EnumMap, but that's an implementation detail so accept any crash.
+            assertFails {
+                networkInfo.setDetailedState(incorrectDetailedState, "reason", "extraInfo")
+            }
+        } else {
+            // Doesn't immediately crash on S-
+            NetworkInfo(null)
+            networkInfo.setDetailedState(null, "reason", "extraInfo")
+        }
     }
-}
+}