Merge "Revoke any open fds when deleting a session/blob." into rvc-dev am: d06ebaeeed

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

Change-Id: I8ce2545ef44519388a3f0667a3249484977631c8

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

@@ -398,6 +398,26 @@ class BlobMetadata {
         return revocableFd.getRevocableFileDescriptor();
     }
 
+    void destroy() {
+        revokeAllFds();
+        getBlobFile().delete();
+    }
+
+    private void revokeAllFds() {
+        synchronized (mRevocableFds) {
+            for (int i = 0, pkgCount = mRevocableFds.size(); i < pkgCount; ++i) {
+                final ArraySet<RevocableFileDescriptor> packageFds =
+                        mRevocableFds.valueAt(i);
+                if (packageFds == null) {
+                    continue;
+                }
+                for (int j = 0, fdCount = packageFds.size(); j < fdCount; ++j) {
+                    packageFds.valueAt(j).revoke();
+                }
+            }
+        }
+    }
+
     boolean shouldBeDeleted(boolean respectLeaseWaitTime) {
         // Expired data blobs
         if (getBlobHandle().isExpired()) {

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

@@ -606,7 +606,11 @@ public class BlobStoreManagerService extends SystemService {
                     UserHandle.getUserId(callingUid));
             userBlobs.entrySet().removeIf(entry -> {
                 final BlobMetadata blobMetadata = entry.getValue();
-                return blobMetadata.getBlobId() == blobId;
+                if (blobMetadata.getBlobId() == blobId) {
+                    deleteBlobLocked(blobMetadata);
+                    return true;
+                }
+                return false;
             });
             writeBlobsInfoAsync();
         }
@@ -657,11 +661,10 @@ public class BlobStoreManagerService extends SystemService {
         switch (session.getState()) {
             case STATE_ABANDONED:
             case STATE_VERIFIED_INVALID:
-                session.getSessionFile().delete();
                 synchronized (mBlobsLock) {
+                    deleteSessionLocked(session);
                     getUserSessionsLocked(UserHandle.getUserId(session.getOwnerUid()))
                             .remove(session.getSessionId());
-                    mActiveBlobIds.remove(session.getSessionId());
                     if (LOGV) {
                         Slog.v(TAG, "Session is invalid; deleted " + session);
                     }
@@ -682,8 +685,7 @@ public class BlobStoreManagerService extends SystemService {
                         Slog.d(TAG, "Failed to commit: too many committed blobs. count: "
                                 + committedBlobsCount + "; blob: " + session);
                         session.sendCommitCallbackResult(COMMIT_RESULT_ERROR);
-                        session.getSessionFile().delete();
+                        deleteSessionLocked(session);
-                        mActiveBlobIds.remove(session.getSessionId());
                         getUserSessionsLocked(UserHandle.getUserId(session.getOwnerUid()))
                                 .remove(session.getSessionId());
                         break;
@@ -732,8 +734,7 @@ public class BlobStoreManagerService extends SystemService {
                     }
                     // Delete redundant data from recommits.
                     if (session.getSessionId() != blob.getBlobId()) {
-                        session.getSessionFile().delete();
+                        deleteSessionLocked(session);
-                        mActiveBlobIds.remove(session.getSessionId());
                     }
                     getUserSessionsLocked(UserHandle.getUserId(session.getOwnerUid()))
                             .remove(session.getSessionId());
@@ -1019,8 +1020,7 @@ public class BlobStoreManagerService extends SystemService {
             userSessions.removeIf((sessionId, blobStoreSession) -> {
                 if (blobStoreSession.getOwnerUid() == uid
                         && blobStoreSession.getOwnerPackageName().equals(packageName)) {
-                    blobStoreSession.getSessionFile().delete();
+                    deleteSessionLocked(blobStoreSession);
-                    mActiveBlobIds.remove(blobStoreSession.getSessionId());
                     return true;
                 }
                 return false;
@@ -1061,8 +1061,7 @@ public class BlobStoreManagerService extends SystemService {
             if (userSessions != null) {
                 for (int i = 0, count = userSessions.size(); i < count; ++i) {
                     final BlobStoreSession session = userSessions.valueAt(i);
-                    session.getSessionFile().delete();
+                    deleteSessionLocked(session);
-                    mActiveBlobIds.remove(session.getSessionId());
                 }
             }
 
@@ -1138,8 +1137,7 @@ public class BlobStoreManagerService extends SystemService {
                 }
 
                 if (shouldRemove) {
-                    blobStoreSession.getSessionFile().delete();
+                    deleteSessionLocked(blobStoreSession);
-                    mActiveBlobIds.remove(blobStoreSession.getSessionId());
                     deletedBlobIds.add(blobStoreSession.getSessionId());
                 }
                 return shouldRemove;
@@ -1150,9 +1148,15 @@ public class BlobStoreManagerService extends SystemService {
         writeBlobSessionsAsync();
     }
 
+    @GuardedBy("mBlobsLock")
+    private void deleteSessionLocked(BlobStoreSession blobStoreSession) {
+        blobStoreSession.destroy();
+        mActiveBlobIds.remove(blobStoreSession.getSessionId());
+    }
+
     @GuardedBy("mBlobsLock")
     private void deleteBlobLocked(BlobMetadata blobMetadata) {
-        blobMetadata.getBlobFile().delete();
+        blobMetadata.destroy();
         mActiveBlobIds.remove(blobMetadata.getBlobId());
     }
 

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

@@ -479,6 +479,11 @@ class BlobStoreSession extends IBlobStoreSession.Stub {
         }
     }
 
+    void destroy() {
+        revokeAllFds();
+        getSessionFile().delete();
+    }
+
     private void revokeAllFds() {
         synchronized (mRevocableFds) {
             for (int i = mRevocableFds.size() - 1; i >= 0; --i) {

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

@@ -174,10 +174,10 @@ public class BlobStoreManagerServiceTest {
         mService.handlePackageRemoved(TEST_PKG1, TEST_UID1);
 
         // Verify sessions are removed
-        verify(sessionFile1).delete();
+        verify(session1).destroy();
-        verify(sessionFile2, never()).delete();
+        verify(session2, never()).destroy();
-        verify(sessionFile3, never()).delete();
+        verify(session3, never()).destroy();
-        verify(sessionFile4).delete();
+        verify(session4).destroy();
 
         assertThat(mUserSessions.size()).isEqualTo(2);
         assertThat(mUserSessions.get(sessionId1)).isNull();
@@ -193,9 +193,9 @@ public class BlobStoreManagerServiceTest {
         verify(blobMetadata3).removeCommitter(TEST_PKG1, TEST_UID1);
         verify(blobMetadata3).removeLeasee(TEST_PKG1, TEST_UID1);
 
-        verify(blobFile1, never()).delete();
+        verify(blobMetadata1, never()).destroy();
-        verify(blobFile2).delete();
+        verify(blobMetadata2).destroy();
-        verify(blobFile3).delete();
+        verify(blobMetadata3).destroy();
 
         assertThat(mUserBlobs.size()).isEqualTo(1);
         assertThat(mUserBlobs.get(blobHandle1)).isNotNull();
@@ -272,9 +272,9 @@ public class BlobStoreManagerServiceTest {
         mService.handleIdleMaintenanceLocked();
 
         // Verify stale sessions are removed
-        verify(sessionFile1).delete();
+        verify(session1).destroy();
-        verify(sessionFile2, never()).delete();
+        verify(session2, never()).destroy();
-        verify(sessionFile3).delete();
+        verify(session3).destroy();
 
         assertThat(mUserSessions.size()).isEqualTo(1);
         assertThat(mUserSessions.get(sessionId2)).isNotNull();
@@ -317,9 +317,9 @@ public class BlobStoreManagerServiceTest {
         mService.handleIdleMaintenanceLocked();
 
         // Verify stale blobs are removed
-        verify(blobFile1).delete();
+        verify(blobMetadata1).destroy();
-        verify(blobFile2, never()).delete();
+        verify(blobMetadata2, never()).destroy();
-        verify(blobFile3).delete();
+        verify(blobMetadata3).destroy();
 
         assertThat(mUserBlobs.size()).isEqualTo(1);
         assertThat(mUserBlobs.get(blobHandle2)).isNotNull();