From 9ed7249ffef64861620e84fa84feb99529279faf Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Wed, 24 Jun 2020 13:33:18 -0700 Subject: [PATCH] Revoke any open fds when deleting a session/blob. Fixes: 159832638 Test: atest --test-mapping apex/blobstore Change-Id: I97d97ec2874ace574d6d64c4793a6b374949725e --- .../com/android/server/blob/BlobMetadata.java | 20 ++++++++++++ .../server/blob/BlobStoreManagerService.java | 32 +++++++++++-------- .../android/server/blob/BlobStoreSession.java | 5 +++ .../blob/BlobStoreManagerServiceTest.java | 26 +++++++-------- 4 files changed, 56 insertions(+), 27 deletions(-) diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java b/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java index 4d29045fa6318..3d4154a227be4 100644 --- a/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java +++ b/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 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()) { diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobStoreManagerService.java b/apex/blobstore/service/java/com/android/server/blob/BlobStoreManagerService.java index 7a6c884848d8f..f7468d8faa62c 100644 --- a/apex/blobstore/service/java/com/android/server/blob/BlobStoreManagerService.java +++ b/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(); - mActiveBlobIds.remove(session.getSessionId()); + deleteSessionLocked(session); 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(); - mActiveBlobIds.remove(session.getSessionId()); + deleteSessionLocked(session); } 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(); - mActiveBlobIds.remove(blobStoreSession.getSessionId()); + deleteSessionLocked(blobStoreSession); 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(); - mActiveBlobIds.remove(session.getSessionId()); + deleteSessionLocked(session); } } @@ -1138,8 +1137,7 @@ public class BlobStoreManagerService extends SystemService { } if (shouldRemove) { - blobStoreSession.getSessionFile().delete(); - mActiveBlobIds.remove(blobStoreSession.getSessionId()); + deleteSessionLocked(blobStoreSession); 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()); } diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java b/apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java index 53e296ba3d119..2f83be1e0370a 100644 --- a/apex/blobstore/service/java/com/android/server/blob/BlobStoreSession.java +++ b/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) { diff --git a/services/tests/mockingservicestests/src/com/android/server/blob/BlobStoreManagerServiceTest.java b/services/tests/mockingservicestests/src/com/android/server/blob/BlobStoreManagerServiceTest.java index 7446289cd498a..4e2f9a495fe8d 100644 --- a/services/tests/mockingservicestests/src/com/android/server/blob/BlobStoreManagerServiceTest.java +++ b/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(sessionFile2, never()).delete(); - verify(sessionFile3, never()).delete(); - verify(sessionFile4).delete(); + verify(session1).destroy(); + verify(session2, never()).destroy(); + verify(session3, never()).destroy(); + 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(blobFile2).delete(); - verify(blobFile3).delete(); + verify(blobMetadata1, never()).destroy(); + verify(blobMetadata2).destroy(); + 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(sessionFile2, never()).delete(); - verify(sessionFile3).delete(); + verify(session1).destroy(); + verify(session2, never()).destroy(); + 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(blobFile2, never()).delete(); - verify(blobFile3).delete(); + verify(blobMetadata1).destroy(); + verify(blobMetadata2, never()).destroy(); + verify(blobMetadata3).destroy(); assertThat(mUserBlobs.size()).isEqualTo(1); assertThat(mUserBlobs.get(blobHandle2)).isNotNull();