Use callback to run binder service to avoid race
The binder interface should always be managed by ServiceWatcher, and
shouldn't be exposed to the caller. Otherwise the caller could cache an
unbound service, and could cause system crash.
Bug: 69008332
Test: build and manual test
Change-Id: I70d1fe0bace7b0fb1c3844c2c9113b1fcf95f784
diff --git a/services/core/java/com/android/server/ServiceWatcher.java b/services/core/java/com/android/server/ServiceWatcher.java
index 2ff036b..f20ca43 100644
--- a/services/core/java/com/android/server/ServiceWatcher.java
+++ b/services/core/java/com/android/server/ServiceWatcher.java
@@ -16,6 +16,7 @@
package com.android.server;
+import android.annotation.NonNull;
import android.annotation.Nullable;
import android.content.BroadcastReceiver;
import android.content.ComponentName;
@@ -397,9 +398,29 @@
}
}
- public @Nullable IBinder getBinder() {
+ /**
+ * The runner that runs on the binder retrieved from {@link ServiceWatcher}.
+ */
+ public interface BinderRunner {
+ /**
+ * Runs on the retrieved binder.
+ * @param binder the binder retrieved from the {@link ServiceWatcher}.
+ */
+ public void run(@NonNull IBinder binder);
+ }
+
+ /**
+ * Retrieves the binder from {@link ServiceWatcher} and runs it.
+ * @return whether a valid service exists.
+ */
+ public boolean runOnBinder(@NonNull BinderRunner runner) {
synchronized (mLock) {
- return mBoundService;
+ if (mBoundService == null) {
+ return false;
+ } else {
+ runner.run(mBoundService);
+ return true;
+ }
}
}
diff --git a/services/core/java/com/android/server/location/ActivityRecognitionProxy.java b/services/core/java/com/android/server/location/ActivityRecognitionProxy.java
index 55222dc..f82b976 100644
--- a/services/core/java/com/android/server/location/ActivityRecognitionProxy.java
+++ b/services/core/java/com/android/server/location/ActivityRecognitionProxy.java
@@ -103,51 +103,55 @@
* Helper function to bind the FusedLocationHardware to the appropriate FusedProvider instance.
*/
private void bindProvider() {
- IBinder binder = mServiceWatcher.getBinder();
- if (binder == null) {
- Log.e(TAG, "Null binder found on connection.");
- return;
- }
- String descriptor;
- try {
- descriptor = binder.getInterfaceDescriptor();
- } catch (RemoteException e) {
- Log.e(TAG, "Unable to get interface descriptor.", e);
- return;
- }
+ if (!mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ String descriptor;
+ try {
+ descriptor = binder.getInterfaceDescriptor();
+ } catch (RemoteException e) {
+ Log.e(TAG, "Unable to get interface descriptor.", e);
+ return;
+ }
- if (IActivityRecognitionHardwareWatcher.class.getCanonicalName().equals(descriptor)) {
- IActivityRecognitionHardwareWatcher watcher =
- IActivityRecognitionHardwareWatcher.Stub.asInterface(binder);
- if (watcher == null) {
- Log.e(TAG, "No watcher found on connection.");
- return;
+ if (IActivityRecognitionHardwareWatcher.class.getCanonicalName()
+ .equals(descriptor)) {
+ IActivityRecognitionHardwareWatcher watcher =
+ IActivityRecognitionHardwareWatcher.Stub.asInterface(binder);
+ if (watcher == null) {
+ Log.e(TAG, "No watcher found on connection.");
+ return;
+ }
+ if (mInstance == null) {
+ // to keep backwards compatibility do not update the watcher when there is
+ // no instance available, or it will cause an NPE
+ Log.d(TAG, "AR HW instance not available, binding will be a no-op.");
+ return;
+ }
+ try {
+ watcher.onInstanceChanged(mInstance);
+ } catch (RemoteException e) {
+ Log.e(TAG, "Error delivering hardware interface to watcher.", e);
+ }
+ } else if (IActivityRecognitionHardwareClient.class.getCanonicalName()
+ .equals(descriptor)) {
+ IActivityRecognitionHardwareClient client =
+ IActivityRecognitionHardwareClient.Stub.asInterface(binder);
+ if (client == null) {
+ Log.e(TAG, "No client found on connection.");
+ return;
+ }
+ try {
+ client.onAvailabilityChanged(mIsSupported, mInstance);
+ } catch (RemoteException e) {
+ Log.e(TAG, "Error delivering hardware interface to client.", e);
+ }
+ } else {
+ Log.e(TAG, "Invalid descriptor found on connection: " + descriptor);
+ }
}
- if (mInstance == null) {
- // to keep backwards compatibility do not update the watcher when there is no
- // instance available, or it will cause an NPE
- Log.d(TAG, "AR HW instance not available, binding will be a no-op.");
- return;
- }
- try {
- watcher.onInstanceChanged(mInstance);
- } catch (RemoteException e) {
- Log.e(TAG, "Error delivering hardware interface to watcher.", e);
- }
- } else if (IActivityRecognitionHardwareClient.class.getCanonicalName().equals(descriptor)) {
- IActivityRecognitionHardwareClient client =
- IActivityRecognitionHardwareClient.Stub.asInterface(binder);
- if (client == null) {
- Log.e(TAG, "No client found on connection.");
- return;
- }
- try {
- client.onAvailabilityChanged(mIsSupported, mInstance);
- } catch (RemoteException e) {
- Log.e(TAG, "Error delivering hardware interface to client.", e);
- }
- } else {
- Log.e(TAG, "Invalid descriptor found on connection: " + descriptor);
+ })) {
+ Log.e(TAG, "Null binder found on connection.");
}
}
}
diff --git a/services/core/java/com/android/server/location/FusedProxy.java b/services/core/java/com/android/server/location/FusedProxy.java
index f7fac77..b90f037 100644
--- a/services/core/java/com/android/server/location/FusedProxy.java
+++ b/services/core/java/com/android/server/location/FusedProxy.java
@@ -23,6 +23,7 @@
import android.hardware.location.IFusedLocationHardware;
import android.location.IFusedProvider;
import android.os.Handler;
+import android.os.IBinder;
import android.os.RemoteException;
import android.util.Log;
@@ -112,17 +113,18 @@
* @param locationHardware The FusedLocationHardware instance to use for the binding operation.
*/
private void bindProvider(IFusedLocationHardware locationHardware) {
- IFusedProvider provider = IFusedProvider.Stub.asInterface(mServiceWatcher.getBinder());
-
- if (provider == null) {
+ if (!mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ IFusedProvider provider = IFusedProvider.Stub.asInterface(binder);
+ try {
+ provider.onFusedLocationHardwareChange(locationHardware);
+ } catch (RemoteException e) {
+ Log.e(TAG, e.toString());
+ }
+ }
+ })) {
Log.e(TAG, "No instance of FusedProvider found on FusedLocationHardware connected.");
- return;
- }
-
- try {
- provider.onFusedLocationHardwareChange(locationHardware);
- } catch (RemoteException e) {
- Log.e(TAG, e.toString());
}
}
}
diff --git a/services/core/java/com/android/server/location/GeocoderProxy.java b/services/core/java/com/android/server/location/GeocoderProxy.java
index 422b94b..9ad4aa1 100644
--- a/services/core/java/com/android/server/location/GeocoderProxy.java
+++ b/services/core/java/com/android/server/location/GeocoderProxy.java
@@ -21,6 +21,7 @@
import android.location.GeocoderParams;
import android.location.IGeocodeProvider;
import android.os.Handler;
+import android.os.IBinder;
import android.os.RemoteException;
import android.util.Log;
@@ -63,42 +64,47 @@
return mServiceWatcher.start();
}
- private IGeocodeProvider getService() {
- return IGeocodeProvider.Stub.asInterface(mServiceWatcher.getBinder());
- }
-
public String getConnectedPackageName() {
return mServiceWatcher.getBestPackageName();
}
public String getFromLocation(double latitude, double longitude, int maxResults,
GeocoderParams params, List<Address> addrs) {
- IGeocodeProvider provider = getService();
- if (provider != null) {
- try {
- return provider.getFromLocation(latitude, longitude, maxResults, params, addrs);
- } catch (RemoteException e) {
- Log.w(TAG, e);
+ final String[] result = new String[] {"Service not Available"};
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ IGeocodeProvider provider = IGeocodeProvider.Stub.asInterface(binder);
+ try {
+ result[0] = provider.getFromLocation(
+ latitude, longitude, maxResults, params, addrs);
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ }
}
- }
- return "Service not Available";
+ });
+ return result[0];
}
public String getFromLocationName(String locationName,
double lowerLeftLatitude, double lowerLeftLongitude,
double upperRightLatitude, double upperRightLongitude, int maxResults,
GeocoderParams params, List<Address> addrs) {
- IGeocodeProvider provider = getService();
- if (provider != null) {
- try {
- return provider.getFromLocationName(locationName, lowerLeftLatitude,
- lowerLeftLongitude, upperRightLatitude, upperRightLongitude,
- maxResults, params, addrs);
- } catch (RemoteException e) {
- Log.w(TAG, e);
+ final String[] result = new String[] {"Service not Available"};
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ IGeocodeProvider provider = IGeocodeProvider.Stub.asInterface(binder);
+ try {
+ result[0] = provider.getFromLocationName(locationName, lowerLeftLatitude,
+ lowerLeftLongitude, upperRightLatitude, upperRightLongitude,
+ maxResults, params, addrs);
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ }
}
- }
- return "Service not Available";
+ });
+ return result[0];
}
}
diff --git a/services/core/java/com/android/server/location/GeofenceProxy.java b/services/core/java/com/android/server/location/GeofenceProxy.java
index b3a0010..eb47b2f 100644
--- a/services/core/java/com/android/server/location/GeofenceProxy.java
+++ b/services/core/java/com/android/server/location/GeofenceProxy.java
@@ -116,15 +116,17 @@
};
private void setGeofenceHardwareInProviderLocked() {
- try {
- IGeofenceProvider provider = IGeofenceProvider.Stub.asInterface(
- mServiceWatcher.getBinder());
- if (provider != null) {
- provider.setGeofenceHardware(mGeofenceHardware);
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ final IGeofenceProvider provider = IGeofenceProvider.Stub.asInterface(binder);
+ try {
+ provider.setGeofenceHardware(mGeofenceHardware);
+ } catch (RemoteException e) {
+ Log.e(TAG, "Remote Exception: setGeofenceHardwareInProviderLocked: " + e);
+ }
}
- } catch (RemoteException e) {
- Log.e(TAG, "Remote Exception: setGeofenceHardwareInProviderLocked: " + e);
- }
+ });
}
private void setGpsGeofenceLocked() {
diff --git a/services/core/java/com/android/server/location/LocationProviderProxy.java b/services/core/java/com/android/server/location/LocationProviderProxy.java
index b44087c..16eae62 100644
--- a/services/core/java/com/android/server/location/LocationProviderProxy.java
+++ b/services/core/java/com/android/server/location/LocationProviderProxy.java
@@ -24,6 +24,7 @@
import android.location.LocationProvider;
import android.os.Bundle;
import android.os.Handler;
+import android.os.IBinder;
import android.os.RemoteException;
import android.os.WorkSource;
import android.util.Log;
@@ -82,10 +83,6 @@
return mServiceWatcher.start();
}
- private ILocationProvider getService() {
- return ILocationProvider.Stub.asInterface(mServiceWatcher.getBinder());
- }
-
public String getConnectedPackageName() {
return mServiceWatcher.getBestPackageName();
}
@@ -101,43 +98,46 @@
if (D) Log.d(TAG, "applying state to connected service");
boolean enabled;
- ProviderProperties properties = null;
+ final ProviderProperties[] properties = new ProviderProperties[1];
ProviderRequest request;
WorkSource source;
- ILocationProvider service;
synchronized (mLock) {
enabled = mEnabled;
request = mRequest;
source = mWorksource;
- service = getService();
}
- if (service == null) return;
- try {
- // load properties from provider
- properties = service.getProperties();
- if (properties == null) {
- Log.e(TAG, mServiceWatcher.getBestPackageName() +
- " has invalid locatino provider properties");
- }
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ // load properties from provider
+ properties[0] = service.getProperties();
+ if (properties[0] == null) {
+ Log.e(TAG, mServiceWatcher.getBestPackageName() +
+ " has invalid location provider properties");
+ }
- // apply current state to new service
- if (enabled) {
- service.enable();
- if (request != null) {
- service.setRequest(request, source);
+ // apply current state to new service
+ if (enabled) {
+ service.enable();
+ if (request != null) {
+ service.setRequest(request, source);
+ }
+ }
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ } catch (Exception e) {
+ // never let remote service crash system server
+ Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
}
}
- } catch (RemoteException e) {
- Log.w(TAG, e);
- } catch (Exception e) {
- // never let remote service crash system server
- Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
- }
+ });
synchronized (mLock) {
- mProperties = properties;
+ mProperties = properties[0];
}
}
};
@@ -159,17 +159,20 @@
synchronized (mLock) {
mEnabled = true;
}
- ILocationProvider service = getService();
- if (service == null) return;
-
- try {
- service.enable();
- } catch (RemoteException e) {
- Log.w(TAG, e);
- } catch (Exception e) {
- // never let remote service crash system server
- Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
- }
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ service.enable();
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ } catch (Exception e) {
+ // never let remote service crash system server
+ Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
+ }
+ }
+ });
}
@Override
@@ -177,17 +180,20 @@
synchronized (mLock) {
mEnabled = false;
}
- ILocationProvider service = getService();
- if (service == null) return;
-
- try {
- service.disable();
- } catch (RemoteException e) {
- Log.w(TAG, e);
- } catch (Exception e) {
- // never let remote service crash system server
- Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
- }
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ service.disable();
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ } catch (Exception e) {
+ // never let remote service crash system server
+ Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
+ }
+ }
+ });
}
@Override
@@ -203,17 +209,20 @@
mRequest = request;
mWorksource = source;
}
- ILocationProvider service = getService();
- if (service == null) return;
-
- try {
- service.setRequest(request, source);
- } catch (RemoteException e) {
- Log.w(TAG, e);
- } catch (Exception e) {
- // never let remote service crash system server
- Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
- }
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ service.setRequest(request, source);
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ } catch (Exception e) {
+ // never let remote service crash system server
+ Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
+ }
+ }
+ });
}
@Override
@@ -223,66 +232,78 @@
pw.append(" pkg=").append(mServiceWatcher.getBestPackageName());
pw.append(" version=").append("" + mServiceWatcher.getBestVersion());
pw.append('\n');
-
- ILocationProvider service = getService();
- if (service == null) {
+ if (!mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ TransferPipe.dumpAsync(service.asBinder(), fd, args);
+ } catch (IOException | RemoteException e) {
+ pw.println("Failed to dump location provider: " + e);
+ }
+ }
+ })) {
pw.println("service down (null)");
- return;
- }
- pw.flush();
-
- try {
- TransferPipe.dumpAsync(service.asBinder(), fd, args);
- } catch (IOException | RemoteException e) {
- pw.println("Failed to dump location provider: " + e);
}
}
@Override
public int getStatus(Bundle extras) {
- ILocationProvider service = getService();
- if (service == null) return LocationProvider.TEMPORARILY_UNAVAILABLE;
-
- try {
- return service.getStatus(extras);
- } catch (RemoteException e) {
- Log.w(TAG, e);
- } catch (Exception e) {
- // never let remote service crash system server
- Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
- }
- return LocationProvider.TEMPORARILY_UNAVAILABLE;
+ final int[] result = new int[] {LocationProvider.TEMPORARILY_UNAVAILABLE};
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ result[0] = service.getStatus(extras);
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ } catch (Exception e) {
+ // never let remote service crash system server
+ Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
+ }
+ }
+ });
+ return result[0];
}
@Override
public long getStatusUpdateTime() {
- ILocationProvider service = getService();
- if (service == null) return 0;
-
- try {
- return service.getStatusUpdateTime();
- } catch (RemoteException e) {
- Log.w(TAG, e);
- } catch (Exception e) {
- // never let remote service crash system server
- Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
- }
- return 0;
+ final long[] result = new long[] {0L};
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ result[0] = service.getStatusUpdateTime();
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ } catch (Exception e) {
+ // never let remote service crash system server
+ Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
+ }
+ }
+ });
+ return result[0];
}
@Override
public boolean sendExtraCommand(String command, Bundle extras) {
- ILocationProvider service = getService();
- if (service == null) return false;
-
- try {
- return service.sendExtraCommand(command, extras);
- } catch (RemoteException e) {
- Log.w(TAG, e);
- } catch (Exception e) {
- // never let remote service crash system server
- Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
- }
- return false;
+ final boolean[] result = new boolean[] {false};
+ mServiceWatcher.runOnBinder(new ServiceWatcher.BinderRunner() {
+ @Override
+ public void run(IBinder binder) {
+ ILocationProvider service = ILocationProvider.Stub.asInterface(binder);
+ try {
+ result[0] = service.sendExtraCommand(command, extras);
+ } catch (RemoteException e) {
+ Log.w(TAG, e);
+ } catch (Exception e) {
+ // never let remote service crash system server
+ Log.e(TAG, "Exception from " + mServiceWatcher.getBestPackageName(), e);
+ }
+ }
+ });
+ return result[0];
}
}