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); }