From 2da50cfe339113e334e61efc0d4927cf6458aa8b Mon Sep 17 00:00:00 2001 From: Hall Liu Date: Wed, 18 Mar 2020 16:24:04 -0700 Subject: [PATCH 1/2] Institute limit on PhoneStateListener Limit apps to 50 concurrently registered instances of PhoneStateListener via TelephonyManager#listen Test: atest CtsTelephonyTestCases:PhoneStateListenerTest#testListenerLimit Bug: 151835251 Change-Id: I8486d86773a1e28b4018620c48003855dae75b9d --- .../android/telephony/PhoneStateListener.java | 12 ++++++++ .../com/android/server/TelephonyRegistry.java | 30 ++++++++++++++++--- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/core/java/android/telephony/PhoneStateListener.java b/core/java/android/telephony/PhoneStateListener.java index d2735008611c0..a2af15866d668 100644 --- a/core/java/android/telephony/PhoneStateListener.java +++ b/core/java/android/telephony/PhoneStateListener.java @@ -64,6 +64,18 @@ public class PhoneStateListener { private static final String LOG_TAG = "PhoneStateListener"; private static final boolean DBG = false; // STOPSHIP if true + + /** + * Limit on registrations of {@link PhoneStateListener}s on a per-pid + * basis. When this limit is exceeded, any calls to {@link TelephonyManager#listen} will fail + * with an {@link IllegalStateException}. + * + * {@link android.os.Process#PHONE_UID}, {@link android.os.Process#SYSTEM_UID}, and the uid that + * TelephonyRegistry runs under are exempt from this limit. + * @hide + */ + public static final int PER_PID_REGISTRATION_LIMIT = 50; + /** * Stop listening for updates. * diff --git a/services/core/java/com/android/server/TelephonyRegistry.java b/services/core/java/com/android/server/TelephonyRegistry.java index 7dedad7f2fea6..ae379e1858fff 100644 --- a/services/core/java/com/android/server/TelephonyRegistry.java +++ b/services/core/java/com/android/server/TelephonyRegistry.java @@ -39,6 +39,7 @@ import android.os.Bundle; import android.os.Handler; import android.os.IBinder; import android.os.Message; +import android.os.Process; import android.os.RemoteException; import android.os.UserHandle; import android.telephony.Annotation; @@ -605,7 +606,7 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { synchronized (mRecords) { // register IBinder b = callback.asBinder(); - Record r = add(b); + Record r = add(b, Binder.getCallingPid(), false); if (r == null) { return; @@ -659,7 +660,7 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { synchronized (mRecords) { // register IBinder b = callback.asBinder(); - Record r = add(b); + Record r = add(b, Binder.getCallingPid(), false); if (r == null) { return; @@ -789,7 +790,11 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { synchronized (mRecords) { // register IBinder b = callback.asBinder(); - Record r = add(b); + boolean shouldEnforceListenerLimit = + Binder.getCallingUid() != Process.SYSTEM_UID + && Binder.getCallingUid() != Process.PHONE_UID + && Binder.getCallingUid() != Process.myUid(); + Record r = add(b, Binder.getCallingPid(), shouldEnforceListenerLimit); if (r == null) { return; @@ -1084,18 +1089,35 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { return record.canReadCallLog() ? mCallIncomingNumber[phoneId] : ""; } - private Record add(IBinder binder) { + private Record add(IBinder binder, int callingPid, boolean enforceLimit) { Record r; synchronized (mRecords) { final int N = mRecords.size(); + // While iterating through the records, keep track of how many we have from this pid. + int numRecordsForPid = 0; for (int i = 0; i < N; i++) { r = mRecords.get(i); if (binder == r.binder) { // Already existed. return r; } + if (r.callerPid == callingPid) { + numRecordsForPid++; + } } + // If we've exceeded the limit for registrations, log a warning and quit. + if (enforceLimit && numRecordsForPid >= PhoneStateListener.PER_PID_REGISTRATION_LIMIT) { + String errorMsg = "Pid " + callingPid + " has exceeded the number of permissible" + + "registered listeners. Ignoring request to add."; + loge(errorMsg); + throw new IllegalStateException(errorMsg); + } else if (enforceLimit + && numRecordsForPid >= PhoneStateListener.PER_PID_REGISTRATION_LIMIT / 2) { + Rlog.w(TAG, "Pid " + callingPid + " has exceeded half the number of permissible" + + "registered listeners. Now at " + numRecordsForPid); + } + r = new Record(); r.binder = binder; r.deathRecipient = new TelephonyRegistryDeathRecipient(binder); From ed6d6e69a86d8ab96b732903fa03d01bc1fa9508 Mon Sep 17 00:00:00 2001 From: Hall Liu Date: Thu, 26 Mar 2020 19:09:30 -0700 Subject: [PATCH 2/2] Add gating, logging for PhoneStateListener's limit Add gating via PlatformCompat and DeviceConfig and logging via PlatformCompat to the limit instituted on per-process listeners Fixes: 152074216 Test: atest CtsTelephonyHostCases Change-Id: I4d6681d90705b68c3349f4124e434a29b50fd3a2 --- .../android/telephony/PhoneStateListener.java | 30 ++++++++- .../com/android/server/TelephonyRegistry.java | 64 +++++++++++++++---- .../java/com/android/server/SystemServer.java | 3 +- .../android/telephony/TelephonyManager.java | 4 ++ .../server/ConnectivityServiceTest.java | 3 + 5 files changed, 90 insertions(+), 14 deletions(-) diff --git a/core/java/android/telephony/PhoneStateListener.java b/core/java/android/telephony/PhoneStateListener.java index a2af15866d668..a732ca55a414f 100644 --- a/core/java/android/telephony/PhoneStateListener.java +++ b/core/java/android/telephony/PhoneStateListener.java @@ -21,6 +21,7 @@ import android.annotation.NonNull; import android.annotation.RequiresPermission; import android.annotation.SystemApi; import android.annotation.TestApi; +import android.compat.annotation.ChangeId; import android.compat.annotation.UnsupportedAppUsage; import android.os.Binder; import android.os.Build; @@ -64,17 +65,42 @@ public class PhoneStateListener { private static final String LOG_TAG = "PhoneStateListener"; private static final boolean DBG = false; // STOPSHIP if true - /** + * Experiment flag to set the per-pid registration limit for PhoneStateListeners + * * Limit on registrations of {@link PhoneStateListener}s on a per-pid * basis. When this limit is exceeded, any calls to {@link TelephonyManager#listen} will fail * with an {@link IllegalStateException}. * * {@link android.os.Process#PHONE_UID}, {@link android.os.Process#SYSTEM_UID}, and the uid that * TelephonyRegistry runs under are exempt from this limit. + * + * If the value of the flag is less than 1, enforcement of the limit will be disabled. * @hide */ - public static final int PER_PID_REGISTRATION_LIMIT = 50; + public static final String FLAG_PER_PID_REGISTRATION_LIMIT = + "phone_state_listener_per_pid_registration_limit"; + + /** + * Default value for the per-pid registation limit. + * See {@link #FLAG_PER_PID_REGISTRATION_LIMIT}. + * @hide + */ + public static final int DEFAULT_PER_PID_REGISTRATION_LIMIT = 50; + + /** + * This change enables a limit on the number of {@link PhoneStateListener} objects any process + * may register via {@link TelephonyManager#listen}. The default limit is 50, which may change + * via remote device config updates. + * + * This limit is enforced via an {@link IllegalStateException} thrown from + * {@link TelephonyManager#listen} when the offending process attempts to register one too many + * listeners. + * + * @hide + */ + @ChangeId + public static final long PHONE_STATE_LISTENER_LIMIT_CHANGE_ID = 150880553L; /** * Stop listening for updates. diff --git a/services/core/java/com/android/server/TelephonyRegistry.java b/services/core/java/com/android/server/TelephonyRegistry.java index ae379e1858fff..5d45e31f2c4a0 100644 --- a/services/core/java/com/android/server/TelephonyRegistry.java +++ b/services/core/java/com/android/server/TelephonyRegistry.java @@ -27,6 +27,7 @@ import android.annotation.NonNull; import android.annotation.Nullable; import android.app.ActivityManager; import android.app.AppOpsManager; +import android.app.compat.CompatChanges; import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; @@ -42,6 +43,7 @@ import android.os.Message; import android.os.Process; import android.os.RemoteException; import android.os.UserHandle; +import android.provider.DeviceConfig; import android.telephony.Annotation; import android.telephony.Annotation.ApnType; import android.telephony.Annotation.DataFailureCause; @@ -178,8 +180,38 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { } } + /** + * Wrapper class to facilitate testing -- encapsulates bits of configuration that are + * normally fetched from static methods with many dependencies. + */ + public static class ConfigurationProvider { + /** + * @return The per-pid registration limit for PhoneStateListeners, as set from DeviceConfig + * @noinspection ConstantConditions + */ + public int getRegistrationLimit() { + return Binder.withCleanCallingIdentity(() -> + DeviceConfig.getInt(DeviceConfig.NAMESPACE_TELEPHONY, + PhoneStateListener.FLAG_PER_PID_REGISTRATION_LIMIT, + PhoneStateListener.DEFAULT_PER_PID_REGISTRATION_LIMIT)); + } + + /** + * @param uid uid to check + * @return Whether enforcement of the per-pid registation limit for PhoneStateListeners is + * enabled in PlatformCompat for the given uid. + * @noinspection ConstantConditions + */ + public boolean isRegistrationLimitEnabledInPlatformCompat(int uid) { + return Binder.withCleanCallingIdentity(() -> CompatChanges.isChangeEnabled( + PhoneStateListener.PHONE_STATE_LISTENER_LIMIT_CHANGE_ID, uid)); + } + } + private final Context mContext; + private ConfigurationProvider mConfigurationProvider; + // access should be inside synchronized (mRecords) for these two fields private final ArrayList mRemoveList = new ArrayList(); private final ArrayList mRecords = new ArrayList(); @@ -507,10 +539,11 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { // handler before they get to app code. @VisibleForTesting(visibility = VisibleForTesting.Visibility.PACKAGE) - public TelephonyRegistry(Context context) { + public TelephonyRegistry(Context context, ConfigurationProvider configurationProvider) { CellLocation location = CellLocation.getEmpty(); mContext = context; + mConfigurationProvider = configurationProvider; mBatteryStats = BatteryStatsService.getService(); int numPhones = getTelephonyManager().getActiveModemCount(); @@ -606,7 +639,7 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { synchronized (mRecords) { // register IBinder b = callback.asBinder(); - Record r = add(b, Binder.getCallingPid(), false); + Record r = add(b, Binder.getCallingUid(), Binder.getCallingPid(), false); if (r == null) { return; @@ -660,7 +693,7 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { synchronized (mRecords) { // register IBinder b = callback.asBinder(); - Record r = add(b, Binder.getCallingPid(), false); + Record r = add(b, Binder.getCallingUid(), Binder.getCallingPid(), false); if (r == null) { return; @@ -790,11 +823,11 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { synchronized (mRecords) { // register IBinder b = callback.asBinder(); - boolean shouldEnforceListenerLimit = + boolean doesLimitApply = Binder.getCallingUid() != Process.SYSTEM_UID && Binder.getCallingUid() != Process.PHONE_UID && Binder.getCallingUid() != Process.myUid(); - Record r = add(b, Binder.getCallingPid(), shouldEnforceListenerLimit); + Record r = add(b, Binder.getCallingUid(), Binder.getCallingPid(), doesLimitApply); if (r == null) { return; @@ -1089,7 +1122,7 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { return record.canReadCallLog() ? mCallIncomingNumber[phoneId] : ""; } - private Record add(IBinder binder, int callingPid, boolean enforceLimit) { + private Record add(IBinder binder, int callingUid, int callingPid, boolean doesLimitApply) { Record r; synchronized (mRecords) { @@ -1106,14 +1139,23 @@ public class TelephonyRegistry extends ITelephonyRegistry.Stub { numRecordsForPid++; } } - // If we've exceeded the limit for registrations, log a warning and quit. - if (enforceLimit && numRecordsForPid >= PhoneStateListener.PER_PID_REGISTRATION_LIMIT) { + // If we've exceeded the limit for registrations, log an error and quit. + int registrationLimit = mConfigurationProvider.getRegistrationLimit(); + + if (doesLimitApply + && registrationLimit >= 1 + && numRecordsForPid >= registrationLimit) { String errorMsg = "Pid " + callingPid + " has exceeded the number of permissible" + "registered listeners. Ignoring request to add."; loge(errorMsg); - throw new IllegalStateException(errorMsg); - } else if (enforceLimit - && numRecordsForPid >= PhoneStateListener.PER_PID_REGISTRATION_LIMIT / 2) { + if (mConfigurationProvider + .isRegistrationLimitEnabledInPlatformCompat(callingUid)) { + throw new IllegalStateException(errorMsg); + } + } else if (doesLimitApply && numRecordsForPid + >= PhoneStateListener.DEFAULT_PER_PID_REGISTRATION_LIMIT / 2) { + // Log the warning independently of the dynamically set limit -- apps shouldn't be + // doing this regardless of whether we're throwing them an exception for it. Rlog.w(TAG, "Pid " + callingPid + " has exceeded half the number of permissible" + "registered listeners. Now at " + numRecordsForPid); } diff --git a/services/java/com/android/server/SystemServer.java b/services/java/com/android/server/SystemServer.java index b019e9dd03bae..4383e92c9a05c 100644 --- a/services/java/com/android/server/SystemServer.java +++ b/services/java/com/android/server/SystemServer.java @@ -1068,7 +1068,8 @@ public final class SystemServer { t.traceEnd(); t.traceBegin("StartTelephonyRegistry"); - telephonyRegistry = new TelephonyRegistry(context); + telephonyRegistry = new TelephonyRegistry( + context, new TelephonyRegistry.ConfigurationProvider()); ServiceManager.addService("telephony.registry", telephonyRegistry); t.traceEnd(); diff --git a/telephony/java/android/telephony/TelephonyManager.java b/telephony/java/android/telephony/TelephonyManager.java index 610ec5eee897b..e2889a289217e 100644 --- a/telephony/java/android/telephony/TelephonyManager.java +++ b/telephony/java/android/telephony/TelephonyManager.java @@ -5560,6 +5560,10 @@ public class TelephonyManager { * call {@link android.os.Binder#clearCallingIdentity()} before calling this method. A * {@link SecurityException} will be thrown otherwise. * + * This API should be used sparingly -- large numbers of listeners will cause system + * instability. If a process has registered too many listeners without unregistering them, it + * may encounter an {@link IllegalStateException} when trying to register more listeners. + * * @param listener The {@link PhoneStateListener} object to register * (or unregister) * @param events The telephony state(s) of interest to the listener, diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index 6985415a6a37b..3904b6f8b58fb 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -205,6 +205,7 @@ import android.os.UserManager; import android.provider.Settings; import android.security.KeyStore; import android.system.Os; +import android.telephony.TelephonyManager; import android.test.mock.MockContentResolver; import android.text.TextUtils; import android.util.ArraySet; @@ -345,6 +346,7 @@ public class ConnectivityServiceTest { @Mock IBinder mIBinder; @Mock LocationManager mLocationManager; @Mock AppOpsManager mAppOpsManager; + @Mock TelephonyManager mTelephonyManager; private ArgumentCaptor mResolverParamsParcelCaptor = ArgumentCaptor.forClass(ResolverParamsParcel.class); @@ -432,6 +434,7 @@ public class ConnectivityServiceTest { if (Context.ALARM_SERVICE.equals(name)) return mAlarmManager; if (Context.LOCATION_SERVICE.equals(name)) return mLocationManager; if (Context.APP_OPS_SERVICE.equals(name)) return mAppOpsManager; + if (Context.TELEPHONY_SERVICE.equals(name)) return mTelephonyManager; return super.getSystemService(name); }