Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 070081bc authored by Bernardo Rufino's avatar Bernardo Rufino
Browse files

Don't use transport binder with the lock held 

There was a deadlock around the transport lock. We registered transports
with the transport lock held, this kicked-off transport onCreate()
synchronously, which called TransportManager
updateTransportAttributes(), which tried to acquire mentioned lock but
couldn't. This CL removes the lock for any call to the transport or
operation that triggers a call to the transport (it was
TransportClient.connect() or its variants).

Test: Load GMSCore before fix, boot, register transports, check no ANR
Test: m -j RunFrameworksServicesRoboTests
Test: adb shell bmgr transport -c <transport>, being registered & not
Bug: 72147303
Change-Id: I72ca145d7fb73c0ef29c4aa1b620fea4969481db
parent e8ffec16

services/backup/java/com/android/server/backup/TransportManager.java

+57 −32
Original line number Diff line number Diff line
@@ -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) {

        synchronized (mTransportLock) {

        // 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) {

            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)) {

            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,7 +531,6 @@ public class TransportManager { 
        if (hosts == null) {

            return;

        }

        synchronized (mTransportLock) {

        for (ResolveInfo host : hosts) {

            ComponentName transportComponent = host.serviceInfo.getComponentName();

            if (transportComponentFilter.test(transportComponent)
@@ -521,7 +539,6 @@ public class TransportManager { 
            }

        }

    }

    }



    /** Transport has to be whitelisted and privileged. */

    private boolean isTransportTrusted(ComponentName transport) {
@@ -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 {

        synchronized (mTransportLock) {

            String name = transport.name();

        checkCanUseTransport();



        TransportDescription description =

                new TransportDescription(

                            name,

                        transport.name(),

                        transport.transportDirName(),

                        transport.configurationIntent(),

                        transport.currentDestinationString(),

                        transport.dataManagementIntent(),

                        transport.dataManagementLabel());

        synchronized (mTransportLock) {

            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());

    }

services/robotests/src/com/android/server/backup/TransportManagerTest.java

+50 −18
Original line number Diff line number Diff line
@@ -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,
@@ -307,6 +302,43 @@ public class TransportManagerTest { 
                .onTransportRegistered(mTransportA2.transportName, mTransportA2.transportDirName);

    }



    @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 {