From ab1a0671c78c6e829bc102ff726ba0afd438a986 Mon Sep 17 00:00:00 2001 From: Motomu Utsumi Date: Fri, 31 May 2024 12:55:31 +0900 Subject: [PATCH] Stop throwing when the invalid capability is passed When new capability is introduced and added to capaility or forbiddenCapability by default, module rollback could cause system server crash since JobScheduler persists network request that contains new capability that is invalid for old code. This CL updates capability validation not to throw exception to avoid system crash in this case. Bug: 343823469 Bug: 313030307 Test: NetworkCapabilitiesTest Change-Id: I5708c475f3b4974284531212b77f9788ca9f497e --- .../src/android/net/NetworkCapabilities.java | 28 +++++++++------ .../android/net/NetworkCapabilitiesTest.java | 36 ++++++++++++++++--- 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/framework/src/android/net/NetworkCapabilities.java b/framework/src/android/net/NetworkCapabilities.java index 45efbfeea5..a5a6723b03 100644 --- a/framework/src/android/net/NetworkCapabilities.java +++ b/framework/src/android/net/NetworkCapabilities.java @@ -36,6 +36,7 @@ import android.os.Parcelable; import android.os.Process; import android.text.TextUtils; import android.util.ArraySet; +import android.util.Log; import android.util.Range; import com.android.internal.annotations.VisibleForTesting; @@ -843,7 +844,10 @@ public final class NetworkCapabilities implements Parcelable { // If the given capability was previously added to the list of forbidden capabilities // then the capability will also be removed from the list of forbidden capabilities. // TODO: Add forbidden capabilities to the public API - checkValidCapability(capability); + if (!isValidCapability(capability)) { + Log.e(TAG, "addCapability is called with invalid capability: " + capability); + return this; + } mNetworkCapabilities |= 1L << capability; // remove from forbidden capability list mForbiddenNetworkCapabilities &= ~(1L << capability); @@ -864,7 +868,10 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public void addForbiddenCapability(@NetCapability int capability) { - checkValidCapability(capability); + if (!isValidCapability(capability)) { + Log.e(TAG, "addForbiddenCapability is called with invalid capability: " + capability); + return; + } mForbiddenNetworkCapabilities |= 1L << capability; mNetworkCapabilities &= ~(1L << capability); // remove from requested capabilities } @@ -878,7 +885,10 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public @NonNull NetworkCapabilities removeCapability(@NetCapability int capability) { - checkValidCapability(capability); + if (!isValidCapability(capability)) { + Log.e(TAG, "removeCapability is called with invalid capability: " + capability); + return this; + } final long mask = ~(1L << capability); mNetworkCapabilities &= mask; return this; @@ -893,7 +903,11 @@ public final class NetworkCapabilities implements Parcelable { * @hide */ public @NonNull NetworkCapabilities removeForbiddenCapability(@NetCapability int capability) { - checkValidCapability(capability); + if (!isValidCapability(capability)) { + Log.e(TAG, + "removeForbiddenCapability is called with invalid capability: " + capability); + return this; + } mForbiddenNetworkCapabilities &= ~(1L << capability); return this; } @@ -2632,12 +2646,6 @@ public final class NetworkCapabilities implements Parcelable { return capability >= 0 && capability <= MAX_NET_CAPABILITY; } - private static void checkValidCapability(@NetworkCapabilities.NetCapability int capability) { - if (!isValidCapability(capability)) { - throw new IllegalArgumentException("NetworkCapability " + capability + " out of range"); - } - } - private static boolean isValidEnterpriseId( @NetworkCapabilities.EnterpriseId int enterpriseId) { return enterpriseId >= NET_ENTERPRISE_ID_1 diff --git a/tests/common/java/android/net/NetworkCapabilitiesTest.java b/tests/common/java/android/net/NetworkCapabilitiesTest.java index 6eb56c7b45..0f0e2f124b 100644 --- a/tests/common/java/android/net/NetworkCapabilitiesTest.java +++ b/tests/common/java/android/net/NetworkCapabilitiesTest.java @@ -61,6 +61,7 @@ import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; import static android.net.NetworkCapabilities.TRANSPORT_WIFI_AWARE; import static android.os.Process.INVALID_UID; + import static com.android.modules.utils.build.SdkLevel.isAtLeastS; import static com.android.modules.utils.build.SdkLevel.isAtLeastT; import static com.android.modules.utils.build.SdkLevel.isAtLeastV; @@ -68,6 +69,7 @@ import static com.android.testutils.DevSdkIgnoreRuleKt.SC_V2; import static com.android.testutils.MiscAsserts.assertEmpty; import static com.android.testutils.MiscAsserts.assertThrows; import static com.android.testutils.ParcelUtils.assertParcelingIsLossless; + import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -83,21 +85,25 @@ import android.net.wifi.aware.WifiAwareNetworkSpecifier; import android.os.Build; import android.util.ArraySet; import android.util.Range; + import androidx.test.filters.SmallTest; + import com.android.testutils.CompatUtil; import com.android.testutils.ConnectivityModuleTest; import com.android.testutils.DevSdkIgnoreRule; import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo; import com.android.testutils.DevSdkIgnoreRunner; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.Set; + import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mockito; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; + @SmallTest @RunWith(DevSdkIgnoreRunner.class) // NetworkCapabilities is only updatable on S+, and this test covers behavior which implementation @@ -1465,4 +1471,26 @@ public class NetworkCapabilitiesTest { assertEquals("-SUPL-VALIDATED-CAPTIVE_PORTAL+MMS+OEM_PAID", nc1.describeCapsDifferencesFrom(nc2)); } + + @Test + public void testInvalidCapability() { + final int invalidCapability = Integer.MAX_VALUE; + // Passing invalid capability does not throw + final NetworkCapabilities nc1 = new NetworkCapabilities.Builder() + .addCapability(NET_CAPABILITY_INTERNET) + .addForbiddenCapability(NET_CAPABILITY_NOT_ROAMING) + .removeCapability(invalidCapability) + .removeForbiddenCapability(invalidCapability) + .addCapability(invalidCapability) + .addForbiddenCapability(invalidCapability) + .build(); + + final NetworkCapabilities nc2 = new NetworkCapabilities.Builder() + .addCapability(NET_CAPABILITY_INTERNET) + .addForbiddenCapability(NET_CAPABILITY_NOT_ROAMING) + .build(); + + // nc1 and nc2 are the same since invalid capability is ignored + assertEquals(nc1, nc2); + } }