From 0a1ebcd8aeb4a854b7a6223ab63da0b44bf676c3 Mon Sep 17 00:00:00 2001 From: Ruslan Tkhakokhov Date: Wed, 11 May 2022 15:29:36 +0100 Subject: [PATCH] Add a null check when getting TransportConnection in PFTBT We currently call TransportManager#getCurrentTransportClient (marked as @Nullable) in PerformFullTransportBackupTask without checking the result for null. This causes occasional crashes (see attached bug). Handle the case when TransportManager returns null. Bug: 224056224 Test: 1. Manual: hardcode null return value in TransportManager#getCurrentTransport client, initiate a full backup, verify system_server does not crash) 2. atest PerformFullTransportBackupTaskTest Change-Id: Ic9fff3944cda712db4faa2f746449240558fd4d7 --- .../backup/UserBackupManagerService.java | 45 +++++++----- .../PerformFullTransportBackupTask.java | 6 ++ .../PerformFullTransportBackupTaskTest.java | 73 +++++++++++++++++++ 3 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 services/tests/servicestests/src/com/android/server/backup/fullbackup/PerformFullTransportBackupTaskTest.java diff --git a/services/backup/java/com/android/server/backup/UserBackupManagerService.java b/services/backup/java/com/android/server/backup/UserBackupManagerService.java index 1af35af9fc172..ca7fe0c571d1a 100644 --- a/services/backup/java/com/android/server/backup/UserBackupManagerService.java +++ b/services/backup/java/com/android/server/backup/UserBackupManagerService.java @@ -2359,13 +2359,37 @@ public class UserBackupManagerService { } } while (headBusy); + if (runBackup) { + CountDownLatch latch = new CountDownLatch(1); + String[] pkg = new String[]{entry.packageName}; + try { + mRunningFullBackupTask = PerformFullTransportBackupTask.newWithCurrentTransport( + this, + mOperationStorage, + /* observer */ null, + pkg, + /* updateSchedule */ true, + scheduledJob, + latch, + /* backupObserver */ null, + /* monitor */ null, + /* userInitiated */ false, + "BMS.beginFullBackup()", + getEligibilityRulesForOperation(OperationType.BACKUP)); + } catch (IllegalStateException e) { + Slog.w(TAG, "Failed to start backup", e); + runBackup = false; + } + } + if (!runBackup) { if (DEBUG_SCHEDULING) { Slog.i( TAG, addUserIdToLogMessage( mUserId, - "Nothing pending full backup; rescheduling +" + latency)); + "Nothing pending full backup or failed to start the " + + "operation; rescheduling +" + latency)); } final long deferTime = latency; // pin for the closure FullBackupJob.schedule(mUserId, mContext, deferTime, mConstants); @@ -2374,21 +2398,6 @@ public class UserBackupManagerService { // Okay, the top thing is ready for backup now. Do it. mFullBackupQueue.remove(0); - CountDownLatch latch = new CountDownLatch(1); - String[] pkg = new String[]{entry.packageName}; - mRunningFullBackupTask = PerformFullTransportBackupTask.newWithCurrentTransport( - this, - mOperationStorage, - /* observer */ null, - pkg, - /* updateSchedule */ true, - scheduledJob, - latch, - /* backupObserver */ null, - /* monitor */ null, - /* userInitiated */ false, - "BMS.beginFullBackup()", - getEligibilityRulesForOperation(OperationType.BACKUP)); // Acquiring wakelock for PerformFullTransportBackupTask before its start. mWakelock.acquire(); (new Thread(mRunningFullBackupTask)).start(); @@ -2884,7 +2893,6 @@ public class UserBackupManagerService { public void fullTransportBackup(String[] pkgNames) { mContext.enforceCallingPermission(android.Manifest.permission.BACKUP, "fullTransportBackup"); - final int callingUserHandle = UserHandle.getCallingUserId(); // TODO: http://b/22388012 if (callingUserHandle != UserHandle.USER_SYSTEM) { @@ -2936,6 +2944,9 @@ public class UserBackupManagerService { for (String pkg : pkgNames) { enqueueFullBackup(pkg, now); } + } catch (IllegalStateException e) { + Slog.w(TAG, "Failed to start backup: ", e); + return; } finally { Binder.restoreCallingIdentity(oldId); } diff --git a/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java b/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java index e74a3b97edd17..eac98f2b23375 100644 --- a/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java +++ b/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java @@ -99,6 +99,9 @@ import java.util.concurrent.atomic.AtomicLong; * mBackupRunner.getBackupResultBlocking(). */ public class PerformFullTransportBackupTask extends FullBackupTask implements BackupRestoreTask { + /** + * @throws IllegalStateException if there's no transport available. + */ public static PerformFullTransportBackupTask newWithCurrentTransport( UserBackupManagerService backupManagerService, OperationStorage operationStorage, @@ -115,6 +118,9 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba TransportManager transportManager = backupManagerService.getTransportManager(); TransportConnection transportConnection = transportManager.getCurrentTransportClient( caller); + if (transportConnection == null) { + throw new IllegalStateException("No TransportConnection available"); + } OnTaskFinishedListener listener = listenerCaller -> transportManager.disposeOfTransportClient(transportConnection, diff --git a/services/tests/servicestests/src/com/android/server/backup/fullbackup/PerformFullTransportBackupTaskTest.java b/services/tests/servicestests/src/com/android/server/backup/fullbackup/PerformFullTransportBackupTaskTest.java new file mode 100644 index 0000000000000..94742537ed1a3 --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/backup/fullbackup/PerformFullTransportBackupTaskTest.java @@ -0,0 +1,73 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.server.backup.fullbackup; + +import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; + +import android.platform.test.annotations.Presubmit; + +import androidx.test.runner.AndroidJUnit4; + +import com.android.server.backup.TransportManager; +import com.android.server.backup.UserBackupManagerService; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +@Presubmit +@RunWith(AndroidJUnit4.class) +public class PerformFullTransportBackupTaskTest { + @Mock + UserBackupManagerService mBackupManagerService; + @Mock + TransportManager mTransportManager; + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + + when(mBackupManagerService.getTransportManager()).thenReturn(mTransportManager); + } + + @Test + public void testNewWithCurrentTransport_noTransportConnection_throws() { + when(mTransportManager.getCurrentTransportClient(any())).thenReturn(null); + + assertThrows(IllegalStateException.class, + () -> { + PerformFullTransportBackupTask task = PerformFullTransportBackupTask + .newWithCurrentTransport( + mBackupManagerService, + /* operationStorage */ null, + /* observer */ null, + /* whichPackages */ null, + /* updateSchedule */ false, + /* runningJob */ null, + /* latch */ null, + /* backupObserver */ null, + /* monitor */ null, + /* userInitiated */ false, + /* caller */ null, + /* backupEligibilityRules */ null); + }); + } +}