Merge "Ensure expired leases are ignored and deleted." into rvc-dev am: 70f5e3e88b

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/11996896

Change-Id: I8630ccefb7e83fd35a283a1f124beada46f448a1

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

@@ -154,10 +154,10 @@ class BlobMetadata {
         }
     }
 
-    void removeInvalidCommitters(SparseArray<String> packages) {
+    void removeCommittersFromUnknownPkgs(SparseArray<String> knownPackages) {
         synchronized (mMetadataLock) {
             mCommitters.removeIf(committer ->
-                    !committer.packageName.equals(packages.get(committer.uid)));
+                    !committer.packageName.equals(knownPackages.get(committer.uid)));
         }
     }
 
@@ -200,16 +200,27 @@ class BlobMetadata {
         }
     }
 
-    void removeInvalidLeasees(SparseArray<String> packages) {
+    void removeLeaseesFromUnknownPkgs(SparseArray<String> knownPackages) {
         synchronized (mMetadataLock) {
             mLeasees.removeIf(leasee ->
-                    !leasee.packageName.equals(packages.get(leasee.uid)));
+                    !leasee.packageName.equals(knownPackages.get(leasee.uid)));
         }
     }
 
-    boolean hasLeases() {
+    void removeExpiredLeases() {
         synchronized (mMetadataLock) {
-            return !mLeasees.isEmpty();
+            mLeasees.removeIf(leasee -> !leasee.isStillValid());
+        }
+    }
+
+    boolean hasValidLeases() {
+        synchronized (mMetadataLock) {
+            for (int i = 0, size = mLeasees.size(); i < size; ++i) {
+                if (mLeasees.valueAt(i).isStillValid()) {
+                    return true;
+                }
+            }
+            return false;
         }
     }
 
@@ -226,8 +237,7 @@ class BlobMetadata {
             // Check if packageName already holds a lease on the blob.
             for (int i = 0, size = mLeasees.size(); i < size; ++i) {
                 final Leasee leasee = mLeasees.valueAt(i);
-                if (leasee.equals(callingPackage, callingUid)
+                if (leasee.isStillValid() && leasee.equals(callingPackage, callingUid)) {
-                        && leasee.isStillValid()) {
                     return true;
                 }
             }
@@ -259,25 +269,32 @@ class BlobMetadata {
 
     boolean isALeasee(@Nullable String packageName, int uid) {
         synchronized (mMetadataLock) {
-            return isAnAccessor(mLeasees, packageName, uid);
+            final Leasee leasee = getAccessor(mLeasees, packageName, uid);
+            return leasee != null && leasee.isStillValid();
         }
     }
 
     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.
+        return getAccessor(accessors, packageName, uid) != null;
+    }
+
+    private static <T extends Accessor> T getAccessor(@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;
+                return (T) accessor;
             } else if (packageName != null && accessor.packageName.equals(packageName)) {
-                return true;
+                return (T) accessor;
             } else if (uid != INVALID_UID && accessor.uid == uid) {
-                return true;
+                return (T) accessor;
             }
         }
-        return false;
+        return null;
     }
 
     boolean isALeasee(@NonNull String packageName) {
@@ -298,11 +315,11 @@ class BlobMetadata {
 
     private boolean hasOtherLeasees(@Nullable String packageName, int uid) {
         synchronized (mMetadataLock) {
-            if (mCommitters.size() > 1 || mLeasees.size() > 1) {
-                return true;
-            }
             for (int i = 0, size = mLeasees.size(); i < size; ++i) {
                 final Leasee leasee = mLeasees.valueAt(i);
+                if (!leasee.isStillValid()) {
+                    continue;
+                }
                 // TODO: Also exclude packages which are signed with same cert?
                 if (packageName != null && uid != INVALID_UID
                         && !leasee.equals(packageName, uid)) {
@@ -322,6 +339,9 @@ class BlobMetadata {
         synchronized (mMetadataLock) {
             for (int i = 0, size = mLeasees.size(); i < size; ++i) {
                 final Leasee leasee = mLeasees.valueAt(i);
+                if (!leasee.isStillValid()) {
+                    continue;
+                }
                 if (leasee.uid == uid && leasee.packageName.equals(packageName)) {
                     final int descriptionResId = leasee.descriptionResEntryName == null
                             ? Resources.ID_NULL
@@ -426,7 +446,7 @@ class BlobMetadata {
 
         // Blobs with no active leases
         if ((!respectLeaseWaitTime || hasLeaseWaitTimeElapsedForAll())
-                && !hasLeases()) {
+                && !hasValidLeases()) {
             return true;
         }
 
@@ -715,7 +735,7 @@ class BlobMetadata {
         }
 
         boolean isStillValid() {
-            return expiryTimeMillis == 0 || expiryTimeMillis <= System.currentTimeMillis();
+            return expiryTimeMillis == 0 || expiryTimeMillis >= System.currentTimeMillis();
         }
 
         void dump(@NonNull Context context, @NonNull IndentingPrintWriter fout) {

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

@@ -539,7 +539,7 @@ public class BlobStoreManagerService extends SystemService {
                 Slog.v(TAG, "Released lease on " + blobHandle
                         + "; callingUid=" + callingUid + ", callingPackage=" + callingPackage);
             }
-            if (!blobMetadata.hasLeases()) {
+            if (!blobMetadata.hasValidLeases()) {
                 mHandler.postDelayed(() -> {
                     synchronized (mBlobsLock) {
                         // Check if blobMetadata object is still valid. If it is not, then
@@ -583,6 +583,9 @@ public class BlobStoreManagerService extends SystemService {
             getUserBlobsLocked(userId).forEach((blobHandle, blobMetadata) -> {
                 final ArrayList<LeaseInfo> leaseInfos = new ArrayList<>();
                 blobMetadata.forEachLeasee(leasee -> {
+                    if (!leasee.isStillValid()) {
+                        return;
+                    }
                     final int descriptionResId = leasee.descriptionResEntryName == null
                             ? Resources.ID_NULL
                             : getDescriptionResourceId(resourcesGetter.apply(leasee.packageName),
@@ -922,8 +925,8 @@ public class BlobStoreManagerService extends SystemService {
                         blobMetadata.getBlobFile().delete();
                     } else {
                         addBlobForUserLocked(blobMetadata, blobMetadata.getUserId());
-                        blobMetadata.removeInvalidCommitters(userPackages);
+                        blobMetadata.removeCommittersFromUnknownPkgs(userPackages);
-                        blobMetadata.removeInvalidLeasees(userPackages);
+                        blobMetadata.removeLeaseesFromUnknownPkgs(userPackages);
                     }
                     mCurrentMaxSessionId = Math.max(mCurrentMaxSessionId, blobMetadata.getBlobId());
                 }
@@ -1110,6 +1113,9 @@ public class BlobStoreManagerService extends SystemService {
             userBlobs.entrySet().removeIf(entry -> {
                 final BlobMetadata blobMetadata = entry.getValue();
 
+                // Remove expired leases
+                blobMetadata.removeExpiredLeases();
+
                 if (blobMetadata.shouldBeDeleted(true /* respectLeaseWaitTime */)) {
                     deleteBlobLocked(blobMetadata);
                     deletedBlobIds.add(blobMetadata.getBlobId());

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

@@ -377,11 +377,11 @@ public class BlobStoreManagerServiceTest {
     }
 
     private BlobMetadata createBlobMetadataMock(long blobId, File blobFile,
-            BlobHandle blobHandle, boolean hasLeases) {
+            BlobHandle blobHandle, boolean hasValidLeases) {
         final BlobMetadata blobMetadata = mock(BlobMetadata.class);
         doReturn(blobId).when(blobMetadata).getBlobId();
         doReturn(blobFile).when(blobMetadata).getBlobFile();
-        doReturn(hasLeases).when(blobMetadata).hasLeases();
+        doReturn(hasValidLeases).when(blobMetadata).hasValidLeases();
         doReturn(blobHandle).when(blobMetadata).getBlobHandle();
         doCallRealMethod().when(blobMetadata).shouldBeDeleted(anyBoolean());
         doReturn(true).when(blobMetadata).hasLeaseWaitTimeElapsedForAll();