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

Commit 3621745f authored by Eric Biggers's avatar Eric Biggers
Browse files

UserDataPreparer: be more careful about auto-deleting data on error 

Currently UserDataPreparer automatically deletes all the user's data
directories if an error occurs.  There are several possible reasons for
this; the original motivation (when this code was added in Android 7) is
not clear.  In any case, it's been demonstrated that it's too dangerous
to apply to all users.  It should only apply to users being created.

Therefore, this CL limits the automatic data deletion to users that have
never "logged in", i.e. users where 'lastLoggedInTime == 0'.

It also limits the call to rebootPromptAndWipeUserData() to first boot.

The disadvantage of this change is that failures for existing users may
now go unnoticed again, considering that UserDataPreparer will just log
and ignore them.  But the error handling really needs to be in the
calling code, in UserController and UserManagerService.  E.g., when
starting (or unlocking) a user, maybe the start (or unlock) should be
cancelled if prepareUserData fails.  All things considered though, even
without other changes, auto-deletion seems like the wrong choice now.

Bug: 307627225
Test: atest UserDataPreparerTest
Test: Verified via the new log message that isNewUser is assigned the
      correct value for both system user and secondary user, both
      existing and newly created; and for both CE and DE storage.
Change-Id: If78d50b17eb4a579586bb659cae2c61f00deb79d
parent 3b84c2ca

services/core/java/com/android/server/pm/UserDataPreparer.java

+36 −18
Original line number Diff line number Diff line
@@ -23,10 +23,10 @@ import android.content.pm.UserInfo; 
import android.os.Environment;

import android.os.FileUtils;

import android.os.RecoverySystem;

import android.os.storage.StorageManager;

import android.os.storage.VolumeInfo;

import android.os.SystemProperties;

import android.os.UserHandle;

import android.os.storage.StorageManager;

import android.os.storage.VolumeInfo;

import android.system.ErrnoException;

import android.system.Os;

import android.system.OsConstants;
@@ -35,6 +35,7 @@ import android.util.Slog; 
import android.util.SparseArray;



import com.android.internal.annotations.VisibleForTesting;

import com.android.server.utils.Slogf;



import java.io.File;

import java.io.IOException;
@@ -43,7 +44,6 @@ import java.util.ArrayList; 
import java.util.Collections;

import java.util.List;

import java.util.Objects;

import java.util.Set;



/**

 * Helper class for preparing and destroying user storage
@@ -65,31 +65,37 @@ class UserDataPreparer { 
    /**

     * Prepare storage areas for given user on all mounted devices.

     */

    void prepareUserData(int userId, int userSerial, int flags) {

    void prepareUserData(UserInfo userInfo, int flags) {

        synchronized (mInstallLock) {

            final StorageManager storage = mContext.getSystemService(StorageManager.class);

            /*

             * Internal storage must be prepared before adoptable storage, since the user's volume

             * keys are stored in their internal storage.

             */

            prepareUserDataLI(null /* internal storage */, userId, userSerial, flags, true);

            prepareUserDataLI(null /* internal storage */, userInfo, flags, true);

            for (VolumeInfo vol : storage.getWritablePrivateVolumes()) {

                final String volumeUuid = vol.getFsUuid();

                if (volumeUuid != null) {

                    prepareUserDataLI(volumeUuid, userId, userSerial, flags, true);

                    prepareUserDataLI(volumeUuid, userInfo, flags, true);

                }

            }

        }

    }



    private void prepareUserDataLI(String volumeUuid, int userId, int userSerial, int flags,

    private void prepareUserDataLI(String volumeUuid, UserInfo userInfo, int flags,

            boolean allowRecover) {

        // Prepare storage and verify that serial numbers are consistent; if

        // there's a mismatch we need to destroy to avoid leaking data

        final int userId = userInfo.id;

        final int userSerial = userInfo.serialNumber;

        final StorageManager storage = mContext.getSystemService(StorageManager.class);

        final boolean isNewUser = userInfo.lastLoggedInTime == 0;

        Slogf.d(TAG, "Preparing user data; volumeUuid=%s, userId=%d, flags=0x%x, isNewUser=%s",

                volumeUuid, userId, flags, isNewUser);

        try {

            // Prepare CE and/or DE storage.

            storage.prepareUserStorage(volumeUuid, userId, userSerial, flags);



            // Ensure that the data directories of a removed user with the same ID are not being

            // reused.  New users must get fresh data directories, to avoid leaking data.

            if ((flags & StorageManager.FLAG_STORAGE_DE) != 0) {

                enforceSerialNumber(getDataUserDeDirectory(volumeUuid, userId), userSerial);

                if (Objects.equals(volumeUuid, StorageManager.UUID_PRIVATE_INTERNAL)) {
@@ -103,9 +109,10 @@ class UserDataPreparer { 
                }

            }



            // Prepare the app data directories.

            mInstaller.createUserData(volumeUuid, userId, userSerial, flags);



            // CE storage is available after they are prepared.

            // If applicable, record that the system user's CE storage has been prepared.

            if ((flags & StorageManager.FLAG_STORAGE_CE) != 0 &&

                    (userId == UserHandle.USER_SYSTEM)) {

                String propertyName = "sys.user." + userId + ".ce_available";
@@ -113,20 +120,31 @@ class UserDataPreparer { 
                SystemProperties.set(propertyName, "true");

            }

        } catch (Exception e) {

            logCriticalInfo(Log.WARN, "Destroying user " + userId + " on volume " + volumeUuid

            // Failed to prepare user data.  For new users, specifically users that haven't ever

            // been unlocked, destroy the user data, and try again (if not already retried).  This

            // might be effective at resolving some errors, such as stale directories from a reused

            // user ID.  Don't auto-destroy data for existing users, since issues with existing

            // users might be fixable via an OTA without having to wipe the user's data.

            if (isNewUser) {

                logCriticalInfo(Log.ERROR, "Destroying user " + userId + " on volume " + volumeUuid

                        + " because we failed to prepare: " + e);

                destroyUserDataLI(volumeUuid, userId, flags);



            } else {

                logCriticalInfo(Log.ERROR, "Failed to prepare user " + userId + " on volume "

                        + volumeUuid + ": " + e);

            }

            if (allowRecover) {

                // Try one last time; if we fail again we're really in trouble

                prepareUserDataLI(volumeUuid, userId, userSerial,

                    flags | StorageManager.FLAG_STORAGE_DE, false);

                prepareUserDataLI(volumeUuid, userInfo, flags | StorageManager.FLAG_STORAGE_DE,

                        false);

            } else {

                // If internal storage of the system user fails to prepare on first boot, then

                // things are *really* broken, so we might as well reboot to recovery right away.

                try {

                    Log.wtf(TAG, "prepareUserData failed for user " + userId, e);

                    if (userId == UserHandle.USER_SYSTEM) {

                    if (isNewUser && userId == UserHandle.USER_SYSTEM && volumeUuid == null) {

                        RecoverySystem.rebootPromptAndWipeUserData(mContext,

                                "prepareUserData failed for system user");

                                "failed to prepare internal storage for system user");

                    }

                } catch (IOException e2) {

                    throw new RuntimeException("error rebooting into recovery", e2);

services/core/java/com/android/server/pm/UserManagerService.java

+3 −6
Original line number Diff line number Diff line
@@ -4914,8 +4914,7 @@ public class UserManagerService extends IUserManager.Stub { 
            // unlocked.  We do this to ensure that CE storage isn't prepared before the CE key is

            // saved to disk.  This also matches what is done for user 0.

            t.traceBegin("prepareUserData");

            mUserDataPreparer.prepareUserData(userId, userInfo.serialNumber,

                    StorageManager.FLAG_STORAGE_DE);

            mUserDataPreparer.prepareUserData(userInfo, StorageManager.FLAG_STORAGE_DE);

            t.traceEnd();



            t.traceBegin("LSS.createNewUser");
@@ -6199,12 +6198,11 @@ public class UserManagerService extends IUserManager.Stub { 
        }

        TimingsTraceAndSlog t = new TimingsTraceAndSlog();

        t.traceBegin("onBeforeStartUser-" + userId);

        final int userSerial = userInfo.serialNumber;

        // Migrate only if build fingerprints mismatch

        boolean migrateAppsData = !PackagePartitions.FINGERPRINT.equals(

                userInfo.lastLoggedInFingerprint);

        t.traceBegin("prepareUserData");

        mUserDataPreparer.prepareUserData(userId, userSerial, StorageManager.FLAG_STORAGE_DE);

        mUserDataPreparer.prepareUserData(userInfo, StorageManager.FLAG_STORAGE_DE);

        t.traceEnd();

        t.traceBegin("reconcileAppsData");

        getPackageManagerInternal().reconcileAppsData(userId, StorageManager.FLAG_STORAGE_DE,
@@ -6230,14 +6228,13 @@ public class UserManagerService extends IUserManager.Stub { 
        if (userInfo == null) {

            return;

        }

        final int userSerial = userInfo.serialNumber;

        // Migrate only if build fingerprints mismatch

        boolean migrateAppsData = !PackagePartitions.FINGERPRINT.equals(

                userInfo.lastLoggedInFingerprint);



        final TimingsTraceAndSlog t = new TimingsTraceAndSlog();

        t.traceBegin("prepareUserData-" + userId);

        mUserDataPreparer.prepareUserData(userId, userSerial, StorageManager.FLAG_STORAGE_CE);

        mUserDataPreparer.prepareUserData(userInfo, StorageManager.FLAG_STORAGE_CE);

        t.traceEnd();



        StorageManagerInternal smInternal = LocalServices.getService(StorageManagerInternal.class);

services/tests/PackageManagerServiceTests/server/src/com/android/server/pm/UserDataPreparerTest.java

+29 −4
Original line number Diff line number Diff line
@@ -21,6 +21,8 @@ import static org.junit.Assert.assertFalse; 
import static org.junit.Assert.assertTrue;

import static org.mockito.Matchers.eq;

import static org.mockito.Matchers.isNull;

import static org.mockito.Mockito.doThrow;

import static org.mockito.Mockito.never;

import static org.mockito.Mockito.verify;

import static org.mockito.Mockito.when;
@@ -56,6 +58,7 @@ public class UserDataPreparerTest { 


    private static final int TEST_USER_SERIAL = 1000;

    private static final int TEST_USER_ID = 10;

    private static final UserInfo TEST_USER = new UserInfo();



    private TestUserDataPreparer mUserDataPreparer;
@@ -72,6 +75,8 @@ public class UserDataPreparerTest { 


    @Before

    public void setup() {

        TEST_USER.id = TEST_USER_ID;

        TEST_USER.serialNumber = TEST_USER_SERIAL;

        Context ctx = InstrumentationRegistry.getContext();

        FileUtils.deleteContents(ctx.getCacheDir());

        mInstallLock = new Object();
@@ -92,8 +97,7 @@ public class UserDataPreparerTest { 
        userDeDir.mkdirs();

        File systemDeDir = mUserDataPreparer.getDataSystemDeDirectory(TEST_USER_ID);

        systemDeDir.mkdirs();

        mUserDataPreparer

                .prepareUserData(TEST_USER_ID, TEST_USER_SERIAL, StorageManager.FLAG_STORAGE_DE);

        mUserDataPreparer.prepareUserData(TEST_USER, StorageManager.FLAG_STORAGE_DE);

        verify(mStorageManagerMock).prepareUserStorage(isNull(String.class), eq(TEST_USER_ID),

                eq(TEST_USER_SERIAL), eq(StorageManager.FLAG_STORAGE_DE));

        verify(mInstaller).createUserData(isNull(String.class), eq(TEST_USER_ID),
@@ -110,8 +114,7 @@ public class UserDataPreparerTest { 
        userCeDir.mkdirs();

        File systemCeDir = mUserDataPreparer.getDataSystemCeDirectory(TEST_USER_ID);

        systemCeDir.mkdirs();

        mUserDataPreparer

                .prepareUserData(TEST_USER_ID, TEST_USER_SERIAL, StorageManager.FLAG_STORAGE_CE);

        mUserDataPreparer.prepareUserData(TEST_USER, StorageManager.FLAG_STORAGE_CE);

        verify(mStorageManagerMock).prepareUserStorage(isNull(String.class), eq(TEST_USER_ID),

                eq(TEST_USER_SERIAL), eq(StorageManager.FLAG_STORAGE_CE));

        verify(mInstaller).createUserData(isNull(String.class), eq(TEST_USER_ID),
@@ -122,6 +125,28 @@ public class UserDataPreparerTest { 
        assertEquals(TEST_USER_SERIAL, serialNumber);

    }



    @Test

    public void testPrepareUserData_forNewUser_destroysOnFailure() throws Exception {

        TEST_USER.lastLoggedInTime = 0;

        doThrow(new IllegalStateException("expected exception for test")).when(mStorageManagerMock)

                .prepareUserStorage(isNull(String.class), eq(TEST_USER_ID), eq(TEST_USER_SERIAL),

                        eq(StorageManager.FLAG_STORAGE_CE));

        mUserDataPreparer.prepareUserData(TEST_USER, StorageManager.FLAG_STORAGE_CE);

        verify(mStorageManagerMock).destroyUserStorage(isNull(String.class), eq(TEST_USER_ID),

                eq(StorageManager.FLAG_STORAGE_CE));

    }



    @Test

    public void testPrepareUserData_forExistingUser_doesNotDestroyOnFailure() throws Exception {

        TEST_USER.lastLoggedInTime = System.currentTimeMillis();

        doThrow(new IllegalStateException("expected exception for test")).when(mStorageManagerMock)

                .prepareUserStorage(isNull(String.class), eq(TEST_USER_ID), eq(TEST_USER_SERIAL),

                        eq(StorageManager.FLAG_STORAGE_CE));

        mUserDataPreparer.prepareUserData(TEST_USER, StorageManager.FLAG_STORAGE_CE);

        verify(mStorageManagerMock, never()).destroyUserStorage(isNull(String.class),

                eq(TEST_USER_ID), eq(StorageManager.FLAG_STORAGE_CE));

    }



    @Test

    public void testDestroyUserData_De_DoesNotDestroyCe() throws Exception {

        // Add file in CE storage