From b7e8a2b97c9596bd11448380d1571a3a8a69acd1 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 31 Jan 2023 14:47:19 +0900 Subject: [PATCH 1/6] Reorder add/remove/register/unregister carrier listeners Helper methods in the middle of the class make it harder and more confusing. This patch moves the helpers out of the way and regroups the useful methods in a more logical order. Test: FrameworkNetTests Change-Id: I386ef7140c0535c9817a663910c19afeaf70981b --- .../CarrierPrivilegeAuthenticator.java | 53 ++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java index ef07201abd..7a001a119b 100644 --- a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java +++ b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java @@ -152,24 +152,13 @@ public class CarrierPrivilegeAuthenticator { } } - private void addCarrierPrivilegesListener(int logicalSlotIndex, Executor executor, - CarrierPrivilegesListenerShim listener) { - try { - mTelephonyManagerShim.addCarrierPrivilegesListener( - logicalSlotIndex, executor, listener); - } catch (UnsupportedApiLevelException unsupportedApiLevelException) { - // Should not happen since CarrierPrivilegeAuthenticator is only used on T+ - Log.e(TAG, "addCarrierPrivilegesListener API is not available"); - } - } - - private void removeCarrierPrivilegesListener(CarrierPrivilegesListenerShim listener) { - try { - mTelephonyManagerShim.removeCarrierPrivilegesListener(listener); - } catch (UnsupportedApiLevelException unsupportedApiLevelException) { - // Should not happen since CarrierPrivilegeAuthenticator is only used on T+ - Log.e(TAG, "removeCarrierPrivilegesListener API is not available"); + @GuardedBy("mLock") + private void unregisterCarrierPrivilegesListeners() { + for (CarrierPrivilegesListenerShim carrierPrivilegesListener : + mCarrierPrivilegesChangedListeners) { + removeCarrierPrivilegesListener(carrierPrivilegesListener); } + mCarrierPrivilegesChangedListeners.clear(); } private String getCarrierServicePackageNameForLogicalSlot(int logicalSlotIndex) { @@ -183,14 +172,6 @@ public class CarrierPrivilegeAuthenticator { return null; } - private void unregisterCarrierPrivilegesListeners() { - for (CarrierPrivilegesListenerShim carrierPrivilegesListener : - mCarrierPrivilegesChangedListeners) { - removeCarrierPrivilegesListener(carrierPrivilegesListener); - } - mCarrierPrivilegesChangedListeners.clear(); - } - /** * Check if a UID is the carrier service app of the subscription ID in the provided capabilities * @@ -273,4 +254,26 @@ public class CarrierPrivilegeAuthenticator { int getCarrierServicePackageUidForSlot(int slotId) { return getUidForPackage(getCarrierServicePackageNameForLogicalSlot(slotId)); } + + // Helper methods to avoid having to deal with UnsupportedApiLevelException. + + private void addCarrierPrivilegesListener(int logicalSlotIndex, Executor executor, + CarrierPrivilegesListenerShim listener) { + try { + mTelephonyManagerShim.addCarrierPrivilegesListener( + logicalSlotIndex, executor, listener); + } catch (UnsupportedApiLevelException unsupportedApiLevelException) { + // Should not happen since CarrierPrivilegeAuthenticator is only used on T+ + Log.e(TAG, "addCarrierPrivilegesListener API is not available"); + } + } + + private void removeCarrierPrivilegesListener(CarrierPrivilegesListenerShim listener) { + try { + mTelephonyManagerShim.removeCarrierPrivilegesListener(listener); + } catch (UnsupportedApiLevelException unsupportedApiLevelException) { + // Should not happen since CarrierPrivilegeAuthenticator is only used on T+ + Log.e(TAG, "removeCarrierPrivilegesListener API is not available"); + } + } } From 0a893c5ca63bc4b1a074e8c97e5fabc877cee2dc Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 31 Jan 2023 14:52:25 +0900 Subject: [PATCH 2/6] Pass the modem count to registerCarrierPrivilegeListeners This is clearer and less dangerous locking-wise Test: FrameworksNetTests Change-Id: Ife09fef848144e74ddb02aba3594913899bc9f46 --- .../connectivity/CarrierPrivilegeAuthenticator.java | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java index 7a001a119b..90fc0fa7b7 100644 --- a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java +++ b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java @@ -119,17 +119,13 @@ public class CarrierPrivilegeAuthenticator { synchronized (mLock) { unregisterCarrierPrivilegesListeners(); mModemCount = mTelephonyManager.getActiveModemCount(); - registerCarrierPrivilegesListeners(); + registerCarrierPrivilegesListeners(mModemCount); updateCarrierServiceUid(); } } - private void registerCarrierPrivilegesListeners() { + private void registerCarrierPrivilegesListeners(final int modemCount) { final HandlerExecutor executor = new HandlerExecutor(mHandler); - int modemCount; - synchronized (mLock) { - modemCount = mModemCount; - } try { for (int i = 0; i < modemCount; i++) { CarrierPrivilegesListenerShim carrierPrivilegesListener = From c9dd664ae2ba67ef6b1b19a730afb5c1c223ba82 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 26 Sep 2023 12:28:04 +0900 Subject: [PATCH 3/6] Update the carrier service UID when onCarrierServiceChanged Tests show that when the carrier service package changes, sometimes neither onCarrierPrivilegesChanged is called nor the ACTION_MULTI_SIM_CONFIG_CHANGED broadcast is sent. Because updating synchronously can be done at any time, it's a strict improvement to also do it onCarrierServiceChanged. Test: CarrierPrivilegeAuthenticatorTest Change-Id: I9525bfe074dd686720d50e19be15529b248e9dbb --- .../connectivity/CarrierPrivilegeAuthenticator.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java index 90fc0fa7b7..e40b8d44ba 100644 --- a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java +++ b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java @@ -21,6 +21,7 @@ import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static com.android.server.connectivity.ConnectivityFlags.CARRIER_SERVICE_CHANGED_USE_CALLBACK; import android.annotation.NonNull; +import android.annotation.Nullable; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; @@ -139,6 +140,15 @@ public class CarrierPrivilegeAuthenticator { // with the onSubscriptionsChangedListener and broadcasts. updateCarrierServiceUid(); } + @Override + public void onCarrierServiceChanged( + @Nullable final String carrierServicePackageName, + final int carrierServiceUid) { + // Re-trigger the synchronous check (which is also very cheap due + // to caching in CarrierPrivilegesTracker). This allows consistency + // with the onSubscriptionsChangedListener and broadcasts. + updateCarrierServiceUid(); + } }; addCarrierPrivilegesListener(i, executor, carrierPrivilegesListener); mCarrierPrivilegesChangedListeners.add(carrierPrivilegesListener); From 1e4c90c71d0bd982b609da9e58911259cfa2d1c7 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 31 Jan 2023 15:18:13 +0900 Subject: [PATCH 4/6] Have a full class (not inline) for privilege listener Test: CarrierPrivilegeAuthenticatorTest Change-Id: I1e8944de203767edf036da3fe77171025e72fcb7 --- .../CarrierPrivilegeAuthenticator.java | 55 +++++++++---------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java index e40b8d44ba..55a1f654e9 100644 --- a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java +++ b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java @@ -73,9 +73,7 @@ public class CarrierPrivilegeAuthenticator { private final Object mLock = new Object(); private final Handler mHandler; @NonNull - @GuardedBy("mLock") - private final List mCarrierPrivilegesChangedListeners = - new ArrayList<>(); + private final List mCarrierPrivilegesChangedListeners = new ArrayList<>(); private final boolean mUseCallbacksForServiceChanged; public CarrierPrivilegeAuthenticator(@NonNull final Context c, @@ -125,31 +123,31 @@ public class CarrierPrivilegeAuthenticator { } } + private class PrivilegeListener implements CarrierPrivilegesListenerShim { + @Override public void onCarrierPrivilegesChanged( + @NonNull List privilegedPackageNames, + @NonNull int[] privilegedUids) { + // Re-trigger the synchronous check (which is also very cheap due + // to caching in CarrierPrivilegesTracker). This allows consistency + // with the onSubscriptionsChangedListener and broadcasts. + updateCarrierServiceUid(); + } + @Override + public void onCarrierServiceChanged( + @Nullable final String carrierServicePackageName, + final int carrierServiceUid) { + // Re-trigger the synchronous check (which is also very cheap due + // to caching in CarrierPrivilegesTracker). This allows consistency + // with the onSubscriptionsChangedListener and broadcasts. + updateCarrierServiceUid(); + } + } + private void registerCarrierPrivilegesListeners(final int modemCount) { final HandlerExecutor executor = new HandlerExecutor(mHandler); try { for (int i = 0; i < modemCount; i++) { - CarrierPrivilegesListenerShim carrierPrivilegesListener = - new CarrierPrivilegesListenerShim() { - @Override - public void onCarrierPrivilegesChanged( - @NonNull List privilegedPackageNames, - @NonNull int[] privilegedUids) { - // Re-trigger the synchronous check (which is also very cheap due - // to caching in CarrierPrivilegesTracker). This allows consistency - // with the onSubscriptionsChangedListener and broadcasts. - updateCarrierServiceUid(); - } - @Override - public void onCarrierServiceChanged( - @Nullable final String carrierServicePackageName, - final int carrierServiceUid) { - // Re-trigger the synchronous check (which is also very cheap due - // to caching in CarrierPrivilegesTracker). This allows consistency - // with the onSubscriptionsChangedListener and broadcasts. - updateCarrierServiceUid(); - } - }; + PrivilegeListener carrierPrivilegesListener = new PrivilegeListener(); addCarrierPrivilegesListener(i, executor, carrierPrivilegesListener); mCarrierPrivilegesChangedListeners.add(carrierPrivilegesListener); } @@ -160,8 +158,7 @@ public class CarrierPrivilegeAuthenticator { @GuardedBy("mLock") private void unregisterCarrierPrivilegesListeners() { - for (CarrierPrivilegesListenerShim carrierPrivilegesListener : - mCarrierPrivilegesChangedListeners) { + for (PrivilegeListener carrierPrivilegesListener : mCarrierPrivilegesChangedListeners) { removeCarrierPrivilegesListener(carrierPrivilegesListener); } mCarrierPrivilegesChangedListeners.clear(); @@ -263,8 +260,8 @@ public class CarrierPrivilegeAuthenticator { // Helper methods to avoid having to deal with UnsupportedApiLevelException. - private void addCarrierPrivilegesListener(int logicalSlotIndex, Executor executor, - CarrierPrivilegesListenerShim listener) { + private void addCarrierPrivilegesListener(final int logicalSlotIndex, + @NonNull final Executor executor, @NonNull final PrivilegeListener listener) { try { mTelephonyManagerShim.addCarrierPrivilegesListener( logicalSlotIndex, executor, listener); @@ -274,7 +271,7 @@ public class CarrierPrivilegeAuthenticator { } } - private void removeCarrierPrivilegesListener(CarrierPrivilegesListenerShim listener) { + private void removeCarrierPrivilegesListener(PrivilegeListener listener) { try { mTelephonyManagerShim.removeCarrierPrivilegesListener(listener); } catch (UnsupportedApiLevelException unsupportedApiLevelException) { From 5a6855743884e674506aa758429565a0fe0ca593 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 31 Jan 2023 15:28:01 +0900 Subject: [PATCH 5/6] Delete slots as the listeners are unregistered. For the current code this is a no-op because the new slots are always computed immediately in the same critical section. When the code uses callbacks for the carrier service UID though, there will no longer be a time where the entire array is reset. Instead, registering the callback will immediately trigger the call that populates it again, which means the value needs to be removed when a SIM card is removed. Unregistering the callback is a good time to do this. Test: CarrierPrivilegeAuthenticatorTest Change-Id: I6de4abdc57ffa455d7f8e4d35f5dd1e18937e94e --- .../CarrierPrivilegeAuthenticator.java | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java index 55a1f654e9..dd1519e190 100644 --- a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java +++ b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java @@ -124,6 +124,11 @@ public class CarrierPrivilegeAuthenticator { } private class PrivilegeListener implements CarrierPrivilegesListenerShim { + public final int mLogicalSlot; + PrivilegeListener(final int logicalSlot) { + mLogicalSlot = logicalSlot; + } + @Override public void onCarrierPrivilegesChanged( @NonNull List privilegedPackageNames, @NonNull int[] privilegedUids) { @@ -147,8 +152,8 @@ public class CarrierPrivilegeAuthenticator { final HandlerExecutor executor = new HandlerExecutor(mHandler); try { for (int i = 0; i < modemCount; i++) { - PrivilegeListener carrierPrivilegesListener = new PrivilegeListener(); - addCarrierPrivilegesListener(i, executor, carrierPrivilegesListener); + PrivilegeListener carrierPrivilegesListener = new PrivilegeListener(i); + addCarrierPrivilegesListener(executor, carrierPrivilegesListener); mCarrierPrivilegesChangedListeners.add(carrierPrivilegesListener); } } catch (IllegalArgumentException e) { @@ -160,6 +165,7 @@ public class CarrierPrivilegeAuthenticator { private void unregisterCarrierPrivilegesListeners() { for (PrivilegeListener carrierPrivilegesListener : mCarrierPrivilegesChangedListeners) { removeCarrierPrivilegesListener(carrierPrivilegesListener); + mCarrierServiceUid.delete(carrierPrivilegesListener.mLogicalSlot); } mCarrierPrivilegesChangedListeners.clear(); } @@ -260,11 +266,11 @@ public class CarrierPrivilegeAuthenticator { // Helper methods to avoid having to deal with UnsupportedApiLevelException. - private void addCarrierPrivilegesListener(final int logicalSlotIndex, - @NonNull final Executor executor, @NonNull final PrivilegeListener listener) { + private void addCarrierPrivilegesListener(@NonNull final Executor executor, + @NonNull final PrivilegeListener listener) { try { - mTelephonyManagerShim.addCarrierPrivilegesListener( - logicalSlotIndex, executor, listener); + mTelephonyManagerShim.addCarrierPrivilegesListener(listener.mLogicalSlot, executor, + listener); } catch (UnsupportedApiLevelException unsupportedApiLevelException) { // Should not happen since CarrierPrivilegeAuthenticator is only used on T+ Log.e(TAG, "addCarrierPrivilegesListener API is not available"); From 361dad3243514e224e001e96014d36eb9d69fa55 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 31 Jan 2023 20:41:53 +0900 Subject: [PATCH 6/6] Use carrier service changed callbacks when flag is on Test: FrameworksNetTests Note that carrierPrivilegeAuthenticatorTest is already an @Parameterized test with flag = on and off, so it already tests both. Change-Id: I52fcfd3f21a13d7a39952ba828464ce6ef4085c2 --- .../CarrierPrivilegeAuthenticator.java | 21 ++++++++++++------- .../CarrierPrivilegeAuthenticatorTest.java | 8 +++++++ 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java index dd1519e190..ab7b1a7902 100644 --- a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java +++ b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java @@ -119,7 +119,7 @@ public class CarrierPrivilegeAuthenticator { unregisterCarrierPrivilegesListeners(); mModemCount = mTelephonyManager.getActiveModemCount(); registerCarrierPrivilegesListeners(mModemCount); - updateCarrierServiceUid(); + if (!mUseCallbacksForServiceChanged) updateCarrierServiceUid(); } } @@ -132,19 +132,26 @@ public class CarrierPrivilegeAuthenticator { @Override public void onCarrierPrivilegesChanged( @NonNull List privilegedPackageNames, @NonNull int[] privilegedUids) { + if (mUseCallbacksForServiceChanged) return; // Re-trigger the synchronous check (which is also very cheap due // to caching in CarrierPrivilegesTracker). This allows consistency // with the onSubscriptionsChangedListener and broadcasts. updateCarrierServiceUid(); } + @Override - public void onCarrierServiceChanged( - @Nullable final String carrierServicePackageName, + public void onCarrierServiceChanged(@Nullable final String carrierServicePackageName, final int carrierServiceUid) { - // Re-trigger the synchronous check (which is also very cheap due - // to caching in CarrierPrivilegesTracker). This allows consistency - // with the onSubscriptionsChangedListener and broadcasts. - updateCarrierServiceUid(); + if (!mUseCallbacksForServiceChanged) { + // Re-trigger the synchronous check (which is also very cheap due + // to caching in CarrierPrivilegesTracker). This allows consistency + // with the onSubscriptionsChangedListener and broadcasts. + updateCarrierServiceUid(); + return; + } + synchronized (mLock) { + mCarrierServiceUid.put(mLogicalSlot, carrierServiceUid); + } } } diff --git a/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java b/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java index 2ecd2a14c4..81136262b9 100644 --- a/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java +++ b/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java @@ -158,6 +158,8 @@ public class CarrierPrivilegeAuthenticatorTest { assertNotNull(initialListeners.get(1)); assertEquals(2, initialListeners.size()); + initialListeners.get(0).onCarrierServiceChanged(null, mCarrierConfigPkgUid); + final NetworkCapabilities.Builder ncBuilder = new NetworkCapabilities.Builder() .addTransportType(TRANSPORT_CELLULAR) .setNetworkSpecifier(new TelephonyNetworkSpecifier(0)); @@ -194,6 +196,8 @@ public class CarrierPrivilegeAuthenticatorTest { assertNotNull(newListeners.get(0)); assertEquals(1, newListeners.size()); + newListeners.get(0).onCarrierServiceChanged(null, mCarrierConfigPkgUid); + final TelephonyNetworkSpecifier specifier = new TelephonyNetworkSpecifier(0); final NetworkCapabilities nc = new NetworkCapabilities.Builder() .addTransportType(TRANSPORT_CELLULAR) @@ -219,6 +223,7 @@ public class CarrierPrivilegeAuthenticatorTest { applicationInfo.uid = mCarrierConfigPkgUid + 1; doReturn(applicationInfo).when(mPackageManager).getApplicationInfo(eq(mTestPkg), anyInt()); listener.onCarrierPrivilegesChanged(Collections.emptyList(), new int[] {}); + listener.onCarrierServiceChanged(null, applicationInfo.uid); assertFalse(mCarrierPrivilegeAuthenticator.hasCarrierPrivilegeForNetworkCapabilities( mCarrierConfigPkgUid, nc)); @@ -228,6 +233,9 @@ public class CarrierPrivilegeAuthenticatorTest { @Test public void testDefaultSubscription() throws Exception { + final CarrierPrivilegesListenerShim listener = getCarrierPrivilegesListeners().get(0); + listener.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + final NetworkCapabilities.Builder ncBuilder = new NetworkCapabilities.Builder(); ncBuilder.addTransportType(TRANSPORT_CELLULAR); assertFalse(mCarrierPrivilegeAuthenticator.hasCarrierPrivilegeForNetworkCapabilities(