From 8c23380d2f3707346a853bbbeeb639355b7717fa Mon Sep 17 00:00:00 2001 From: Nathan Harold Date: Fri, 29 Jan 2021 16:10:29 -0800 Subject: Ensure Async APIs receive callbacks There are currently a couple silent failure for APIs that are defined as asynchronous. -If the remote binder object is null then in some cases the response is dropped. It's important that the response be triggered on a thread other than the calling one due to the oddball possibility of non-reentrant locking. -If the remote dies while awaiting a response, the RemoteException is generally dropped, and because the API is asynchronous there is no way for the caller to know, leaving a dangling async request. This CL ensures that remote exceptions due to process death invoke the asynchronous callbacks, delivering an error message using the BackgroundThread to ensure that the calls are received on a thread other than the original caller's. Bug: 182185642 Test: atest CtsTelephonyTestCases Change-Id: I5121734194f07c53f94fb6585da3c90e3e2c4bf7 --- .../java/android/telephony/TelephonyManager.java | 140 +++++++++++---------- 1 file changed, 73 insertions(+), 67 deletions(-) diff --git a/telephony/java/android/telephony/TelephonyManager.java b/telephony/java/android/telephony/TelephonyManager.java index 7c39cf0b47a6..2ae12ce22928 100644 --- a/telephony/java/android/telephony/TelephonyManager.java +++ b/telephony/java/android/telephony/TelephonyManager.java @@ -99,6 +99,7 @@ import android.util.Pair; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.os.BackgroundThread; import com.android.internal.telephony.CellNetworkScanResult; import com.android.internal.telephony.IBooleanConsumer; import com.android.internal.telephony.ICallForwardingInfoCallback; @@ -128,6 +129,7 @@ import java.util.Map; import java.util.Objects; import java.util.UUID; import java.util.concurrent.Executor; +import java.util.concurrent.RejectedExecutionException; import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.IntStream; @@ -416,6 +418,27 @@ public class TelephonyManager { return Process.myUid() == Process.SYSTEM_UID; } + /** + * Post a runnable to the BackgroundThread. + * + * Used to invoke user callbacks without calling into the caller's executor from the caller's + * calling thread context, for example to provide asynchronous error information that is + * generated locally (not over a binder thread). + * + *

This is not necessary unless you are invoking caller's code asynchronously from within + * the caller's thread context. + * + * @param r a runnable. + */ + private static void runOnBackgroundThread(@NonNull Runnable r) { + try { + BackgroundThread.getExecutor().execute(r); + } catch (RejectedExecutionException e) { + throw new IllegalStateException( + "Failed to post a callback from the caller's thread context.", e); + } + } + /** * Returns the multi SIM variant * Returns DSDS for Dual SIM Dual Standby @@ -5912,7 +5935,8 @@ public class TelephonyManager { @NonNull @CallbackExecutor Executor executor, @NonNull CellInfoCallback callback) { try { ITelephony telephony = getITelephony(); - if (telephony == null) return; + if (telephony == null) throw new IllegalStateException("Telephony is null"); + telephony.requestCellInfoUpdate( getSubId(), new ICellInfoCallback.Stub() { @@ -5939,6 +5963,8 @@ public class TelephonyManager { } }, getOpPackageName(), getAttributionTag()); } catch (RemoteException ex) { + runOnBackgroundThread(() -> executor.execute( + () -> callback.onError(CellInfoCallback.ERROR_MODEM_ERROR, ex))); } } @@ -5966,7 +5992,7 @@ public class TelephonyManager { @NonNull @CallbackExecutor Executor executor, @NonNull CellInfoCallback callback) { try { ITelephony telephony = getITelephony(); - if (telephony == null) return; + if (telephony == null) throw new IllegalStateException("Telephony is null"); telephony.requestCellInfoUpdateWithWorkSource( getSubId(), new ICellInfoCallback.Stub() { @@ -5994,6 +6020,8 @@ public class TelephonyManager { } }, getOpPackageName(), getAttributionTag(), workSource); } catch (RemoteException ex) { + runOnBackgroundThread(() -> executor.execute( + () -> callback.onError(CellInfoCallback.ERROR_MODEM_ERROR, ex))); } } @@ -6960,14 +6988,15 @@ public class TelephonyManager { try { ITelephony telephony = getITelephony(); - if (telephony != null) { - telephony.requestNumberVerification(range, timeoutMillis, internalCallback, - getOpPackageName()); - } + if (telephony == null) throw new IllegalStateException("Telephony is null"); + + telephony.requestNumberVerification(range, timeoutMillis, internalCallback, + getOpPackageName()); } catch (RemoteException ex) { Rlog.e(TAG, "requestNumberVerification RemoteException", ex); - executor.execute(() -> - callback.onVerificationFailed(NumberVerificationCallback.REASON_UNSPECIFIED)); + runOnBackgroundThread(() -> executor.execute( + () -> callback.onVerificationFailed( + NumberVerificationCallback.REASON_UNSPECIFIED))); } } @@ -10333,6 +10362,8 @@ public class TelephonyManager { } try { ITelephony telephony = getITelephony(); + if (telephony == null) throw new IllegalStateException("Telephony is null."); + IIntegerConsumer internalCallback = new IIntegerConsumer.Stub() { @Override public void accept(int result) { @@ -10340,11 +10371,11 @@ public class TelephonyManager { Binder.withCleanCallingIdentity(() -> callback.accept(result))); } }; - if (telephony != null) { - telephony.setSimPowerStateForSlotWithCallback(slotIndex, state, internalCallback); - } + telephony.setSimPowerStateForSlotWithCallback(slotIndex, state, internalCallback); } catch (RemoteException e) { Log.e(TAG, "Error calling ITelephony#setSimPowerStateForSlot", e); + runOnBackgroundThread(() -> executor.execute( + () -> callback.accept(SET_SIM_POWER_STATE_MODEM_ERROR))); } catch (SecurityException e) { Log.e(TAG, "Permission error calling ITelephony#setSimPowerStateForSlot", e); @@ -12774,22 +12805,7 @@ public class TelephonyManager { try { IOns iOpportunisticNetworkService = getIOns(); if (iOpportunisticNetworkService == null) { - if (executor == null || callback == null) { - return; - } - final long identity = Binder.clearCallingIdentity(); - try { - executor.execute(() -> { - if (Compatibility.isChangeEnabled(CALLBACK_ON_MORE_ERROR_CODE_CHANGE)) { - callback.accept(SET_OPPORTUNISTIC_SUB_REMOTE_SERVICE_EXCEPTION); - } else { - callback.accept(SET_OPPORTUNISTIC_SUB_INACTIVE_SUBSCRIPTION); - } - }); - } finally { - Binder.restoreCallingIdentity(identity); - } - return; + throw new IllegalStateException("Opportunistic Network Service is null"); } ISetOpportunisticDataCallback callbackStub = new ISetOpportunisticDataCallback.Stub() { @Override @@ -12812,9 +12828,18 @@ public class TelephonyManager { .setPreferredDataSubscriptionId(subId, needValidation, callbackStub, pkgForDebug); } catch (RemoteException ex) { - Rlog.e(TAG, "setPreferredDataSubscriptionId RemoteException", ex); + Rlog.e(TAG, "setPreferredOpportunisticDataSubscription RemoteException", ex); + if (executor == null || callback == null) { + return; + } + runOnBackgroundThread(() -> executor.execute(() -> { + if (Compatibility.isChangeEnabled(CALLBACK_ON_MORE_ERROR_CODE_CHANGE)) { + callback.accept(SET_OPPORTUNISTIC_SUB_REMOTE_SERVICE_EXCEPTION); + } else { + callback.accept(SET_OPPORTUNISTIC_SUB_INACTIVE_SUBSCRIPTION); + } + })); } - return; } /** @@ -12871,37 +12896,13 @@ public class TelephonyManager { @Nullable @CallbackExecutor Executor executor, @UpdateAvailableNetworksResult @Nullable Consumer callback) { String pkgForDebug = mContext != null ? mContext.getOpPackageName() : ""; + Objects.requireNonNull(availableNetworks, "availableNetworks must not be null."); try { IOns iOpportunisticNetworkService = getIOns(); - if (iOpportunisticNetworkService == null || availableNetworks == null) { - if (executor == null || callback == null) { - return; - } - if (iOpportunisticNetworkService == null) { - final long identity = Binder.clearCallingIdentity(); - try { - executor.execute(() -> { - if (Compatibility.isChangeEnabled(CALLBACK_ON_MORE_ERROR_CODE_CHANGE)) { - callback.accept(UPDATE_AVAILABLE_NETWORKS_REMOTE_SERVICE_EXCEPTION); - } else { - callback.accept(UPDATE_AVAILABLE_NETWORKS_UNKNOWN_FAILURE); - } - }); - } finally { - Binder.restoreCallingIdentity(identity); - } - } else { - final long identity = Binder.clearCallingIdentity(); - try { - executor.execute(() -> { - callback.accept(UPDATE_AVAILABLE_NETWORKS_INVALID_ARGUMENTS); - }); - } finally { - Binder.restoreCallingIdentity(identity); - } - } - return; + if (iOpportunisticNetworkService == null) { + throw new IllegalStateException("Opportunistic Network Service is null"); } + IUpdateAvailableNetworksCallback callbackStub = new IUpdateAvailableNetworksCallback.Stub() { @Override @@ -12909,20 +12910,25 @@ public class TelephonyManager { if (executor == null || callback == null) { return; } - final long identity = Binder.clearCallingIdentity(); - try { - executor.execute(() -> { - callback.accept(result); - }); - } finally { - Binder.restoreCallingIdentity(identity); - } + Binder.withCleanCallingIdentity(() -> { + executor.execute(() -> callback.accept(result)); + }); } }; - iOpportunisticNetworkService.updateAvailableNetworks(availableNetworks, callbackStub, - pkgForDebug); + iOpportunisticNetworkService + .updateAvailableNetworks(availableNetworks, callbackStub, pkgForDebug); } catch (RemoteException ex) { Rlog.e(TAG, "updateAvailableNetworks RemoteException", ex); + if (executor == null || callback == null) { + return; + } + runOnBackgroundThread(() -> executor.execute(() -> { + if (Compatibility.isChangeEnabled(CALLBACK_ON_MORE_ERROR_CODE_CHANGE)) { + callback.accept(UPDATE_AVAILABLE_NETWORKS_REMOTE_SERVICE_EXCEPTION); + } else { + callback.accept(UPDATE_AVAILABLE_NETWORKS_UNKNOWN_FAILURE); + } + })); } } -- cgit v1.2.3-59-g8ed1b From 31c7d6162472077c20760025128271b8aa9f5a21 Mon Sep 17 00:00:00 2001 From: Nathan Harold Date: Wed, 24 Mar 2021 17:41:01 -0700 Subject: Add a Compat Flag for Throwing For Null Telephony In order to prevent a sea of random crashes as part of migrating to throwing behavior for callback APIs that hit null ITelephony and null IOns objects, this change re-introduces backwards-compatible behavior for APIs that are being "upgraded" to throwing behavior. Bug: 182185642 Test: atest CtsTelephonyTestCases Change-Id: I492ba6a79e74350986afbd6ab5cc5e7b05b8cd35 --- .../java/android/telephony/TelephonyManager.java | 62 +++++++++++++++++++--- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/telephony/java/android/telephony/TelephonyManager.java b/telephony/java/android/telephony/TelephonyManager.java index 2ae12ce22928..a2467f2067ac 100644 --- a/telephony/java/android/telephony/TelephonyManager.java +++ b/telephony/java/android/telephony/TelephonyManager.java @@ -5898,7 +5898,7 @@ public class TelephonyManager { /** * Error response to - * {@link android.telephony.TelephonyManager#requestCellInfoUpdate requestCellInfoUpdate()}. + * {@link TelephonyManager#requestCellInfoUpdate requestCellInfoUpdate()}. * * Invoked when an error condition prevents updated {@link CellInfo} from being fetched * and returned from the modem. Callers of requestCellInfoUpdate() should override this @@ -5915,6 +5915,20 @@ public class TelephonyManager { } }; + /** + * Used for checking if the target SDK version for the current process is S or above. + * + *

Applies to the following methods: + * {@link #requestCellInfoUpdate}, + * {@link #setPreferredOpportunisticDataSubscription}, + * {@link #updateAvailableNetworks}, + * requestNumberVerification(), + * setSimPowerStateForSlot(), + */ + @ChangeId + @EnabledAfter(targetSdkVersion = Build.VERSION_CODES.R) + private static final long NULL_TELEPHONY_THROW_NO_CB = 182185642L; + /** * Requests all available cell information from the current subscription for observed * camped/registered, serving, and neighboring cells. @@ -5935,7 +5949,13 @@ public class TelephonyManager { @NonNull @CallbackExecutor Executor executor, @NonNull CellInfoCallback callback) { try { ITelephony telephony = getITelephony(); - if (telephony == null) throw new IllegalStateException("Telephony is null"); + if (telephony == null) { + if (Compatibility.isChangeEnabled(NULL_TELEPHONY_THROW_NO_CB)) { + throw new IllegalStateException("Telephony is null"); + } else { + return; + } + } telephony.requestCellInfoUpdate( getSubId(), @@ -5992,7 +6012,14 @@ public class TelephonyManager { @NonNull @CallbackExecutor Executor executor, @NonNull CellInfoCallback callback) { try { ITelephony telephony = getITelephony(); - if (telephony == null) throw new IllegalStateException("Telephony is null"); + if (telephony == null) { + if (Compatibility.isChangeEnabled(NULL_TELEPHONY_THROW_NO_CB)) { + throw new IllegalStateException("Telephony is null"); + } else { + return; + } + } + telephony.requestCellInfoUpdateWithWorkSource( getSubId(), new ICellInfoCallback.Stub() { @@ -6988,7 +7015,13 @@ public class TelephonyManager { try { ITelephony telephony = getITelephony(); - if (telephony == null) throw new IllegalStateException("Telephony is null"); + if (telephony == null) { + if (Compatibility.isChangeEnabled(NULL_TELEPHONY_THROW_NO_CB)) { + throw new IllegalStateException("Telephony is null"); + } else { + return; + } + } telephony.requestNumberVerification(range, timeoutMillis, internalCallback, getOpPackageName()); @@ -10371,6 +10404,13 @@ public class TelephonyManager { Binder.withCleanCallingIdentity(() -> callback.accept(result))); } }; + if (telephony == null) { + if (Compatibility.isChangeEnabled(NULL_TELEPHONY_THROW_NO_CB)) { + throw new IllegalStateException("Telephony is null"); + } else { + return; + } + } telephony.setSimPowerStateForSlotWithCallback(slotIndex, state, internalCallback); } catch (RemoteException e) { Log.e(TAG, "Error calling ITelephony#setSimPowerStateForSlot", e); @@ -12805,7 +12845,12 @@ public class TelephonyManager { try { IOns iOpportunisticNetworkService = getIOns(); if (iOpportunisticNetworkService == null) { - throw new IllegalStateException("Opportunistic Network Service is null"); + if (Compatibility.isChangeEnabled(NULL_TELEPHONY_THROW_NO_CB)) { + throw new IllegalStateException("Opportunistic Network Service is null"); + } else { + // Let the general remote exception handling catch this. + throw new RemoteException("Null Opportunistic Network Service!"); + } } ISetOpportunisticDataCallback callbackStub = new ISetOpportunisticDataCallback.Stub() { @Override @@ -12900,7 +12945,12 @@ public class TelephonyManager { try { IOns iOpportunisticNetworkService = getIOns(); if (iOpportunisticNetworkService == null) { - throw new IllegalStateException("Opportunistic Network Service is null"); + if (Compatibility.isChangeEnabled(NULL_TELEPHONY_THROW_NO_CB)) { + throw new IllegalStateException("Opportunistic Network Service is null"); + } else { + // Let the general remote exception handling catch this. + throw new RemoteException("Null Opportunistic Network Service!"); + } } IUpdateAvailableNetworksCallback callbackStub = -- cgit v1.2.3-59-g8ed1b