From 4fcaa4e803d19fee091a97d4dca11d13988580eb Mon Sep 17 00:00:00 2001 From: Motomu Utsumi Date: Wed, 28 Feb 2024 17:15:41 +0900 Subject: [PATCH] Use connectivityHandler in CarrierPrivilegeAuthenticator When request_restricted_wifi is enabled CarrierPrivilegeAuthenticator calls CarrierPrivilegesLost callback passed from ConnectivityService. But CarrierPrivilegeAuthenticator was using its own handler thread to receive broadcast and callbacks so CarrierPrivilegesLost callback needed to post a message to the CS handler. This CL updates CarrierPrivilegeAuthenticator to use CS handler if request_restricted_wifi is enabled, and remove the message post in ConnectivityService. Test: TH Change-Id: Ib41304a30e3dd366b0f6f1d7df81145f0e05ece1 --- .../android/server/ConnectivityService.java | 32 +++----- .../CarrierPrivilegeAuthenticator.java | 73 ++++++++++++++----- .../ConnectivityServiceIntegrationTest.kt | 21 +++--- .../server/ConnectivityServiceTest.java | 11 ++- .../CarrierPrivilegeAuthenticatorTest.java | 71 +++++++++++++----- .../server/connectivityservice/base/CSTest.kt | 3 +- 6 files changed, 139 insertions(+), 72 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 30440f557a..a5d1645de2 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -844,11 +844,6 @@ public class ConnectivityService extends IConnectivityManager.Stub */ private static final int EVENT_UID_FROZEN_STATE_CHANGED = 61; - /** - * Event to inform the ConnectivityService handler when a uid has lost carrier privileges. - */ - private static final int EVENT_UID_CARRIER_PRIVILEGES_LOST = 62; - /** * Argument for {@link #EVENT_PROVISIONING_NOTIFICATION} to indicate that the notification * should be shown. @@ -1289,14 +1284,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } private final LegacyTypeTracker mLegacyTypeTracker = new LegacyTypeTracker(this); - @VisibleForTesting - void onCarrierPrivilegesLost(Integer uid, Integer subId) { - if (mRequestRestrictedWifiEnabled) { - mHandler.sendMessage(mHandler.obtainMessage( - EVENT_UID_CARRIER_PRIVILEGES_LOST, uid, subId)); - } - } - final LocalPriorityDump mPriorityDumper = new LocalPriorityDump(); /** * Helper class which parses out priority arguments and dumps sections according to their @@ -1518,10 +1505,11 @@ public class ConnectivityService extends IConnectivityManager.Stub @NonNull final Context context, @NonNull final TelephonyManager tm, boolean requestRestrictedWifiEnabled, - @NonNull BiConsumer listener) { + @NonNull BiConsumer listener, + @NonNull final Handler connectivityServiceHandler) { if (isAtLeastT()) { - return new CarrierPrivilegeAuthenticator( - context, tm, requestRestrictedWifiEnabled, listener); + return new CarrierPrivilegeAuthenticator(context, tm, requestRestrictedWifiEnabled, + listener, connectivityServiceHandler); } else { return null; } @@ -1806,7 +1794,7 @@ public class ConnectivityService extends IConnectivityManager.Stub && mDeps.isFeatureEnabled(context, REQUEST_RESTRICTED_WIFI); mCarrierPrivilegeAuthenticator = mDeps.makeCarrierPrivilegeAuthenticator( mContext, mTelephonyManager, mRequestRestrictedWifiEnabled, - this::onCarrierPrivilegesLost); + this::handleUidCarrierPrivilegesLost, mHandler); if (mDeps.isAtLeastU() && mDeps @@ -3823,6 +3811,10 @@ public class ConnectivityService extends IConnectivityManager.Stub mSatelliteAccessController.start(); } + if (mCarrierPrivilegeAuthenticator != null) { + mCarrierPrivilegeAuthenticator.start(); + } + // On T+ devices, register callback for statsd to pull NETWORK_BPF_MAP_INFO atom if (mDeps.isAtLeastT()) { mBpfNetMaps.setPullAtomCallback(mContext); @@ -6510,9 +6502,6 @@ public class ConnectivityService extends IConnectivityManager.Stub UidFrozenStateChangedArgs args = (UidFrozenStateChangedArgs) msg.obj; handleFrozenUids(args.mUids, args.mFrozenStates); break; - case EVENT_UID_CARRIER_PRIVILEGES_LOST: - handleUidCarrierPrivilegesLost(msg.arg1, msg.arg2); - break; } } } @@ -9168,6 +9157,9 @@ public class ConnectivityService extends IConnectivityManager.Stub } private void handleUidCarrierPrivilegesLost(int uid, int subId) { + if (!mRequestRestrictedWifiEnabled) { + return; + } ensureRunningOnConnectivityServiceThread(); // A NetworkRequest needs to be revoked when all the conditions are met // 1. It requests restricted network diff --git a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java index 04d0fc1ee7..f5fa4fb2e1 100644 --- a/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java +++ b/service/src/com/android/server/connectivity/CarrierPrivilegeAuthenticator.java @@ -91,42 +91,62 @@ public class CarrierPrivilegeAuthenticator { @NonNull final TelephonyManager t, @NonNull final TelephonyManagerShim telephonyManagerShim, final boolean requestRestrictedWifiEnabled, - @NonNull BiConsumer listener) { + @NonNull BiConsumer listener, + @NonNull final Handler connectivityServiceHandler) { mContext = c; mTelephonyManager = t; mTelephonyManagerShim = telephonyManagerShim; - final HandlerThread thread = deps.makeHandlerThread(); - thread.start(); - mHandler = new Handler(thread.getLooper()); mUseCallbacksForServiceChanged = deps.isFeatureEnabled( c, CARRIER_SERVICE_CHANGED_USE_CALLBACK); mRequestRestrictedWifiEnabled = requestRestrictedWifiEnabled; mListener = listener; + if (mRequestRestrictedWifiEnabled) { + mHandler = connectivityServiceHandler; + } else { + final HandlerThread thread = deps.makeHandlerThread(); + thread.start(); + mHandler = new Handler(thread.getLooper()); + synchronized (mLock) { + registerSimConfigChangedReceiver(); + simConfigChanged(); + } + } + } + + private void registerSimConfigChangedReceiver() { final IntentFilter filter = new IntentFilter(); filter.addAction(TelephonyManager.ACTION_MULTI_SIM_CONFIG_CHANGED); - synchronized (mLock) { - // Never unregistered because the system server never stops - c.registerReceiver(new BroadcastReceiver() { - @Override - public void onReceive(final Context context, final Intent intent) { - switch (intent.getAction()) { - case TelephonyManager.ACTION_MULTI_SIM_CONFIG_CHANGED: - simConfigChanged(); - break; - default: - Log.d(TAG, "Unknown intent received, action: " + intent.getAction()); - } + // Never unregistered because the system server never stops + mContext.registerReceiver(new BroadcastReceiver() { + @Override + public void onReceive(final Context context, final Intent intent) { + switch (intent.getAction()) { + case TelephonyManager.ACTION_MULTI_SIM_CONFIG_CHANGED: + simConfigChanged(); + break; + default: + Log.d(TAG, "Unknown intent received, action: " + intent.getAction()); } - }, filter, null, mHandler); - simConfigChanged(); + } + }, filter, null, mHandler); + } + + /** + * Start CarrierPrivilegeAuthenticator + */ + public void start() { + if (mRequestRestrictedWifiEnabled) { + registerSimConfigChangedReceiver(); + mHandler.post(this::simConfigChanged); } } public CarrierPrivilegeAuthenticator(@NonNull final Context c, @NonNull final TelephonyManager t, final boolean requestRestrictedWifiEnabled, - @NonNull BiConsumer listener) { + @NonNull BiConsumer listener, + @NonNull final Handler connectivityServiceHandler) { this(c, new Dependencies(), t, TelephonyManagerShimImpl.newInstance(t), - requestRestrictedWifiEnabled, listener); + requestRestrictedWifiEnabled, listener, connectivityServiceHandler); } public static class Dependencies { @@ -146,6 +166,10 @@ public class CarrierPrivilegeAuthenticator { } private void simConfigChanged() { + // If mRequestRestrictedWifiEnabled is false, constructor calls simConfigChanged + if (mRequestRestrictedWifiEnabled) { + ensureRunningOnHandlerThread(); + } synchronized (mLock) { unregisterCarrierPrivilegesListeners(); mModemCount = mTelephonyManager.getActiveModemCount(); @@ -188,6 +212,7 @@ public class CarrierPrivilegeAuthenticator { public void onCarrierPrivilegesChanged( @NonNull List privilegedPackageNames, @NonNull int[] privilegedUids) { + ensureRunningOnHandlerThread(); if (mUseCallbacksForServiceChanged) return; // Re-trigger the synchronous check (which is also very cheap due // to caching in CarrierPrivilegesTracker). This allows consistency @@ -198,6 +223,7 @@ public class CarrierPrivilegeAuthenticator { @Override public void onCarrierServiceChanged(@Nullable final String carrierServicePackageName, final int carrierServiceUid) { + ensureRunningOnHandlerThread(); if (!mUseCallbacksForServiceChanged) { // Re-trigger the synchronous check (which is also very cheap due // to caching in CarrierPrivilegesTracker). This allows consistency @@ -439,6 +465,13 @@ public class CarrierPrivilegeAuthenticator { } } + private void ensureRunningOnHandlerThread() { + if (mHandler.getLooper().getThread() != Thread.currentThread()) { + throw new IllegalStateException( + "Not running on handler thread: " + Thread.currentThread().getName()); + } + } + public void dump(IndentingPrintWriter pw) { pw.println("CarrierPrivilegeAuthenticator:"); pw.println("mRequestRestrictedWifiEnabled = " + mRequestRestrictedWifiEnabled); diff --git a/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt b/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt index 035f607310..d2e46af8b8 100644 --- a/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt +++ b/tests/integration/src/com/android/server/net/integrationtests/ConnectivityServiceIntegrationTest.kt @@ -243,18 +243,19 @@ class ConnectivityServiceIntegrationTest { super.makeHandlerThread(tag).also { handlerThreads.add(it) } override fun makeCarrierPrivilegeAuthenticator( - context: Context, - tm: TelephonyManager, - requestRestrictedWifiEnabled: Boolean, - listener: BiConsumer + context: Context, + tm: TelephonyManager, + requestRestrictedWifiEnabled: Boolean, + listener: BiConsumer, + handler: Handler ): CarrierPrivilegeAuthenticator { return CarrierPrivilegeAuthenticator(context, - object : CarrierPrivilegeAuthenticator.Dependencies() { - override fun makeHandlerThread(): HandlerThread = - super.makeHandlerThread().also { handlerThreads.add(it) } - }, - tm, TelephonyManagerShimImpl.newInstance(tm), - requestRestrictedWifiEnabled, listener) + object : CarrierPrivilegeAuthenticator.Dependencies() { + override fun makeHandlerThread(): HandlerThread = + super.makeHandlerThread().also { handlerThreads.add(it) } + }, + tm, TelephonyManagerShimImpl.newInstance(tm), + requestRestrictedWifiEnabled, listener, handler) } override fun makeSatelliteAccessController( diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index e09a2cbf56..98ba91fafa 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -2054,12 +2054,16 @@ public class ConnectivityServiceTest { }; } + private BiConsumer mCarrierPrivilegesLostListener; + @Override public CarrierPrivilegeAuthenticator makeCarrierPrivilegeAuthenticator( @NonNull final Context context, @NonNull final TelephonyManager tm, final boolean requestRestrictedWifiEnabled, - BiConsumer listener) { + BiConsumer listener, + @NonNull final Handler handler) { + mCarrierPrivilegesLostListener = listener; return mDeps.isAtLeastT() ? mCarrierPrivilegeAuthenticator : null; } @@ -17446,7 +17450,10 @@ public class ConnectivityServiceTest { .isCarrierServiceUidForNetworkCapabilities(eq(Process.myUid()), any()); doReturn(TEST_SUBSCRIPTION_ID).when(mCarrierPrivilegeAuthenticator) .getSubIdFromNetworkCapabilities(any()); - mService.onCarrierPrivilegesLost(lostPrivilegeUid, lostPrivilegeSubId); + + visibleOnHandlerThread(mCsHandlerThread.getThreadHandler(), () -> { + mDeps.mCarrierPrivilegesLostListener.accept(lostPrivilegeUid, lostPrivilegeSubId); + }); waitForIdle(); if (expectCapChanged) { diff --git a/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java b/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java index 7bd2b5693f..ab81abc030 100644 --- a/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java +++ b/tests/unit/java/com/android/server/connectivity/CarrierPrivilegeAuthenticatorTest.java @@ -21,6 +21,7 @@ import static android.net.NetworkCapabilities.TRANSPORT_WIFI; import static android.telephony.TelephonyManager.ACTION_MULTI_SIM_CONFIG_CHANGED; import static com.android.server.connectivity.ConnectivityFlags.CARRIER_SERVICE_CHANGED_USE_CALLBACK; +import static com.android.testutils.HandlerUtils.visibleOnHandlerThread; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -45,6 +46,7 @@ import android.content.pm.PackageManager; import android.net.NetworkCapabilities; import android.net.TelephonyNetworkSpecifier; import android.os.Build; +import android.os.Handler; import android.os.HandlerThread; import android.telephony.TelephonyManager; @@ -56,6 +58,7 @@ import com.android.server.connectivity.CarrierPrivilegeAuthenticator.Dependencie import com.android.testutils.DevSdkIgnoreRule; import com.android.testutils.DevSdkIgnoreRule.IgnoreUpTo; import com.android.testutils.DevSdkIgnoreRunner; +import com.android.testutils.HandlerUtils; import org.junit.After; import org.junit.Rule; @@ -85,6 +88,7 @@ public class CarrierPrivilegeAuthenticatorTest { private static final int SUBSCRIPTION_COUNT = 2; private static final int TEST_SUBSCRIPTION_ID = 1; + private static final int TIMEOUT_MS = 1_000; @NonNull private final Context mContext; @NonNull private final TelephonyManager mTelephonyManager; @@ -97,13 +101,16 @@ public class CarrierPrivilegeAuthenticatorTest { private final String mTestPkg = "com.android.server.connectivity.test"; private final BroadcastReceiver mMultiSimBroadcastReceiver; @NonNull private final HandlerThread mHandlerThread; + @NonNull private final Handler mCsHandler; + @NonNull private final HandlerThread mCsHandlerThread; public class TestCarrierPrivilegeAuthenticator extends CarrierPrivilegeAuthenticator { TestCarrierPrivilegeAuthenticator(@NonNull final Context c, @NonNull final Dependencies deps, - @NonNull final TelephonyManager t) { + @NonNull final TelephonyManager t, + @NonNull final Handler handler) { super(c, deps, t, mTelephonyManagerShim, true /* requestRestrictedWifiEnabled */, - mListener); + mListener, handler); } @Override protected int getSubId(int slotIndex) { @@ -112,8 +119,11 @@ public class CarrierPrivilegeAuthenticatorTest { } @After - public void tearDown() { + public void tearDown() throws Exception { mHandlerThread.quit(); + mHandlerThread.join(); + mCsHandlerThread.quit(); + mCsHandlerThread.join(); } /** Parameters to test both using callbacks or the old broadcast */ @@ -141,8 +151,14 @@ public class CarrierPrivilegeAuthenticatorTest { final ApplicationInfo applicationInfo = new ApplicationInfo(); applicationInfo.uid = mCarrierConfigPkgUid; doReturn(applicationInfo).when(mPackageManager).getApplicationInfo(eq(mTestPkg), anyInt()); - mCarrierPrivilegeAuthenticator = - new TestCarrierPrivilegeAuthenticator(mContext, deps, mTelephonyManager); + mCsHandlerThread = new HandlerThread( + CarrierPrivilegeAuthenticatorTest.class.getSimpleName() + "-CsHandlerThread"); + mCsHandlerThread.start(); + mCsHandler = new Handler(mCsHandlerThread.getLooper()); + mCarrierPrivilegeAuthenticator = new TestCarrierPrivilegeAuthenticator(mContext, deps, + mTelephonyManager, mCsHandler); + mCarrierPrivilegeAuthenticator.start(); + HandlerUtils.waitForIdle(mCsHandlerThread, TIMEOUT_MS); final ArgumentCaptor receiverCaptor = ArgumentCaptor.forClass(BroadcastReceiver.class); verify(mContext).registerReceiver(receiverCaptor.capture(), argThat(filter -> @@ -178,7 +194,9 @@ public class CarrierPrivilegeAuthenticatorTest { assertNotNull(initialListeners.get(1)); assertEquals(2, initialListeners.size()); - initialListeners.get(0).onCarrierServiceChanged(null, mCarrierConfigPkgUid); + visibleOnHandlerThread(mCsHandler, () -> { + initialListeners.get(0).onCarrierServiceChanged(null, mCarrierConfigPkgUid); + }); final NetworkCapabilities.Builder ncBuilder = new NetworkCapabilities.Builder() .addTransportType(TRANSPORT_CELLULAR) @@ -201,10 +219,10 @@ public class CarrierPrivilegeAuthenticatorTest { doReturn(1).when(mTelephonyManager).getActiveModemCount(); - // This is a little bit cavalier in that the call to onReceive is not on the handler - // thread that was specified in registerReceiver. - // TODO : capture the handler and call this on it if this causes flakiness. - mMultiSimBroadcastReceiver.onReceive(mContext, buildTestMultiSimConfigBroadcastIntent()); + visibleOnHandlerThread(mCsHandler, () -> { + mMultiSimBroadcastReceiver.onReceive(mContext, + buildTestMultiSimConfigBroadcastIntent()); + }); // Check all listeners have been removed for (CarrierPrivilegesListenerShim listener : initialListeners.values()) { verify(mTelephonyManagerShim).removeCarrierPrivilegesListener(eq(listener)); @@ -216,7 +234,9 @@ public class CarrierPrivilegeAuthenticatorTest { assertNotNull(newListeners.get(0)); assertEquals(1, newListeners.size()); - newListeners.get(0).onCarrierServiceChanged(null, mCarrierConfigPkgUid); + visibleOnHandlerThread(mCsHandler, () -> { + newListeners.get(0).onCarrierServiceChanged(null, mCarrierConfigPkgUid); + }); final TelephonyNetworkSpecifier specifier = new TelephonyNetworkSpecifier(TEST_SUBSCRIPTION_ID); @@ -235,12 +255,17 @@ public class CarrierPrivilegeAuthenticatorTest { public void testCarrierPrivilegesLostDueToCarrierServiceUpdate() throws Exception { final CarrierPrivilegesListenerShim l = getCarrierPrivilegesListeners().get(0); - l.onCarrierServiceChanged(null, mCarrierConfigPkgUid); - l.onCarrierServiceChanged(null, mCarrierConfigPkgUid + 1); + visibleOnHandlerThread(mCsHandler, () -> { + l.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + l.onCarrierServiceChanged(null, mCarrierConfigPkgUid + 1); + }); if (mUseCallbacks) { verify(mListener).accept(eq(mCarrierConfigPkgUid), eq(TEST_SUBSCRIPTION_ID)); } - l.onCarrierServiceChanged(null, mCarrierConfigPkgUid + 2); + + visibleOnHandlerThread(mCsHandler, () -> { + l.onCarrierServiceChanged(null, mCarrierConfigPkgUid + 2); + }); if (mUseCallbacks) { verify(mListener).accept(eq(mCarrierConfigPkgUid + 1), eq(TEST_SUBSCRIPTION_ID)); } @@ -260,8 +285,10 @@ public class CarrierPrivilegeAuthenticatorTest { final ApplicationInfo applicationInfo = new ApplicationInfo(); applicationInfo.uid = mCarrierConfigPkgUid + 1; doReturn(applicationInfo).when(mPackageManager).getApplicationInfo(eq(mTestPkg), anyInt()); - listener.onCarrierPrivilegesChanged(Collections.emptyList(), new int[] {}); - listener.onCarrierServiceChanged(null, applicationInfo.uid); + visibleOnHandlerThread(mCsHandler, () -> { + listener.onCarrierPrivilegesChanged(Collections.emptyList(), new int[]{}); + listener.onCarrierServiceChanged(null, applicationInfo.uid); + }); assertFalse(mCarrierPrivilegeAuthenticator.isCarrierServiceUidForNetworkCapabilities( mCarrierConfigPkgUid, nc)); @@ -272,7 +299,9 @@ public class CarrierPrivilegeAuthenticatorTest { @Test public void testDefaultSubscription() throws Exception { final CarrierPrivilegesListenerShim listener = getCarrierPrivilegesListeners().get(0); - listener.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + visibleOnHandlerThread(mCsHandler, () -> { + listener.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + }); final NetworkCapabilities.Builder ncBuilder = new NetworkCapabilities.Builder(); ncBuilder.addTransportType(TRANSPORT_CELLULAR); @@ -297,7 +326,9 @@ public class CarrierPrivilegeAuthenticatorTest { @IgnoreUpTo(Build.VERSION_CODES.TIRAMISU) public void testNetworkCapabilitiesContainOneSubId() throws Exception { final CarrierPrivilegesListenerShim listener = getCarrierPrivilegesListeners().get(0); - listener.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + visibleOnHandlerThread(mCsHandler, () -> { + listener.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + }); final NetworkCapabilities.Builder ncBuilder = new NetworkCapabilities.Builder(); ncBuilder.addTransportType(TRANSPORT_WIFI); @@ -311,7 +342,9 @@ public class CarrierPrivilegeAuthenticatorTest { @IgnoreUpTo(Build.VERSION_CODES.TIRAMISU) public void testNetworkCapabilitiesContainTwoSubIds() throws Exception { final CarrierPrivilegesListenerShim listener = getCarrierPrivilegesListeners().get(0); - listener.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + visibleOnHandlerThread(mCsHandler, () -> { + listener.onCarrierServiceChanged(null, mCarrierConfigPkgUid); + }); final NetworkCapabilities.Builder ncBuilder = new NetworkCapabilities.Builder(); ncBuilder.addTransportType(TRANSPORT_WIFI); diff --git a/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt b/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt index b0fa7ff859..3fc57b47ee 100644 --- a/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt +++ b/tests/unit/java/com/android/server/connectivityservice/base/CSTest.kt @@ -234,7 +234,8 @@ open class CSTest { context: Context, tm: TelephonyManager, requestRestrictedWifiEnabled: Boolean, - listener: BiConsumer + listener: BiConsumer, + handler: Handler ) = if (SdkLevel.isAtLeastT()) mock() else null var satelliteNetworkFallbackUidUpdate: Consumer>? = null