From f183b69cf57f037987fca8a5abdb448f90b18612 Mon Sep 17 00:00:00 2001 From: Nathan Harold Date: Wed, 20 Oct 2021 12:42:27 -0700 Subject: Ensure that ScanInfo is cached before callbacks fire Fixes a regression that was introduced in aosp/1312940. There is a critical section in requestNetworkScan after the scanId has been generated and before the ScanInfo is added to the cache. If during this time a callback fires on a separate thread, it will not yet find the ScanInfo record, and due to strict checking, this causes a RuntimeException. This CL re-expands the synchronized block to cover the critical section, and clarifies a little bounds checking. Note: this change is not ideal. This means that all callbacks for all scans will be blocked while a request for a new scan is being made, which was the motivation behind the change that introduced this regression. But, a narrow performance hit is better than a crash, and it's not worth the complexity of a bigger fix. Bug: 200634560 Test: compilation Merged-In: I4670da109256170121ceb6d8fbad0efda310335f Change-Id: I4670da109256170121ceb6d8fbad0efda310335f --- .../android/telephony/TelephonyScanManager.java | 35 +++++++++++----------- 1 file changed, 18 insertions(+), 17 deletions(-) (limited to 'telephony/java/android') diff --git a/telephony/java/android/telephony/TelephonyScanManager.java b/telephony/java/android/telephony/TelephonyScanManager.java index e890acb36b48..9572154ec773 100644 --- a/telephony/java/android/telephony/TelephonyScanManager.java +++ b/telephony/java/android/telephony/TelephonyScanManager.java @@ -36,6 +36,7 @@ import com.android.telephony.Rlog; import java.util.Arrays; import java.util.List; +import java.util.Objects; import java.util.concurrent.Executor; /** @@ -152,16 +153,9 @@ public final class TelephonyScanManager { throw new RuntimeException( "Failed to find NetworkScanInfo with id " + message.arg2); } - NetworkScanCallback callback = nsi.mCallback; - Executor executor = nsi.mExecutor; - if (callback == null) { - throw new RuntimeException( - "Failed to find NetworkScanCallback with id " + message.arg2); - } - if (executor == null) { - throw new RuntimeException( - "Failed to find Executor with id " + message.arg2); - } + + final NetworkScanCallback callback = nsi.mCallback; + final Executor executor = nsi.mExecutor; switch (message.what) { case CALLBACK_RESTRICTED_SCAN_RESULTS: @@ -246,17 +240,24 @@ public final class TelephonyScanManager { NetworkScanRequest request, Executor executor, NetworkScanCallback callback, String callingPackage, @Nullable String callingFeatureId) { try { + Objects.requireNonNull(request, "Request was null"); + Objects.requireNonNull(callback, "Callback was null"); + Objects.requireNonNull(executor, "Executor was null"); final ITelephony telephony = getITelephony(); if (telephony == null) return null; - int scanId = telephony.requestNetworkScan( - subId, request, mMessenger, new Binder(), callingPackage, - callingFeatureId); - if (scanId == INVALID_SCAN_ID) { - Rlog.e(TAG, "Failed to initiate network scan"); - return null; - } + // The lock must be taken before calling requestNetworkScan because the resulting + // scanId can be invoked asynchronously on another thread at any time after + // requestNetworkScan invoked, leaving a critical section between that call and adding + // the record to the ScanInfo cache. synchronized (mScanInfo) { + int scanId = telephony.requestNetworkScan( + subId, request, mMessenger, new Binder(), callingPackage, + callingFeatureId); + if (scanId == INVALID_SCAN_ID) { + Rlog.e(TAG, "Failed to initiate network scan"); + return null; + } // We link to death whenever a scan is started to ensure that we are linked // at the point that phone process death might matter. // We never unlink because: -- cgit v1.2.3