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
diff --git a/framework/Android.bp b/framework/Android.bp
index 5e7262a..d31f74f 100644
--- a/framework/Android.bp
+++ b/framework/Android.bp
@@ -80,6 +80,9 @@
"framework-wifi.stubs.module_lib",
"net-utils-device-common",
],
+ static_libs: [
+ "modules-utils-build",
+ ],
libs: [
"unsupportedappusage",
],
diff --git a/framework/jarjar-rules.txt b/framework/jarjar-rules.txt
index 2e5848c..ae6b9c3 100644
--- a/framework/jarjar-rules.txt
+++ b/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
diff --git a/framework/src/android/net/NetworkInfo.java b/framework/src/android/net/NetworkInfo.java
index 433933f..b7ec519 100644
--- a/framework/src/android/net/NetworkInfo.java
+++ b/framework/src/android/net/NetworkInfo.java
@@ -24,6 +24,7 @@
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 @@
/** {@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 @@
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);
}
}
diff --git a/tests/cts/net/src/android/net/cts/NetworkInfoTest.kt b/tests/cts/net/src/android/net/cts/NetworkInfoTest.kt
index fa15e8f..d6120f8 100644
--- a/tests/cts/net/src/android/net/cts/NetworkInfoTest.kt
+++ b/tests/cts/net/src/android/net/cts/NetworkInfoTest.kt
@@ -26,16 +26,19 @@
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 @@
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 @@
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")
+ }
}
-}
+}
\ No newline at end of file