From 98f8122702709355750e618ce434ab798864f885 Mon Sep 17 00:00:00 2001 From: Ruslan Tkhakokhov Date: Mon, 17 Jan 2022 23:39:21 +0000 Subject: [PATCH] Skip restore of read-only directories Currently trying to restore a read-only directory results in only the directory being restored but not its contents. This causes problems for some apps (see the associated bug). Skip restore of read-only directories while we evaluate the priority of proper suport for this use case. Bug: 194894879 Test: Manual: 1. Populate a read-only directory in an app's folder (/data/user/0/) 2. Run backup for the app 3. Clear app data (adb shell pm clear 4. Run restore for the app 5. Inspect app folder to ensure all writable content is restored. Merged-In: I644a5784fb42352ba28ac944d3aa3564b0215a78 Change-Id: I644a5784fb42352ba28ac944d3aa3564b0215a78 --- .../backup/restore/FullRestoreEngine.java | 65 ++++++++ .../backup/restore/FullRestoreEngineTest.java | 150 ++++++++++++++++++ 2 files changed, 215 insertions(+) create mode 100644 services/tests/servicestests/src/com/android/server/backup/restore/FullRestoreEngineTest.java diff --git a/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java b/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java index 5718bdfc49713..9298b1e7efbf5 100644 --- a/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java +++ b/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java @@ -25,8 +25,10 @@ import static com.android.server.backup.UserBackupManagerService.OP_TYPE_RESTORE import static com.android.server.backup.UserBackupManagerService.SHARED_BACKUP_AGENT_PACKAGE; import static com.android.server.backup.internal.BackupHandler.MSG_RESTORE_OPERATION_TIMEOUT; +import android.annotation.NonNull; import android.app.ApplicationThreadConstants; import android.app.IBackupAgent; +import android.app.backup.BackupAgent; import android.app.backup.BackupManager; import android.app.backup.FullBackup; import android.app.backup.IBackupManagerMonitor; @@ -39,10 +41,12 @@ import android.content.pm.Signature; import android.os.ParcelFileDescriptor; import android.os.RemoteException; import android.provider.Settings; +import android.system.OsConstants; import android.text.TextUtils; import android.util.Slog; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; import com.android.server.LocalServices; import com.android.server.backup.BackupAgentTimeoutParameters; import com.android.server.backup.BackupRestoreTask; @@ -56,6 +60,7 @@ import com.android.server.backup.utils.FullBackupRestoreObserverUtils; import com.android.server.backup.utils.RestoreUtils; import com.android.server.backup.utils.TarBackupReader; +import java.io.File; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; @@ -132,6 +137,7 @@ public class FullRestoreEngine extends RestoreEngine { @GuardedBy("mPipesLock") private boolean mPipesClosed; private final BackupEligibilityRules mBackupEligibilityRules; + private FileMetadata mReadOnlyParent = null; public FullRestoreEngine(UserBackupManagerService backupManagerService, BackupRestoreTask monitorTask, IFullBackupRestoreObserver observer, @@ -154,6 +160,21 @@ public class FullRestoreEngine extends RestoreEngine { mBackupEligibilityRules = backupEligibilityRules; } + @VisibleForTesting + FullRestoreEngine() { + mIsAdbRestore = false; + mAllowApks = false; + mEphemeralOpToken = 0; + mUserId = 0; + mBackupEligibilityRules = null; + mAgentTimeoutParameters = null; + mBuffer = null; + mBackupManagerService = null; + mMonitor = null; + mMonitorTask = null; + mOnlyPackage = null; + } + public IBackupAgent getAgent() { return mAgent; } @@ -393,6 +414,11 @@ public class FullRestoreEngine extends RestoreEngine { okay = false; } + if (shouldSkipReadOnlyDir(info)) { + // b/194894879: We don't support restore of read-only dirs. + okay = false; + } + // At this point we have an agent ready to handle the full // restore data as well as a pipe for sending data to // that agent. Tell the agent to start reading from the @@ -569,6 +595,45 @@ public class FullRestoreEngine extends RestoreEngine { return (info != null); } + boolean shouldSkipReadOnlyDir(FileMetadata info) { + if (isValidParent(mReadOnlyParent, info)) { + // This file has a read-only parent directory, we shouldn't + // restore it. + return true; + } else { + // We're now in a different branch of the file tree, update the parent + // value. + if (isReadOnlyDir(info)) { + // Current directory is read-only. Remember it so that we can skip all + // of its contents. + mReadOnlyParent = info; + Slog.w(TAG, "Skipping restore of " + info.path + " and its contents as " + + "read-only dirs are currently not supported."); + return true; + } else { + mReadOnlyParent = null; + } + } + + return false; + } + + private static boolean isValidParent(FileMetadata parentDir, @NonNull FileMetadata childDir) { + return parentDir != null + && childDir.packageName.equals(parentDir.packageName) + && childDir.domain.equals(parentDir.domain) + && childDir.path.startsWith(getPathWithTrailingSeparator(parentDir.path)); + } + + private static String getPathWithTrailingSeparator(String path) { + return path.endsWith(File.separator) ? path : path + File.separator; + } + + private static boolean isReadOnlyDir(FileMetadata file) { + // Check if owner has 'write' bit in the file's mode value (see 'man -7 inode' for details). + return file.type == BackupAgent.TYPE_DIRECTORY && (file.mode & OsConstants.S_IWUSR) == 0; + } + private void setUpPipes() throws IOException { synchronized (mPipesLock) { mPipes = ParcelFileDescriptor.createPipe(); diff --git a/services/tests/servicestests/src/com/android/server/backup/restore/FullRestoreEngineTest.java b/services/tests/servicestests/src/com/android/server/backup/restore/FullRestoreEngineTest.java new file mode 100644 index 0000000000000..049c745fc128e --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/backup/restore/FullRestoreEngineTest.java @@ -0,0 +1,150 @@ +/* + * 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.restore; + +import static com.google.common.truth.Truth.assertWithMessage; + +import android.app.backup.BackupAgent; +import android.platform.test.annotations.Presubmit; +import android.system.OsConstants; + +import androidx.test.runner.AndroidJUnit4; + +import com.android.server.backup.FileMetadata; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +@Presubmit +@RunWith(AndroidJUnit4.class) +public class FullRestoreEngineTest { + private static final String DEFAULT_PACKAGE_NAME = "package"; + private static final String DEFAULT_DOMAIN_NAME = "domain"; + private static final String NEW_PACKAGE_NAME = "new_package"; + private static final String NEW_DOMAIN_NAME = "new_domain"; + + private FullRestoreEngine mRestoreEngine; + + @Before + public void setUp() { + mRestoreEngine = new FullRestoreEngine(); + } + + @Test + public void shouldSkipReadOnlyDir_skipsAllReadonlyDirsAndTheirChildren() { + // Create the file tree. + TestFile[] testFiles = new TestFile[] { + TestFile.dir("root"), + TestFile.file("root/auth_token"), + TestFile.dir("root/media"), + TestFile.file("root/media/picture1.png"), + TestFile.file("root/push_token.txt"), + TestFile.dir("root/read-only-dir-1").markReadOnly().expectSkipped(), + TestFile.dir("root/read-only-dir-1/writable-subdir").expectSkipped(), + TestFile.file("root/read-only-dir-1/writable-subdir/writable-file").expectSkipped(), + TestFile.dir("root/read-only-dir-1/writable-subdir/read-only-subdir-2") + .markReadOnly().expectSkipped(), + TestFile.file("root/read-only-dir-1/writable-file").expectSkipped(), + TestFile.file("root/random-stuff.txt"), + TestFile.dir("root/database"), + TestFile.file("root/database/users.db"), + TestFile.dir("root/read-only-dir-2").markReadOnly().expectSkipped(), + TestFile.file("root/read-only-dir-2/writable-file-1").expectSkipped(), + TestFile.file("root/read-only-dir-2/writable-file-2").expectSkipped(), + }; + + assertCorrectItemsAreSkipped(testFiles); + } + + @Test + public void shouldSkipReadOnlyDir_onlySkipsChildrenUnderTheSamePackage() { + TestFile[] testFiles = new TestFile[]{ + TestFile.dir("read-only-dir").markReadOnly().expectSkipped(), + TestFile.file("read-only-dir/file").expectSkipped(), + TestFile.file("read-only-dir/file-from-different-package") + .setPackage(NEW_PACKAGE_NAME), + }; + + assertCorrectItemsAreSkipped(testFiles); + } + + @Test + public void shouldSkipReadOnlyDir_onlySkipsChildrenUnderTheSameDomain() { + TestFile[] testFiles = new TestFile[]{ + TestFile.dir("read-only-dir").markReadOnly().expectSkipped(), + TestFile.file("read-only-dir/file").expectSkipped(), + TestFile.file("read-only-dir/file-from-different-domain") + .setDomain(NEW_DOMAIN_NAME), + }; + + assertCorrectItemsAreSkipped(testFiles); + } + + private void assertCorrectItemsAreSkipped(TestFile[] testFiles) { + // Verify all directories marked with .expectSkipped are skipped. + for (TestFile testFile : testFiles) { + boolean actualExcluded = mRestoreEngine.shouldSkipReadOnlyDir(testFile.mMetadata); + boolean expectedExcluded = testFile.mShouldSkip; + assertWithMessage(testFile.mMetadata.path).that(actualExcluded).isEqualTo( + expectedExcluded); + } + } + + private static class TestFile { + private final FileMetadata mMetadata; + private boolean mShouldSkip; + + static TestFile dir(String path) { + return new TestFile(path, BackupAgent.TYPE_DIRECTORY); + } + + static TestFile file(String path) { + return new TestFile(path, BackupAgent.TYPE_FILE); + } + + TestFile markReadOnly() { + mMetadata.mode = 0; + return this; + } + + TestFile expectSkipped() { + mShouldSkip = true; + return this; + } + + TestFile setPackage(String packageName) { + mMetadata.packageName = packageName; + return this; + } + + TestFile setDomain(String domain) { + mMetadata.domain = domain; + return this; + } + + private TestFile(String path, int type) { + FileMetadata metadata = new FileMetadata(); + metadata.path = path; + metadata.type = type; + metadata.packageName = DEFAULT_PACKAGE_NAME; + metadata.domain = DEFAULT_DOMAIN_NAME; + metadata.mode = OsConstants.S_IWUSR; // Mark as writable. + mMetadata = metadata; + } + } +}