Merge "Delete a blob after the last lease of it is released." into rvc-dev am: 0c0abbf7f7

Change-Id: Ia802e29ae18ab2148c171ea800179ae2ee306c28

apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java

@@ -32,6 +32,7 @@ import static android.system.OsConstants.O_RDONLY;
 import static com.android.server.blob.BlobStoreConfig.TAG;
 import static com.android.server.blob.BlobStoreConfig.XML_VERSION_ADD_DESC_RES_NAME;
 import static com.android.server.blob.BlobStoreConfig.XML_VERSION_ADD_STRING_DESC;
+import static com.android.server.blob.BlobStoreConfig.hasLeaseWaitTimeElapsed;
 import static com.android.server.blob.BlobStoreUtils.getDescriptionResourceId;
 import static com.android.server.blob.BlobStoreUtils.getPackageResources;
 
@@ -227,6 +228,35 @@ class BlobMetadata {
         return false;
     }
 
+    boolean isACommitter(@NonNull String packageName, int uid) {
+        synchronized (mMetadataLock) {
+            return isAnAccessor(mCommitters, packageName, uid);
+        }
+    }
+
+    boolean isALeasee(@Nullable String packageName, int uid) {
+        synchronized (mMetadataLock) {
+            return isAnAccessor(mLeasees, packageName, uid);
+        }
+    }
+
+    private static <T extends Accessor> boolean isAnAccessor(@NonNull ArraySet<T> accessors,
+            @Nullable String packageName, int uid) {
+        // Check if the package is an accessor of the data blob.
+        for (int i = 0, size = accessors.size(); i < size; ++i) {
+            final Accessor accessor = accessors.valueAt(i);
+            if (packageName != null && uid != INVALID_UID
+                    && accessor.equals(packageName, uid)) {
+                return true;
+            } else if (packageName != null && accessor.packageName.equals(packageName)) {
+                return true;
+            } else if (uid != INVALID_UID && accessor.uid == uid) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     boolean isALeasee(@NonNull String packageName) {
         return isALeasee(packageName, INVALID_UID);
     }
@@ -243,24 +273,6 @@ class BlobMetadata {
         return hasOtherLeasees(null, uid);
     }
 
-    boolean isALeasee(@Nullable String packageName, int uid) {
-        synchronized (mMetadataLock) {
-            // Check if the package is a leasee of the data blob.
-            for (int i = 0, size = mLeasees.size(); i < size; ++i) {
-                final Leasee leasee = mLeasees.valueAt(i);
-                if (packageName != null && uid != INVALID_UID
-                        && leasee.equals(packageName, uid)) {
-                    return true;
-                } else if (packageName != null && leasee.packageName.equals(packageName)) {
-                    return true;
-                } else if (uid != INVALID_UID && leasee.uid == uid) {
-                    return true;
-                }
-            }
-        }
-        return false;
-    }
-
     private boolean hasOtherLeasees(@Nullable String packageName, int uid) {
         synchronized (mMetadataLock) {
             if (mCommitters.size() > 1 || mLeasees.size() > 1) {
@@ -355,6 +367,22 @@ class BlobMetadata {
         return revocableFd.getRevocableFileDescriptor();
     }
 
+    boolean shouldBeDeleted(boolean respectLeaseWaitTime) {
+        // Expired data blobs
+        if (getBlobHandle().isExpired()) {
+            return true;
+        }
+
+        // Blobs with no active leases
+        // TODO: Track commit time instead of using last modified time.
+        if ((!respectLeaseWaitTime || hasLeaseWaitTimeElapsed(getBlobFile().lastModified()))
+                && !hasLeases()) {
+            return true;
+        }
+
+        return false;
+    }
+
     void dump(IndentingPrintWriter fout, DumpArgs dumpArgs) {
         fout.println("blobHandle:");
         fout.increaseIndent();

apex/blobstore/service/java/com/android/server/blob/BlobStoreConfig.java

@@ -29,6 +29,7 @@ import android.provider.DeviceConfig.Properties;
 import android.util.DataUnit;
 import android.util.Log;
 import android.util.Slog;
+import android.util.TimeUtils;
 
 import com.android.internal.util.IndentingPrintWriter;
 
@@ -88,6 +89,17 @@ class BlobStoreConfig {
         public static float TOTAL_BYTES_PER_APP_LIMIT_FRACTION =
                 DEFAULT_TOTAL_BYTES_PER_APP_LIMIT_FRACTION;
 
+        /**
+         * Denotes the duration from the time a blob is committed that we wait for a lease to
+         * be acquired before deciding to delete the blob for having no leases.
+         */
+        public static final String KEY_LEASE_ACQUISITION_WAIT_DURATION_MS =
+                "lease_acquisition_wait_time_ms";
+        public static final long DEFAULT_LEASE_ACQUISITION_WAIT_DURATION_MS =
+                TimeUnit.HOURS.toMillis(6);
+        public static long LEASE_ACQUISITION_WAIT_DURATION_MS =
+                DEFAULT_LEASE_ACQUISITION_WAIT_DURATION_MS;
+
         static void refresh(Properties properties) {
             if (!NAMESPACE_BLOBSTORE.equals(properties.getNamespace())) {
                 return;
@@ -102,6 +114,10 @@ class BlobStoreConfig {
                         TOTAL_BYTES_PER_APP_LIMIT_FRACTION = properties.getFloat(key,
                                 DEFAULT_TOTAL_BYTES_PER_APP_LIMIT_FRACTION);
                         break;
+                    case KEY_LEASE_ACQUISITION_WAIT_DURATION_MS:
+                        LEASE_ACQUISITION_WAIT_DURATION_MS = properties.getLong(key,
+                                DEFAULT_LEASE_ACQUISITION_WAIT_DURATION_MS);
+                        break;
                     default:
                         Slog.wtf(TAG, "Unknown key in device config properties: " + key);
                 }
@@ -117,6 +133,9 @@ class BlobStoreConfig {
             fout.println(String.format(dumpFormat, KEY_TOTAL_BYTES_PER_APP_LIMIT_FRACTION,
                     TOTAL_BYTES_PER_APP_LIMIT_FRACTION,
                     DEFAULT_TOTAL_BYTES_PER_APP_LIMIT_FRACTION));
+            fout.println(String.format(dumpFormat, KEY_LEASE_ACQUISITION_WAIT_DURATION_MS,
+                    TimeUtils.formatDuration(LEASE_ACQUISITION_WAIT_DURATION_MS),
+                    TimeUtils.formatDuration(DEFAULT_LEASE_ACQUISITION_WAIT_DURATION_MS)));
         }
     }
 
@@ -136,6 +155,14 @@ class BlobStoreConfig {
         return Math.max(DeviceConfigProperties.TOTAL_BYTES_PER_APP_LIMIT_FLOOR, totalBytesLimit);
     }
 
+    /**
+     * Returns whether the wait time for lease acquisition for a blob has elapsed.
+     */
+    public static boolean hasLeaseWaitTimeElapsed(long commitTimeMs) {
+        return commitTimeMs + DeviceConfigProperties.LEASE_ACQUISITION_WAIT_DURATION_MS
+                < System.currentTimeMillis();
+    }
+
     @Nullable
     public static File prepareBlobFile(long sessionId) {
         final File blobsDir = prepareBlobsDir();

apex/blobstore/service/java/com/android/server/blob/BlobStoreManagerService.java

@@ -424,8 +424,9 @@ public class BlobStoreManagerService extends SystemService {
     private void releaseLeaseInternal(BlobHandle blobHandle, int callingUid,
             String callingPackage) {
         synchronized (mBlobsLock) {
-            final BlobMetadata blobMetadata = getUserBlobsLocked(UserHandle.getUserId(callingUid))
+            final ArrayMap<BlobHandle, BlobMetadata> userBlobs =
-                    .get(blobHandle);
+                    getUserBlobsLocked(UserHandle.getUserId(callingUid));
+            final BlobMetadata blobMetadata = userBlobs.get(blobHandle);
             if (blobMetadata == null || !blobMetadata.isAccessAllowedForCaller(
                     callingPackage, callingUid)) {
                 throw new SecurityException("Caller not allowed to access " + blobHandle
@@ -436,6 +437,10 @@ public class BlobStoreManagerService extends SystemService {
                 Slog.v(TAG, "Released lease on " + blobHandle
                         + "; callingUid=" + callingUid + ", callingPackage=" + callingPackage);
             }
+            if (blobMetadata.shouldBeDeleted(true /* respectLeaseWaitTime */)) {
+                deleteBlobLocked(blobMetadata);
+                userBlobs.remove(blobHandle);
+            }
             writeBlobsInfoAsync();
         }
     }
@@ -863,12 +868,15 @@ public class BlobStoreManagerService extends SystemService {
                     getUserBlobsLocked(UserHandle.getUserId(uid));
             userBlobs.entrySet().removeIf(entry -> {
                 final BlobMetadata blobMetadata = entry.getValue();
-                blobMetadata.removeCommitter(packageName, uid);
+                final boolean isACommitter = blobMetadata.isACommitter(packageName, uid);
+                if (isACommitter) {
+                    blobMetadata.removeCommitter(packageName, uid);
+                }
                 blobMetadata.removeLeasee(packageName, uid);
-                // Delete the blob if it doesn't have any active leases.
+                // Regardless of when the blob is committed, we need to delete
-                if (!blobMetadata.hasLeases()) {
+                // it if it was from the deleted package to ensure we delete all traces of it.
-                    blobMetadata.getBlobFile().delete();
+                if (blobMetadata.shouldBeDeleted(isACommitter /* respectLeaseWaitTime */)) {
-                    mActiveBlobIds.remove(blobMetadata.getBlobId());
+                    deleteBlobLocked(blobMetadata);
                     return true;
                 }
                 return false;
@@ -899,8 +907,7 @@ public class BlobStoreManagerService extends SystemService {
             if (userBlobs != null) {
                 for (int i = 0, count = userBlobs.size(); i < count; ++i) {
                     final BlobMetadata blobMetadata = userBlobs.valueAt(i);
-                    blobMetadata.getBlobFile().delete();
+                    deleteBlobLocked(blobMetadata);
-                    mActiveBlobIds.remove(blobMetadata.getBlobId());
                 }
             }
             if (LOGV) {
@@ -938,27 +945,14 @@ public class BlobStoreManagerService extends SystemService {
         for (int i = 0, userCount = mBlobsMap.size(); i < userCount; ++i) {
             final ArrayMap<BlobHandle, BlobMetadata> userBlobs = mBlobsMap.valueAt(i);
             userBlobs.entrySet().removeIf(entry -> {
-                final BlobHandle blobHandle = entry.getKey();
                 final BlobMetadata blobMetadata = entry.getValue();
-                boolean shouldRemove = false;
 
-                // Cleanup expired data blobs.
+                if (blobMetadata.shouldBeDeleted(true /* respectLeaseWaitTime */)) {
-                if (blobHandle.isExpired()) {
+                    deleteBlobLocked(blobMetadata);
-                    shouldRemove = true;
-                }
-
-                // Cleanup blobs with no active leases.
-                // TODO: Exclude blobs which were just committed.
-                if (!blobMetadata.hasLeases()) {
-                    shouldRemove = true;
-                }
-
-                if (shouldRemove) {
-                    blobMetadata.getBlobFile().delete();
-                    mActiveBlobIds.remove(blobMetadata.getBlobId());
                     deletedBlobIds.add(blobMetadata.getBlobId());
+                    return true;
                 }
-                return shouldRemove;
+                return false;
             });
         }
         writeBlobsInfoAsync();
@@ -995,6 +989,12 @@ public class BlobStoreManagerService extends SystemService {
         writeBlobSessionsAsync();
     }
 
+    @GuardedBy("mBlobsLock")
+    private void deleteBlobLocked(BlobMetadata blobMetadata) {
+        blobMetadata.getBlobFile().delete();
+        mActiveBlobIds.remove(blobMetadata.getBlobId());
+    }
+
     void runClearAllSessions(@UserIdInt int userId) {
         synchronized (mBlobsLock) {
             if (userId == UserHandle.USER_ALL) {
@@ -1024,9 +1024,8 @@ public class BlobStoreManagerService extends SystemService {
             if (blobMetadata == null) {
                 return;
             }
-            blobMetadata.getBlobFile().delete();
+            deleteBlobLocked(blobMetadata);
             userBlobs.remove(blobHandle);
-            mActiveBlobIds.remove(blobMetadata.getBlobId());
             writeBlobsInfoAsync();
         }
     }

services/tests/mockingservicestests/src/com/android/server/blob/BlobStoreManagerServiceTest.java

@@ -15,6 +15,7 @@
  */
 package com.android.server.blob;
 
+import static com.android.dx.mockito.inline.extended.ExtendedMockito.doCallRealMethod;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.doReturn;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.mock;
 import static com.android.dx.mockito.inline.extended.ExtendedMockito.mockitoSession;
@@ -22,6 +23,8 @@ import static com.android.server.blob.BlobStoreConfig.SESSION_EXPIRY_TIMEOUT_MIL
 
 import static com.google.common.truth.Truth.assertThat;
 
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
@@ -89,6 +92,7 @@ public class BlobStoreManagerServiceTest {
         doReturn(mBlobsDir).when(() -> BlobStoreConfig.getBlobsDir());
         doReturn(true).when(mBlobsDir).exists();
         doReturn(new File[0]).when(mBlobsDir).listFiles();
+        doReturn(true).when(() -> BlobStoreConfig.hasLeaseWaitTimeElapsed(anyLong()));
 
         mContext = InstrumentationRegistry.getTargetContext();
         mHandler = new TestHandler(Looper.getMainLooper());
@@ -138,19 +142,32 @@ public class BlobStoreManagerServiceTest {
         final long blobId1 = 978;
         final File blobFile1 = mock(File.class);
         final BlobHandle blobHandle1 = BlobHandle.createWithSha256("digest1".getBytes(),
-                "label1", System.currentTimeMillis(), "tag1");
+                "label1", System.currentTimeMillis() + 10000, "tag1");
-        final BlobMetadata blobMetadata1 = createBlobMetadataMock(blobId1, blobFile1, true);
+        final BlobMetadata blobMetadata1 = createBlobMetadataMock(blobId1, blobFile1,
+                blobHandle1, true /* hasLeases */);
+        doReturn(true).when(blobMetadata1).isACommitter(TEST_PKG1, TEST_UID1);
         mUserBlobs.put(blobHandle1, blobMetadata1);
 
         final long blobId2 = 347;
         final File blobFile2 = mock(File.class);
         final BlobHandle blobHandle2 = BlobHandle.createWithSha256("digest2".getBytes(),
-                "label2", System.currentTimeMillis(), "tag2");
+                "label2", System.currentTimeMillis() + 20000, "tag2");
-        final BlobMetadata blobMetadata2 = createBlobMetadataMock(blobId2, blobFile2, false);
+        final BlobMetadata blobMetadata2 = createBlobMetadataMock(blobId2, blobFile2,
+                blobHandle2, false /* hasLeases */);
+        doReturn(false).when(blobMetadata2).isACommitter(TEST_PKG1, TEST_UID1);
         mUserBlobs.put(blobHandle2, blobMetadata2);
 
+        final long blobId3 = 49875;
+        final File blobFile3 = mock(File.class);
+        final BlobHandle blobHandle3 = BlobHandle.createWithSha256("digest3".getBytes(),
+                "label3", System.currentTimeMillis() - 1000, "tag3");
+        final BlobMetadata blobMetadata3 = createBlobMetadataMock(blobId3, blobFile3,
+                blobHandle3, true /* hasLeases */);
+        doReturn(true).when(blobMetadata3).isACommitter(TEST_PKG1, TEST_UID1);
+        mUserBlobs.put(blobHandle3, blobMetadata3);
+
         mService.addActiveIdsForTest(sessionId1, sessionId2, sessionId3, sessionId4,
-                blobId1, blobId2);
+                blobId1, blobId2, blobId3);
 
         // Invoke test method
         mService.handlePackageRemoved(TEST_PKG1, TEST_UID1);
@@ -170,20 +187,24 @@ public class BlobStoreManagerServiceTest {
         // Verify blobs are removed
         verify(blobMetadata1).removeCommitter(TEST_PKG1, TEST_UID1);
         verify(blobMetadata1).removeLeasee(TEST_PKG1, TEST_UID1);
-        verify(blobMetadata2).removeCommitter(TEST_PKG1, TEST_UID1);
+        verify(blobMetadata2, never()).removeCommitter(TEST_PKG1, TEST_UID1);
         verify(blobMetadata2).removeLeasee(TEST_PKG1, TEST_UID1);
+        verify(blobMetadata3).removeCommitter(TEST_PKG1, TEST_UID1);
+        verify(blobMetadata3).removeLeasee(TEST_PKG1, TEST_UID1);
 
         verify(blobFile1, never()).delete();
         verify(blobFile2).delete();
+        verify(blobFile3).delete();
 
         assertThat(mUserBlobs.size()).isEqualTo(1);
         assertThat(mUserBlobs.get(blobHandle1)).isNotNull();
         assertThat(mUserBlobs.get(blobHandle2)).isNull();
+        assertThat(mUserBlobs.get(blobHandle3)).isNull();
 
         assertThat(mService.getActiveIdsForTest()).containsExactly(
                 sessionId2, sessionId3, blobId1);
         assertThat(mService.getKnownIdsForTest()).containsExactly(
-                sessionId1, sessionId2, sessionId3, sessionId4, blobId1, blobId2);
+                sessionId1, sessionId2, sessionId3, sessionId4, blobId1, blobId2, blobId3);
     }
 
     @Test
@@ -269,21 +290,24 @@ public class BlobStoreManagerServiceTest {
         final File blobFile1 = mock(File.class);
         final BlobHandle blobHandle1 = BlobHandle.createWithSha256("digest1".getBytes(),
                 "label1", System.currentTimeMillis() - 2000, "tag1");
-        final BlobMetadata blobMetadata1 = createBlobMetadataMock(blobId1, blobFile1, true);
+        final BlobMetadata blobMetadata1 = createBlobMetadataMock(blobId1, blobFile1, blobHandle1,
+                true /* hasLeases */);
         mUserBlobs.put(blobHandle1, blobMetadata1);
 
         final long blobId2 = 78974;
         final File blobFile2 = mock(File.class);
         final BlobHandle blobHandle2 = BlobHandle.createWithSha256("digest2".getBytes(),
                 "label2", System.currentTimeMillis() + 30000, "tag2");
-        final BlobMetadata blobMetadata2 = createBlobMetadataMock(blobId2, blobFile2, true);
+        final BlobMetadata blobMetadata2 = createBlobMetadataMock(blobId2, blobFile2, blobHandle2,
+                true /* hasLeases */);
         mUserBlobs.put(blobHandle2, blobMetadata2);
 
         final long blobId3 = 97;
         final File blobFile3 = mock(File.class);
         final BlobHandle blobHandle3 = BlobHandle.createWithSha256("digest3".getBytes(),
                 "label3", System.currentTimeMillis() + 4400000, "tag3");
-        final BlobMetadata blobMetadata3 = createBlobMetadataMock(blobId3, blobFile3, false);
+        final BlobMetadata blobMetadata3 = createBlobMetadataMock(blobId3, blobFile3, blobHandle3,
+                false /* hasLeases */);
         mUserBlobs.put(blobHandle3, blobMetadata3);
 
         mService.addActiveIdsForTest(blobId1, blobId2, blobId3);
@@ -350,11 +374,14 @@ public class BlobStoreManagerServiceTest {
         return session;
     }
 
-    private BlobMetadata createBlobMetadataMock(long blobId, File blobFile, boolean hasLeases) {
+    private BlobMetadata createBlobMetadataMock(long blobId, File blobFile,
+            BlobHandle blobHandle, boolean hasLeases) {
         final BlobMetadata blobMetadata = mock(BlobMetadata.class);
         doReturn(blobId).when(blobMetadata).getBlobId();
         doReturn(blobFile).when(blobMetadata).getBlobFile();
         doReturn(hasLeases).when(blobMetadata).hasLeases();
+        doReturn(blobHandle).when(blobMetadata).getBlobHandle();
+        doCallRealMethod().when(blobMetadata).shouldBeDeleted(anyBoolean());
         return blobMetadata;
     }
 

tests/BlobStoreTestUtils/src/com/android/utils/blob/DummyBlobData.java

@@ -46,7 +46,7 @@ public class DummyBlobData {
     byte[] mFileDigest;
     long mExpiryTimeMs;
 
-    public DummyBlobData(Builder builder) {
+    private DummyBlobData(Builder builder) {
         mRandom = new Random(builder.getRandomSeed());
         mFile = new File(builder.getContext().getFilesDir(), builder.getFileName());
         mFileSize = builder.getFileSize();

tests/BlobStoreTestUtils/src/com/android/utils/blob/Utils.java

@@ -124,7 +124,11 @@ public class Utils {
         final BlobStoreManager blobStoreManager = (BlobStoreManager) context.getSystemService(
                 Context.BLOB_STORE_SERVICE);
         blobStoreManager.releaseLease(blobHandle);
-        assertThat(blobStoreManager.getLeaseInfo(blobHandle)).isNull();
+        try {
+            assertThat(blobStoreManager.getLeaseInfo(blobHandle)).isNull();
+        } catch (SecurityException e) {
+            // Expected, ignore
+        }
     }
 
     private static void assertLeaseInfo(LeaseInfo leaseInfo, String packageName,