Merge "Don't use transport binder with the lock held"

2 files changed, 107 insertions, 50 deletions

diff --git a/services/backup/java/com/android/server/backup/TransportManager.java b/services/backup/java/com/android/server/backup/TransportManager.java
index 30fd25a92484..5b901ee2b3da 100644
--- a/services/backup/java/com/android/server/backup/TransportManager.java
+++ b/services/backup/java/com/android/server/backup/TransportManager.java
@@ -62,9 +62,17 @@ public class TransportManager {
     private final PackageManager mPackageManager;
     private final Set<ComponentName> mTransportWhitelist;
     private final TransportClientManager mTransportClientManager;
-    private final Object mTransportLock = new Object();
     private OnTransportRegisteredListener mOnTransportRegisteredListener = (c, n) -> {};
 
+    /**
+     * Lock for registered transports and currently selected transport.
+     *
+     * <p><b>Warning:</b> No calls to {@link IBackupTransport} or calls that result in transport
+     * code being executed such as {@link TransportClient#connect(String)}} and its variants should
+     * be made with this lock held, risk of deadlock.
+     */
+    private final Object mTransportLock = new Object();
+
     /** @see #getRegisteredTransportNames() */
     @GuardedBy("mTransportLock")
     private final Map<ComponentName, TransportDescription> mRegisteredTransportsDescriptionMap =
@@ -109,15 +117,16 @@ public class TransportManager {
 
     @WorkerThread
     void onPackageChanged(String packageName, String... components) {
+        // Unfortunately this can't be atomic because we risk a deadlock if
+        // registerTransportsFromPackage() is put inside the synchronized block
+        Set<ComponentName> transportComponents =
+                Stream.of(components)
+                        .map(component -> new ComponentName(packageName, component))
+                        .collect(Collectors.toSet());
         synchronized (mTransportLock) {
-            Set<ComponentName> transportComponents =
-                    Stream.of(components)
-                            .map(component -> new ComponentName(packageName, component))
-                            .collect(Collectors.toSet());
-
             mRegisteredTransportsDescriptionMap.keySet().removeIf(transportComponents::contains);
-            registerTransportsFromPackage(packageName, transportComponents::contains);
         }
+        registerTransportsFromPackage(packageName, transportComponents::contains);
     }
 
     /**
@@ -263,6 +272,9 @@ public class TransportManager {
      * This is called with an internal lock held, ensuring that the transport will remain registered
      * while {@code transportConsumer} is being executed. Don't do heavy operations in {@code
      * transportConsumer}.
+     *
+     * <p><b>Warning:</b> Do NOT make any calls to {@link IBackupTransport} or call any variants of
+     * {@link TransportClient#connect(String)} here, otherwise you risk deadlock.
      */
     public void forEachRegisteredTransport(Consumer<String> transportConsumer) {
         synchronized (mTransportLock) {
@@ -465,20 +477,27 @@ public class TransportManager {
      */
     @WorkerThread
     public int registerAndSelectTransport(ComponentName transportComponent) {
+        // If it's already registered we select and return
         synchronized (mTransportLock) {
-            if (!mRegisteredTransportsDescriptionMap.containsKey(transportComponent)) {
-                int result = registerTransport(transportComponent);
-                if (result != BackupManager.SUCCESS) {
-                    return result;
-                }
+            try {
+                selectTransport(getTransportName(transportComponent));
+                return BackupManager.SUCCESS;
+            } catch (TransportNotRegisteredException e) {
+                // Fall through and release lock
             }
+        }
 
+        // We can't call registerTransport() with the transport lock held
+        int result = registerTransport(transportComponent);
+        if (result != BackupManager.SUCCESS) {
+            return result;
+        }
+        synchronized (mTransportLock) {
             try {
                 selectTransport(getTransportName(transportComponent));
                 return BackupManager.SUCCESS;
             } catch (TransportNotRegisteredException e) {
-                // Shouldn't happen because we are holding the lock
-                Slog.wtf(TAG, "Transport unexpectedly not registered");
+                Slog.wtf(TAG, "Transport got unregistered");
                 return BackupManager.ERROR_TRANSPORT_UNAVAILABLE;
             }
         }
@@ -512,13 +531,11 @@ public class TransportManager {
         if (hosts == null) {
             return;
         }
-        synchronized (mTransportLock) {
-            for (ResolveInfo host : hosts) {
-                ComponentName transportComponent = host.serviceInfo.getComponentName();
-                if (transportComponentFilter.test(transportComponent)
-                        && isTransportTrusted(transportComponent)) {
-                    registerTransport(transportComponent);
-                }
+        for (ResolveInfo host : hosts) {
+            ComponentName transportComponent = host.serviceInfo.getComponentName();
+            if (transportComponentFilter.test(transportComponent)
+                    && isTransportTrusted(transportComponent)) {
+                registerTransport(transportComponent);
             }
         }
     }
@@ -547,22 +564,24 @@ public class TransportManager {
     /**
      * Tries to register transport represented by {@code transportComponent}.
      *
+     * <p><b>Warning:</b> Don't call this with the transport lock held.
+     *
      * @param transportComponent Host of the transport that we want to register.
      * @return One of {@link BackupManager#SUCCESS}, {@link BackupManager#ERROR_TRANSPORT_INVALID}
      *     or {@link BackupManager#ERROR_TRANSPORT_UNAVAILABLE}.
      */
     @WorkerThread
     private int registerTransport(ComponentName transportComponent) {
+        checkCanUseTransport();
+
         if (!isTransportTrusted(transportComponent)) {
             return BackupManager.ERROR_TRANSPORT_INVALID;
         }
 
         String transportString = transportComponent.flattenToShortString();
-
         String callerLogString = "TransportManager.registerTransport()";
         TransportClient transportClient =
                 mTransportClientManager.getTransportClient(transportComponent, callerLogString);
-
         final IBackupTransport transport;
         try {
             transport = transportClient.connectOrThrow(callerLogString);
@@ -593,20 +612,26 @@ public class TransportManager {
     /** If {@link RemoteException} is thrown the transport is guaranteed to not be registered. */
     private void registerTransport(ComponentName transportComponent, IBackupTransport transport)
             throws RemoteException {
+        checkCanUseTransport();
+
+        TransportDescription description =
+                new TransportDescription(
+                        transport.name(),
+                        transport.transportDirName(),
+                        transport.configurationIntent(),
+                        transport.currentDestinationString(),
+                        transport.dataManagementIntent(),
+                        transport.dataManagementLabel());
         synchronized (mTransportLock) {
-            String name = transport.name();
-            TransportDescription description =
-                    new TransportDescription(
-                            name,
-                            transport.transportDirName(),
-                            transport.configurationIntent(),
-                            transport.currentDestinationString(),
-                            transport.dataManagementIntent(),
-                            transport.dataManagementLabel());
             mRegisteredTransportsDescriptionMap.put(transportComponent, description);
         }
     }
 
+    private void checkCanUseTransport() {
+        Preconditions.checkState(
+                !Thread.holdsLock(mTransportLock), "Can't call transport with transport lock held");
+    }
+
     private static Predicate<ComponentName> fromPackageFilter(String packageName) {
         return transportComponent -> packageName.equals(transportComponent.getPackageName());
     }
diff --git a/services/robotests/src/com/android/server/backup/TransportManagerTest.java b/services/robotests/src/com/android/server/backup/TransportManagerTest.java
index cf0bc235dc10..6753d73590d3 100644
--- a/services/robotests/src/com/android/server/backup/TransportManagerTest.java
+++ b/services/robotests/src/com/android/server/backup/TransportManagerTest.java
@@ -19,9 +19,13 @@ package com.android.server.backup;
 import static com.android.server.backup.testing.TransportData.genericTransport;
 import static com.android.server.backup.testing.TransportTestUtils.mockTransport;
 import static com.android.server.backup.testing.TransportTestUtils.setUpTransportsForTransportManager;
-
 import static com.google.common.truth.Truth.assertThat;
-
+import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+import static java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toSet;
+import static java.util.stream.Stream.concat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.never;
@@ -31,23 +35,15 @@ import static org.mockito.Mockito.when;
 import static org.robolectric.shadow.api.Shadow.extract;
 import static org.testng.Assert.expectThrows;
 
-import static java.util.Arrays.asList;
-import static java.util.Collections.emptyList;
-import static java.util.Collections.singletonList;
-import static java.util.stream.Collectors.toList;
-import static java.util.stream.Collectors.toSet;
-import static java.util.stream.Stream.concat;
-
 import android.annotation.Nullable;
+import android.app.backup.BackupManager;
 import android.content.ComponentName;
 import android.content.Context;
 import android.content.Intent;
 import android.content.pm.ApplicationInfo;
 import android.content.pm.PackageInfo;
 import android.platform.test.annotations.Presubmit;
-
 import com.android.server.backup.testing.ShadowContextImplForBackup;
-import com.android.server.testing.shadows.FrameworkShadowPackageManager;
 import com.android.server.backup.testing.TransportData;
 import com.android.server.backup.testing.TransportTestUtils.TransportMock;
 import com.android.server.backup.transport.OnTransportRegisteredListener;
@@ -57,7 +53,12 @@ import com.android.server.backup.transport.TransportNotRegisteredException;
 import com.android.server.testing.FrameworkRobolectricTestRunner;
 import com.android.server.testing.SystemLoaderClasses;
 import com.android.server.testing.shadows.FrameworkShadowContextImpl;
-
+import com.android.server.testing.shadows.FrameworkShadowPackageManager;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Stream;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -68,12 +69,6 @@ import org.robolectric.RuntimeEnvironment;
 import org.robolectric.annotation.Config;
 import org.robolectric.shadows.ShadowPackageManager;
 
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Objects;
-import java.util.Set;
-import java.util.stream.Stream;
-
 @RunWith(FrameworkRobolectricTestRunner.class)
 @Config(
     manifest = Config.NONE,
@@ -308,6 +303,43 @@ public class TransportManagerTest {
     }
 
     @Test
+    public void testRegisterAndSelectTransport_whenTransportRegistered() throws Exception {
+        setUpPackage(PACKAGE_A, ApplicationInfo.PRIVATE_FLAG_PRIVILEGED);
+        setUpTransports(mTransportA1);
+        TransportManager transportManager = createTransportManager(null, mTransportA1);
+        transportManager.registerTransports();
+        ComponentName transportComponent = mTransportA1.getTransportComponent();
+
+        int result = transportManager.registerAndSelectTransport(transportComponent);
+
+        assertThat(result).isEqualTo(BackupManager.SUCCESS);
+        assertThat(transportManager.getRegisteredTransportComponents())
+                .asList()
+                .contains(transportComponent);
+        assertThat(transportManager.getCurrentTransportName())
+                .isEqualTo(mTransportA1.transportName);
+    }
+
+    @Test
+    public void testRegisterAndSelectTransport_whenTransportNotRegistered() throws Exception {
+        setUpPackage(PACKAGE_A, ApplicationInfo.PRIVATE_FLAG_PRIVILEGED);
+        setUpTransports(mTransportA1);
+        TransportManager transportManager = createTransportManager(null, mTransportA1);
+        ComponentName transportComponent = mTransportA1.getTransportComponent();
+
+        int result = transportManager.registerAndSelectTransport(transportComponent);
+
+        assertThat(result).isEqualTo(BackupManager.SUCCESS);
+        assertThat(transportManager.getRegisteredTransportComponents())
+                .asList()
+                .contains(transportComponent);
+        assertThat(transportManager.getTransportDirName(mTransportA1.transportName))
+                .isEqualTo(mTransportA1.transportDirName);
+        assertThat(transportManager.getCurrentTransportName())
+                .isEqualTo(mTransportA1.transportName);
+    }
+
+    @Test
     public void testGetCurrentTransportName_whenSelectTransportNotCalled_returnsDefaultTransport()
             throws Exception {
         setUpPackage(PACKAGE_A, ApplicationInfo.PRIVATE_FLAG_PRIVILEGED);