Don't throw in FullScore#policyNameOf.
This code is correct on userdebug builds, but it is dangerous on
user builds because proguard might strip out the POLICY_*
constants and that would lead to crashes.
For now just log a wtf if an invalid policy name is found. A
better solution would be to make MessageUtils robust to this
problem, e.g., by having it store the SparseArray internally and
providing getters that do not throw, instead of the current
behaviour that returns the SparseArray. That is left to a future
CL.
Fix: 227161413
Test: atest FullScoreTest
Change-Id: I68b69ee9dd84773018e62c9a8f43e754ae04c486
diff --git a/service/src/com/android/server/connectivity/FullScore.java b/service/src/com/android/server/connectivity/FullScore.java
index 799f46b..b13ba93 100644
--- a/service/src/com/android/server/connectivity/FullScore.java
+++ b/service/src/com/android/server/connectivity/FullScore.java
@@ -29,6 +29,7 @@
import android.net.NetworkCapabilities;
import android.net.NetworkScore;
import android.net.NetworkScore.KeepConnectedReason;
+import android.util.Log;
import android.util.SparseArray;
import com.android.internal.annotations.VisibleForTesting;
@@ -46,6 +47,8 @@
* they are handling a score that had the CS-managed bits set.
*/
public class FullScore {
+ private static final String TAG = FullScore.class.getSimpleName();
+
// This will be removed soon. Do *NOT* depend on it for any new code that is not part of
// a migration.
private final int mLegacyInt;
@@ -126,7 +129,15 @@
@VisibleForTesting
static @NonNull String policyNameOf(final int policy) {
final String name = sMessageNames.get(policy);
- if (name == null) throw new IllegalArgumentException("Unknown policy: " + policy);
+ if (name == null) {
+ // Don't throw here because name might be null due to proguard stripping out the
+ // POLICY_* constants, potentially causing a crash only on user builds because proguard
+ // does not run on userdebug builds.
+ // TODO: make MessageUtils safer by not returning the array and instead storing it
+ // internally and providing a getter (that does not throw) for individual values.
+ Log.wtf(TAG, "Unknown policy: " + policy);
+ return Integer.toString(policy);
+ }
return name.substring("POLICY_".length());
}
diff --git a/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt b/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt
index e7f6245..c03a9cd 100644
--- a/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt
+++ b/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt
@@ -22,6 +22,7 @@
import android.os.Build
import android.text.TextUtils
import android.util.ArraySet
+import android.util.Log
import androidx.test.filters.SmallTest
import com.android.server.connectivity.FullScore.MAX_CS_MANAGED_POLICY
import com.android.server.connectivity.FullScore.POLICY_ACCEPT_UNVALIDATED
@@ -32,11 +33,12 @@
import com.android.server.connectivity.FullScore.POLICY_IS_VPN
import com.android.testutils.DevSdkIgnoreRule
import com.android.testutils.DevSdkIgnoreRunner
+import org.junit.After
+import org.junit.Before
import org.junit.Test
import org.junit.runner.RunWith
import kotlin.reflect.full.staticProperties
import kotlin.test.assertEquals
-import kotlin.test.assertFailsWith
import kotlin.test.assertFalse
import kotlin.test.assertTrue
@@ -63,6 +65,23 @@
return mixInScore(nc, nac, validated, false /* yieldToBadWifi */, destroyed)
}
+ private val TAG = this::class.simpleName
+
+ private var wtfHandler: Log.TerribleFailureHandler? = null
+
+ @Before
+ fun setUp() {
+ // policyNameOf will call Log.wtf if passed an invalid policy.
+ wtfHandler = Log.setWtfHandler() { tagString, what, system ->
+ Log.d(TAG, "WTF captured, ignoring: $tagString $what")
+ }
+ }
+
+ @After
+ fun tearDown() {
+ Log.setWtfHandler(wtfHandler)
+ }
+
@Test
fun testGetLegacyInt() {
val ns = FullScore(50, 0L /* policy */, KEEP_CONNECTED_NONE)
@@ -101,10 +120,9 @@
assertFalse(foundNames.contains(name))
foundNames.add(name)
}
- assertFailsWith<IllegalArgumentException> {
- FullScore.policyNameOf(MAX_CS_MANAGED_POLICY + 1)
- }
assertEquals("IS_UNMETERED", FullScore.policyNameOf(POLICY_IS_UNMETERED))
+ val invalidPolicy = MAX_CS_MANAGED_POLICY + 1
+ assertEquals(Integer.toString(invalidPolicy), FullScore.policyNameOf(invalidPolicy))
}
fun getAllPolicies() = Regex("POLICY_.*").let { nameRegex ->