From 1727d9f1f25ffad24912d02fbddcacd1f7bac16a Mon Sep 17 00:00:00 2001
From: Lorenzo Colitti <lorenzo@google.com>
Date: Thu, 24 Mar 2022 20:53:24 +0900
Subject: [PATCH] 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
---
 .../server/connectivity/FullScore.java        | 13 +++++++++-
 .../server/connectivity/FullScoreTest.kt      | 26 ++++++++++++++++---
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/service/src/com/android/server/connectivity/FullScore.java b/service/src/com/android/server/connectivity/FullScore.java
index 799f46b524..b13ba9328a 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.NetworkAgentConfig;
 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 @@ import java.util.StringJoiner;
  * 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 @@ public class FullScore {
     @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 e7f6245547..c03a9cde70 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.net.NetworkScore.KEEP_CONNECTED_NONE
 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_VALIDATED
 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 @@ class FullScoreTest {
         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 @@ class FullScoreTest {
             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 ->