From af3f887deb5314af031f7881f79b2680bb3caeae Mon Sep 17 00:00:00 2001 From: Sudheer Shanka Date: Tue, 24 Mar 2020 11:26:23 -0700 Subject: [PATCH] Keep track of commit times for each blob. Also, add a commit cool-off period such that any new commits of the same data blob from the same committer will be treated as if they occurred at the earlier commit time. This is to ensure apps don't keep on committing the same blob and prevent deletion of it. Bug: 151378266 Test: atest --test-mapping apex/blobstore Change-Id: I44b3fddbc465b7d6f00f40b299b91d3d84740989 --- .../java/android/app/blob/XmlTags.java | 1 + .../com/android/server/blob/BlobMetadata.java | 52 +++++++++++++++---- .../android/server/blob/BlobStoreConfig.java | 36 ++++++++++++- .../server/blob/BlobStoreManagerService.java | 14 +++-- .../android/server/blob/BlobStoreUtils.java | 6 +++ .../blob/BlobStoreManagerServiceTest.java | 1 + 6 files changed, 94 insertions(+), 16 deletions(-) diff --git a/apex/blobstore/framework/java/android/app/blob/XmlTags.java b/apex/blobstore/framework/java/android/app/blob/XmlTags.java index e64edc393769c..dbfdcba05a734 100644 --- a/apex/blobstore/framework/java/android/app/blob/XmlTags.java +++ b/apex/blobstore/framework/java/android/app/blob/XmlTags.java @@ -48,6 +48,7 @@ public final class XmlTags { // For committer public static final String TAG_COMMITTER = "c"; + public static final String ATTR_COMMIT_TIME_MS = "cmt"; // For leasee public static final String TAG_LEASEE = "l"; 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 c8ca44b6ef74f..49b3ec1d113b6 100644 --- a/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java +++ b/apex/blobstore/service/java/com/android/server/blob/BlobMetadata.java @@ -15,6 +15,7 @@ */ package com.android.server.blob; +import static android.app.blob.XmlTags.ATTR_COMMIT_TIME_MS; import static android.app.blob.XmlTags.ATTR_DESCRIPTION; import static android.app.blob.XmlTags.ATTR_DESCRIPTION_RES_NAME; import static android.app.blob.XmlTags.ATTR_EXPIRY_TIME; @@ -30,6 +31,7 @@ import static android.os.Process.INVALID_UID; 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_COMMIT_TIME; 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; @@ -54,6 +56,7 @@ import android.util.Slog; import android.util.SparseArray; import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.util.IndentingPrintWriter; import com.android.internal.util.XmlUtils; import com.android.server.blob.BlobStoreManagerService.DumpArgs; @@ -125,7 +128,7 @@ class BlobMetadata { } } - void addCommitters(ArraySet committers) { + void setCommitters(ArraySet committers) { synchronized (mMetadataLock) { mCommitters.clear(); mCommitters.addAll(committers); @@ -153,11 +156,16 @@ class BlobMetadata { } @Nullable - Committer getExistingCommitter(@NonNull Committer newCommitter) { + Committer getExistingCommitter(@NonNull String packageName, int uid) { synchronized (mCommitters) { - final int index = mCommitters.indexOf(newCommitter); - return index >= 0 ? mCommitters.valueAt(index) : null; + for (int i = 0, size = mCommitters.size(); i < size; ++i) { + final Committer committer = mCommitters.valueAt(i); + if (committer.uid == uid && committer.packageName.equals(packageName)) { + return committer; + } + } } + return null; } void addOrReplaceLeasee(String callingPackage, int callingUid, int descriptionResId, @@ -172,7 +180,7 @@ class BlobMetadata { } } - void addLeasees(ArraySet leasees) { + void setLeasees(ArraySet leasees) { synchronized (mMetadataLock) { mLeasees.clear(); mLeasees.addAll(leasees); @@ -380,8 +388,7 @@ class BlobMetadata { } // Blobs with no active leases - // TODO: Track commit time instead of using last modified time. - if ((!respectLeaseWaitTime || hasLeaseWaitTimeElapsed(getBlobFile().lastModified())) + if ((!respectLeaseWaitTime || hasLeaseWaitTimeElapsedForAll()) && !hasLeases()) { return true; } @@ -389,6 +396,17 @@ class BlobMetadata { return false; } + @VisibleForTesting + boolean hasLeaseWaitTimeElapsedForAll() { + for (int i = 0, size = mCommitters.size(); i < size; ++i) { + final Committer committer = mCommitters.valueAt(i); + if (!hasLeaseWaitTimeElapsed(committer.getCommitTimeMs())) { + return false; + } + } + return true; + } + void dump(IndentingPrintWriter fout, DumpArgs dumpArgs) { fout.println("blobHandle:"); fout.increaseIndent(); @@ -492,20 +510,28 @@ class BlobMetadata { } final BlobMetadata blobMetadata = new BlobMetadata(context, blobId, blobHandle, userId); - blobMetadata.addCommitters(committers); - blobMetadata.addLeasees(leasees); + blobMetadata.setCommitters(committers); + blobMetadata.setLeasees(leasees); return blobMetadata; } static final class Committer extends Accessor { public final BlobAccessMode blobAccessMode; + public final long commitTimeMs; - Committer(String packageName, int uid, BlobAccessMode blobAccessMode) { + Committer(String packageName, int uid, BlobAccessMode blobAccessMode, long commitTimeMs) { super(packageName, uid); this.blobAccessMode = blobAccessMode; + this.commitTimeMs = commitTimeMs; + } + + long getCommitTimeMs() { + return commitTimeMs; } void dump(IndentingPrintWriter fout) { + fout.println("commit time: " + + (commitTimeMs == 0 ? "" : BlobStoreUtils.formatTime(commitTimeMs))); fout.println("accessMode:"); fout.increaseIndent(); blobAccessMode.dump(fout); @@ -515,6 +541,7 @@ class BlobMetadata { void writeToXml(@NonNull XmlSerializer out) throws IOException { XmlUtils.writeStringAttribute(out, ATTR_PACKAGE, packageName); XmlUtils.writeIntAttribute(out, ATTR_UID, uid); + XmlUtils.writeLongAttribute(out, ATTR_COMMIT_TIME_MS, commitTimeMs); out.startTag(null, TAG_ACCESS_MODE); blobAccessMode.writeToXml(out); @@ -526,6 +553,9 @@ class BlobMetadata { throws XmlPullParserException, IOException { final String packageName = XmlUtils.readStringAttribute(in, ATTR_PACKAGE); final int uid = XmlUtils.readIntAttribute(in, ATTR_UID); + final long commitTimeMs = version >= XML_VERSION_ADD_COMMIT_TIME + ? XmlUtils.readLongAttribute(in, ATTR_COMMIT_TIME_MS) + : 0; final int depth = in.getDepth(); BlobAccessMode blobAccessMode = null; @@ -538,7 +568,7 @@ class BlobMetadata { Slog.wtf(TAG, "blobAccessMode should be available"); return null; } - return new Committer(packageName, uid, blobAccessMode); + return new Committer(packageName, uid, blobAccessMode, commitTimeMs); } } diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobStoreConfig.java b/apex/blobstore/service/java/com/android/server/blob/BlobStoreConfig.java index f2c158658562d..6af1178b55f1a 100644 --- a/apex/blobstore/service/java/com/android/server/blob/BlobStoreConfig.java +++ b/apex/blobstore/service/java/com/android/server/blob/BlobStoreConfig.java @@ -45,8 +45,9 @@ class BlobStoreConfig { // Added a string variant of lease description. public static final int XML_VERSION_ADD_STRING_DESC = 2; public static final int XML_VERSION_ADD_DESC_RES_NAME = 3; + public static final int XML_VERSION_ADD_COMMIT_TIME = 4; - public static final int XML_VERSION_CURRENT = XML_VERSION_ADD_DESC_RES_NAME; + public static final int XML_VERSION_CURRENT = XML_VERSION_ADD_COMMIT_TIME; private static final String ROOT_DIR_NAME = "blobstore"; private static final String BLOBS_DIR_NAME = "blobs"; @@ -100,6 +101,18 @@ class BlobStoreConfig { public static long LEASE_ACQUISITION_WAIT_DURATION_MS = DEFAULT_LEASE_ACQUISITION_WAIT_DURATION_MS; + /** + * Denotes the duration from the time a blob is committed that any new commits of the same + * data blob from the same committer will be treated as if they occurred at the earlier + * commit time. + */ + public static final String KEY_COMMIT_COOL_OFF_DURATION_MS = + "commit_cool_off_duration_ms"; + public static final long DEFAULT_COMMIT_COOL_OFF_DURATION_MS = + TimeUnit.HOURS.toMillis(48); + public static long COMMIT_COOL_OFF_DURATION_MS = + DEFAULT_COMMIT_COOL_OFF_DURATION_MS; + static void refresh(Properties properties) { if (!NAMESPACE_BLOBSTORE.equals(properties.getNamespace())) { return; @@ -163,6 +176,27 @@ class BlobStoreConfig { < System.currentTimeMillis(); } + /** + * Returns an adjusted commit time depending on whether commit cool-off period has elapsed. + * + * If this is the initial commit or the earlier commit cool-off period has elapsed, then + * the new commit time is used. Otherwise, the earlier commit time is used. + */ + public static long getAdjustedCommitTimeMs(long oldCommitTimeMs, long newCommitTimeMs) { + if (oldCommitTimeMs == 0 || hasCommitCoolOffPeriodElapsed(oldCommitTimeMs)) { + return newCommitTimeMs; + } + return oldCommitTimeMs; + } + + /** + * Returns whether the commit cool-off period has elapsed. + */ + private static boolean hasCommitCoolOffPeriodElapsed(long commitTimeMs) { + return commitTimeMs + DeviceConfigProperties.COMMIT_COOL_OFF_DURATION_MS + < System.currentTimeMillis(); + } + @Nullable public static File prepareBlobFile(long sessionId) { final File blobsDir = prepareBlobsDir(); 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 e472d052f32f8..35a2436702dac 100644 --- a/apex/blobstore/service/java/com/android/server/blob/BlobStoreManagerService.java +++ b/apex/blobstore/service/java/com/android/server/blob/BlobStoreManagerService.java @@ -31,6 +31,7 @@ import static com.android.server.blob.BlobStoreConfig.LOGV; import static com.android.server.blob.BlobStoreConfig.SESSION_EXPIRY_TIMEOUT_MILLIS; import static com.android.server.blob.BlobStoreConfig.TAG; import static com.android.server.blob.BlobStoreConfig.XML_VERSION_CURRENT; +import static com.android.server.blob.BlobStoreConfig.getAdjustedCommitTimeMs; import static com.android.server.blob.BlobStoreSession.STATE_ABANDONED; import static com.android.server.blob.BlobStoreSession.STATE_COMMITTED; import static com.android.server.blob.BlobStoreSession.STATE_VERIFIED_INVALID; @@ -566,13 +567,18 @@ public class BlobStoreManagerService extends SystemService { userId); BlobMetadata blob = userBlobs.get(session.getBlobHandle()); if (blob == null) { - blob = new BlobMetadata(mContext, - session.getSessionId(), session.getBlobHandle(), userId); + blob = new BlobMetadata(mContext, session.getSessionId(), + session.getBlobHandle(), userId); addBlobForUserLocked(blob, userBlobs); } + final Committer existingCommitter = blob.getExistingCommitter( + session.getOwnerPackageName(), session.getOwnerUid()); + final long existingCommitTimeMs = + (existingCommitter == null) ? 0 : existingCommitter.getCommitTimeMs(); final Committer newCommitter = new Committer(session.getOwnerPackageName(), - session.getOwnerUid(), session.getBlobAccessMode()); - final Committer existingCommitter = blob.getExistingCommitter(newCommitter); + session.getOwnerUid(), session.getBlobAccessMode(), + getAdjustedCommitTimeMs(existingCommitTimeMs, + System.currentTimeMillis())); blob.addOrReplaceCommitter(newCommitter); try { writeBlobsInfoLocked(); diff --git a/apex/blobstore/service/java/com/android/server/blob/BlobStoreUtils.java b/apex/blobstore/service/java/com/android/server/blob/BlobStoreUtils.java index fabce766c237d..1d07e88773c3b 100644 --- a/apex/blobstore/service/java/com/android/server/blob/BlobStoreUtils.java +++ b/apex/blobstore/service/java/com/android/server/blob/BlobStoreUtils.java @@ -24,6 +24,7 @@ import android.annotation.Nullable; import android.content.Context; import android.content.pm.PackageManager; import android.content.res.Resources; +import android.text.format.TimeMigrationUtils; import android.util.Slog; class BlobStoreUtils { @@ -56,4 +57,9 @@ class BlobStoreUtils { ? Resources.ID_NULL : getDescriptionResourceId(resources, resourceEntryName, packageName); } + + @NonNull + static String formatTime(long timeMs) { + return TimeMigrationUtils.formatMillisWithFixedFormat(timeMs); + } } 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 f4d7b8bd5b1aa..d338b587e059d 100644 --- a/services/tests/mockingservicestests/src/com/android/server/blob/BlobStoreManagerServiceTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/blob/BlobStoreManagerServiceTest.java @@ -382,6 +382,7 @@ public class BlobStoreManagerServiceTest { doReturn(hasLeases).when(blobMetadata).hasLeases(); doReturn(blobHandle).when(blobMetadata).getBlobHandle(); doCallRealMethod().when(blobMetadata).shouldBeDeleted(anyBoolean()); + doReturn(true).when(blobMetadata).hasLeaseWaitTimeElapsedForAll(); return blobMetadata; }