From a703d2f5cb4c80b5b3632e3d3f51cdb379862465 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 29 Apr 2022 05:26:17 +0000 Subject: [PATCH] UserDataPreparer: only delete contents of user's system CE and DE dirs The /data/system_ce/$userId and /data/system_de/$userId directories are created by vold, so they should be deleted by vold as well, and in fact that would already happen except that system_server deletes them recursively before vold gets to it. Change system_server to delete just the contents of these directories. This is a prerequisite to locking down the ability to create these directories (https://r.android.com/2078213), which is needed to stop subdirectories from accidentally being created too early. Technically we could achieve this goal without limiting delete access, as it's create access that really matters, but having the operations be paired properly is much cleaner. Test: Created and deleted a user, and verified that all their directories still got deleted. Test: atest UserDataPreparerTest Bug: 156305599 Change-Id: Iec908e1bc15a00c7179fcd1d80321c315682d339 --- .../android/server/pm/UserDataPreparer.java | 10 +++++-- .../server/pm/UserDataPreparerTest.java | 29 ++++++++++++------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/services/core/java/com/android/server/pm/UserDataPreparer.java b/services/core/java/com/android/server/pm/UserDataPreparer.java index 504769064808c..5418dc59097cd 100644 --- a/services/core/java/com/android/server/pm/UserDataPreparer.java +++ b/services/core/java/com/android/server/pm/UserDataPreparer.java @@ -150,14 +150,18 @@ class UserDataPreparer { if (Objects.equals(volumeUuid, StorageManager.UUID_PRIVATE_INTERNAL)) { if ((flags & StorageManager.FLAG_STORAGE_DE) != 0) { FileUtils.deleteContentsAndDir(getUserSystemDirectory(userId)); - FileUtils.deleteContentsAndDir(getDataSystemDeDirectory(userId)); + // Delete the contents of /data/system_de/$userId, but not the directory itself + // since vold is responsible for that and system_server isn't allowed to do it. + FileUtils.deleteContents(getDataSystemDeDirectory(userId)); } if ((flags & StorageManager.FLAG_STORAGE_CE) != 0) { - FileUtils.deleteContentsAndDir(getDataSystemCeDirectory(userId)); + // Likewise, delete the contents of /data/system_ce/$userId but not the + // directory itself. + FileUtils.deleteContents(getDataSystemCeDirectory(userId)); } } - // Data with special labels is now gone, so finish the job + // All the user's data directories should be empty now, so finish the job. storage.destroyUserStorage(volumeUuid, userId, flags); } catch (Exception e) { diff --git a/services/tests/servicestests/src/com/android/server/pm/UserDataPreparerTest.java b/services/tests/servicestests/src/com/android/server/pm/UserDataPreparerTest.java index c489cf0a138d4..de83e518067e1 100644 --- a/services/tests/servicestests/src/com/android/server/pm/UserDataPreparerTest.java +++ b/services/tests/servicestests/src/com/android/server/pm/UserDataPreparerTest.java @@ -17,6 +17,8 @@ package com.android.server.pm; import static org.junit.Assert.assertEquals; +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.verify; @@ -129,22 +131,16 @@ public class UserDataPreparerTest { } @Test - public void testDestroyUserData() throws Exception { - // Add file in CE + public void testDestroyUserData_De_DoesNotDestroyCe() throws Exception { + // Add file in CE storage File systemCeDir = mUserDataPreparer.getDataSystemCeDirectory(TEST_USER_ID); systemCeDir.mkdirs(); File ceFile = new File(systemCeDir, "file"); writeFile(ceFile, "-----" ); - testDestroyUserData_De(); - // CE directory should be preserved + // Destroy DE storage, then verify that CE storage wasn't destroyed too. + mUserDataPreparer.destroyUserData(TEST_USER_ID, StorageManager.FLAG_STORAGE_DE); assertEquals(Collections.singletonList(ceFile), Arrays.asList(FileUtils.listFilesOrEmpty( systemCeDir))); - - testDestroyUserData_Ce(); - - // Verify that testDir is empty - assertEquals(Collections.emptyList(), Arrays.asList(FileUtils.listFilesOrEmpty( - mUserDataPreparer.testDir))); } @Test @@ -163,7 +159,13 @@ public class UserDataPreparerTest { verify(mStorageManagerMock).destroyUserStorage(isNull(String.class), eq(TEST_USER_ID), eq(StorageManager.FLAG_STORAGE_DE)); - assertEquals(Collections.emptyList(), Arrays.asList(FileUtils.listFilesOrEmpty(systemDir))); + // systemDir (normal path: /data/system/users/$userId) should have been deleted. + assertFalse(systemDir.exists()); + // systemDeDir (normal path: /data/system_de/$userId) should still exist but be empty, since + // UserDataPreparer itself is responsible for deleting the contents of this directory, but + // it delegates to StorageManager.destroyUserStorage() for deleting the directory itself. + // We've mocked out StorageManager, so StorageManager.destroyUserStorage() will be a no-op. + assertTrue(systemDeDir.exists()); assertEquals(Collections.emptyList(), Arrays.asList(FileUtils.listFilesOrEmpty( systemDeDir))); } @@ -181,6 +183,11 @@ public class UserDataPreparerTest { verify(mStorageManagerMock).destroyUserStorage(isNull(String.class), eq(TEST_USER_ID), eq(StorageManager.FLAG_STORAGE_CE)); + // systemCeDir (normal path: /data/system_ce/$userId) should still exist but be empty, since + // UserDataPreparer itself is responsible for deleting the contents of this directory, but + // it delegates to StorageManager.destroyUserStorage() for deleting the directory itself. + // We've mocked out StorageManager, so StorageManager.destroyUserStorage() will be a no-op. + assertTrue(systemCeDir.exists()); assertEquals(Collections.emptyList(), Arrays.asList(FileUtils.listFilesOrEmpty( systemCeDir))); }