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

When NetworkInfo(null) or setDetailedState(null, any, any) are
called, S used to not crash but plant a null bomb for later
which may explode in some calls (notably, parceling) : see the
bug referenced below for details.

To help catching these errors earlier a patch was made to crash
as soon as one of these methods is called with a null argument,
but this will also crash incorrect use on existing code that
may never actually step on the mine, crashing code that used not
to crash. For safety, implement the new behavior only on T.

Bug: 145972387
Test: NetworkInfoTest
Change-Id: Ib710497d83b2d26439c2bd4d2f572310db97d6fd

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")
+        }
     }
-}
+}