From dece0d805fe0de736a410a11e4866d1150bed98e Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 15 Nov 2021 18:17:33 +0900 Subject: [PATCH] S- not to crash on NetworkInfo(null) or setDetailedState(null) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 3 ++ framework/jarjar-rules.txt | 1 + framework/src/android/net/NetworkInfo.java | 10 ++++- .../src/android/net/cts/NetworkInfoTest.kt | 43 ++++++++++++++++--- 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/framework/Android.bp b/framework/Android.bp index 5e7262a945..d31f74fe0d 100644 --- a/framework/Android.bp +++ b/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", ], diff --git a/framework/jarjar-rules.txt b/framework/jarjar-rules.txt index 2e5848cb10..ae6b9c3337 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 433933f841..b7ec519d2c 100644 --- a/framework/src/android/net/NetworkInfo.java +++ b/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); } } diff --git a/tests/cts/net/src/android/net/cts/NetworkInfoTest.kt b/tests/cts/net/src/android/net/cts/NetworkInfoTest.kt index fa15e8f82c..d6120f8769 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 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 { 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 { 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 { + NetworkInfo(null) + } + assertFailsWith { + 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