From 192710b578b1d839a2c83ad5114e503669ffcef2 Mon Sep 17 00:00:00 2001 From: Soonil Nagarkar Date: Thu, 24 Jan 2019 17:42:49 -0800 Subject: [PATCH] Fix potential deadlock in ServiceWatcher Bug: 123363078 Test: manually Change-Id: I9ac83c37c0c00170f95b505bdac68bb5db6e0283 --- .../com/android/server/ServiceWatcher.java | 154 +++++++++++------- .../server/location/GeocoderProxy.java | 31 +--- .../location/LocationProviderProxy.java | 57 ++----- 3 files changed, 123 insertions(+), 119 deletions(-) diff --git a/services/core/java/com/android/server/ServiceWatcher.java b/services/core/java/com/android/server/ServiceWatcher.java index d09823efb6fab..a5a515f93e757 100644 --- a/services/core/java/com/android/server/ServiceWatcher.java +++ b/services/core/java/com/android/server/ServiceWatcher.java @@ -34,11 +34,11 @@ import android.os.Bundle; import android.os.Handler; import android.os.IBinder; import android.os.Looper; +import android.os.RemoteException; import android.os.UserHandle; import android.util.Log; import android.util.Slog; -import com.android.internal.annotations.GuardedBy; import com.android.internal.content.PackageMonitor; import com.android.internal.util.Preconditions; @@ -48,6 +48,9 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.FutureTask; /** * Find the best Service, and bind to it. @@ -61,16 +64,20 @@ public class ServiceWatcher implements ServiceConnection { public static final String EXTRA_SERVICE_VERSION = "serviceVersion"; public static final String EXTRA_SERVICE_IS_MULTIUSER = "serviceIsMultiuser"; - /** - * The runner that runs on the binder retrieved from {@link ServiceWatcher}. - */ + + /** Function to run on binder interface. */ public interface BinderRunner { - /** - * Runs on the retrieved binder. - * - * @param binder the binder retrieved from the {@link ServiceWatcher}. - */ - void run(IBinder binder); + /** Called to run client code with the binder. */ + void run(IBinder binder) throws RemoteException; + } + + /** + * Function to run on binder interface. + * @param Type to return. + */ + public interface BlockingBinderRunner { + /** Called to run client code with the binder. */ + T run(IBinder binder) throws RemoteException; } public static ArrayList> getSignatureSets(Context context, @@ -120,18 +127,14 @@ public class ServiceWatcher implements ServiceConnection { private final Handler mHandler; - // this lock is held to ensure the service binder is not exposed (via runOnBinder) until after - // the new service initialization work has completed - private final Object mBindLock = new Object(); - // read/write from handler thread + private IBinder mBestService; private int mCurrentUserId; // read from any thread, write from handler thread private volatile ComponentName mBestComponent; private volatile int mBestVersion; private volatile int mBestUserId; - private volatile IBinder mBestService; public ServiceWatcher(Context context, String logTag, String action, int overlaySwitchResId, int defaultServicePackageNameResId, @@ -163,17 +166,9 @@ public class ServiceWatcher implements ServiceConnection { mBestService = null; } - // called on handler thread - @GuardedBy("mBindLock") - protected void onBind() { - Preconditions.checkState(Looper.myLooper() == mHandler.getLooper()); - } + protected void onBind() {} - // called on handler thread - @GuardedBy("mBindLock") - protected void onUnbind() { - Preconditions.checkState(Looper.myLooper() == mHandler.getLooper()); - } + protected void onUnbind() {} /** * Start this watcher, including binding to the current best match and @@ -248,25 +243,6 @@ public class ServiceWatcher implements ServiceConnection { return bestComponent == null ? null : bestComponent.getPackageName(); } - /** - * Runs the given BinderRunner if currently connected. All invocations to runOnBinder are run - * serially. - */ - public final void runOnBinder(BinderRunner runner) { - synchronized (mBindLock) { - IBinder service = mBestService; - if (service != null) { - try { - runner.run(service); - } catch (Exception e) { - // remote exceptions cannot be allowed to crash system server - Log.e(TAG, "exception while while running " + runner + " on " + service - + " from " + this, e); - } - } - } - } - private boolean isServiceMissing() { return mContext.getPackageManager().queryIntentServicesAsUser(new Intent(mAction), PackageManager.MATCH_DIRECT_BOOT_AWARE @@ -380,28 +356,66 @@ public class ServiceWatcher implements ServiceConnection { mBestUserId = UserHandle.USER_NULL; } + /** + * Runs the given function asynchronously if currently connected. Suppresses any RemoteException + * thrown during execution. + */ + public final void runOnBinder(BinderRunner runner) { + runOnHandler(() -> { + if (mBestService == null) { + return; + } + + try { + runner.run(mBestService); + } catch (RuntimeException e) { + // the code being run is privileged, but may be outside the system server, and thus + // we cannot allow runtime exceptions to crash the system server + Log.e(TAG, "exception while while running " + runner + " on " + mBestService + + " from " + this, e); + } catch (RemoteException e) { + // do nothing + } + }); + } + + /** + * Runs the given function synchronously if currently connected, and returns the default value + * if not currently connected or if any exception is thrown. + */ + public final T runOnBinderBlocking(BlockingBinderRunner runner, T defaultValue) { + try { + return runOnHandlerBlocking(() -> { + if (mBestService == null) { + return defaultValue; + } + + try { + return runner.run(mBestService); + } catch (RemoteException e) { + return defaultValue; + } + }); + } catch (InterruptedException e) { + return defaultValue; + } + } + @Override public final void onServiceConnected(ComponentName component, IBinder binder) { - mHandler.post(() -> { + runOnHandler(() -> { if (D) Log.d(mTag, component + " connected"); - - // hold the lock so that mBestService cannot be used by runOnBinder until complete - synchronized (mBindLock) { - mBestService = binder; - onBind(); - } + mBestService = binder; + onBind(); }); } @Override public final void onServiceDisconnected(ComponentName component) { - mHandler.post(() -> { + runOnHandler(() -> { if (D) Log.d(mTag, component + " disconnected"); - mBestService = null; - synchronized (mBindLock) { - onUnbind(); - } + onUnbind(); }); } @@ -410,4 +424,32 @@ public class ServiceWatcher implements ServiceConnection { ComponentName bestComponent = mBestComponent; return bestComponent == null ? "null" : bestComponent.toShortString() + "@" + mBestVersion; } + + private void runOnHandler(Runnable r) { + if (Looper.myLooper() == mHandler.getLooper()) { + r.run(); + } else { + mHandler.post(r); + } + } + + private T runOnHandlerBlocking(Callable c) throws InterruptedException { + if (Looper.myLooper() == mHandler.getLooper()) { + try { + return c.call(); + } catch (Exception e) { + // Function cannot throw exception, this should never happen + throw new IllegalStateException(e); + } + } else { + FutureTask task = new FutureTask<>(c); + mHandler.post(task); + try { + return task.get(); + } catch (ExecutionException e) { + // Function cannot throw exception, this should never happen + throw new IllegalStateException(e); + } + } + } } diff --git a/services/core/java/com/android/server/location/GeocoderProxy.java b/services/core/java/com/android/server/location/GeocoderProxy.java index f1de371885102..e6f0ed9d14b06 100644 --- a/services/core/java/com/android/server/location/GeocoderProxy.java +++ b/services/core/java/com/android/server/location/GeocoderProxy.java @@ -20,8 +20,6 @@ import android.content.Context; import android.location.Address; import android.location.GeocoderParams; import android.location.IGeocodeProvider; -import android.os.RemoteException; -import android.util.Log; import com.android.internal.os.BackgroundThread; import com.android.server.ServiceWatcher; @@ -68,35 +66,22 @@ public class GeocoderProxy { public String getFromLocation(double latitude, double longitude, int maxResults, GeocoderParams params, List
addrs) { - final String[] result = new String[]{"Service not Available"}; - mServiceWatcher.runOnBinder(binder -> { + return mServiceWatcher.runOnBinderBlocking(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 result[0]; + return provider.getFromLocation(latitude, longitude, maxResults, params, addrs); + }, "Service not Available"); } public String getFromLocationName(String locationName, double lowerLeftLatitude, double lowerLeftLongitude, double upperRightLatitude, double upperRightLongitude, int maxResults, GeocoderParams params, List
addrs) { - final String[] result = new String[]{"Service not Available"}; - mServiceWatcher.runOnBinder(binder -> { + return mServiceWatcher.runOnBinderBlocking(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 result[0]; + return provider.getFromLocationName(locationName, lowerLeftLatitude, + lowerLeftLongitude, upperRightLatitude, upperRightLongitude, + maxResults, params, addrs); + }, "Service not Available"); } } diff --git a/services/core/java/com/android/server/location/LocationProviderProxy.java b/services/core/java/com/android/server/location/LocationProviderProxy.java index a6da8c5e77133..6b5b1bebd20fe 100644 --- a/services/core/java/com/android/server/location/LocationProviderProxy.java +++ b/services/core/java/com/android/server/location/LocationProviderProxy.java @@ -127,20 +127,16 @@ public class LocationProviderProxy extends AbstractLocationProvider { return mServiceWatcher.start(); } - private void initializeService(IBinder binder) { + private void initializeService(IBinder binder) throws RemoteException { ILocationProvider service = ILocationProvider.Stub.asInterface(binder); if (D) Log.d(TAG, "applying state to connected service " + mServiceWatcher); - try { - service.setLocationProviderManager(mManager); + service.setLocationProviderManager(mManager); - synchronized (mRequestLock) { - if (mRequest != null) { - service.setRequest(mRequest, mWorkSource); - } + synchronized (mRequestLock) { + if (mRequest != null) { + service.setRequest(mRequest, mWorkSource); } - } catch (RemoteException e) { - Log.w(TAG, e); } } @@ -157,63 +153,44 @@ public class LocationProviderProxy extends AbstractLocationProvider { } mServiceWatcher.runOnBinder(binder -> { ILocationProvider service = ILocationProvider.Stub.asInterface(binder); - try { - service.setRequest(request, source); - } catch (RemoteException e) { - Log.w(TAG, e); - } + service.setRequest(request, source); }); } @Override public void dump(FileDescriptor fd, PrintWriter pw, String[] args) { pw.println(" service=" + mServiceWatcher); - mServiceWatcher.runOnBinder(binder -> { + mServiceWatcher.runOnBinderBlocking(binder -> { try { TransferPipe.dumpAsync(binder, fd, args); } catch (IOException | RemoteException e) { - pw.println(" failed to dump location provider: " + e); + pw.println(" failed to dump location provider"); } - }); + return null; + }, null); } @Override public int getStatus(Bundle extras) { - int[] status = new int[] {LocationProvider.TEMPORARILY_UNAVAILABLE}; - mServiceWatcher.runOnBinder(binder -> { + return mServiceWatcher.runOnBinderBlocking(binder -> { ILocationProvider service = ILocationProvider.Stub.asInterface(binder); - try { - status[0] = service.getStatus(extras); - } catch (RemoteException e) { - Log.w(TAG, e); - } - }); - return status[0]; + return service.getStatus(extras); + }, LocationProvider.TEMPORARILY_UNAVAILABLE); } @Override public long getStatusUpdateTime() { - long[] updateTime = new long[] {0L}; - mServiceWatcher.runOnBinder(binder -> { + return mServiceWatcher.runOnBinderBlocking(binder -> { ILocationProvider service = ILocationProvider.Stub.asInterface(binder); - try { - updateTime[0] = service.getStatusUpdateTime(); - } catch (RemoteException e) { - Log.w(TAG, e); - } - }); - return updateTime[0]; + return service.getStatusUpdateTime(); + }, 0L); } @Override public void sendExtraCommand(String command, Bundle extras) { mServiceWatcher.runOnBinder(binder -> { ILocationProvider service = ILocationProvider.Stub.asInterface(binder); - try { - service.sendExtraCommand(command, extras); - } catch (RemoteException e) { - Log.w(TAG, e); - } + service.sendExtraCommand(command, extras); }); } }