From acae3c0f3bb4dbb7fd815f38f34fc6a5f6b27c9b Mon Sep 17 00:00:00 2001 From: JW Wang Date: Mon, 27 Jul 2020 14:52:28 +0800 Subject: [PATCH 1/5] Ensure mutation to multiple objects is done atomically Acquire both parent and child locks before mutation begins. This ensures no concurrent mutation is possible on another thread. Bug: 161954235 Test: atest StagedInstallTest AtomicInstallTest Change-Id: If4b83ef83bbe7e85e91a6b8f16e01d4dc0d6d8e6 --- .../server/pm/PackageInstallerSession.java | 76 ++++++++++++++----- 1 file changed, 59 insertions(+), 17 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 92da005babfa0..703a3ba4ce399 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -157,6 +157,7 @@ import java.util.Arrays; import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; public class PackageInstallerSession extends IPackageInstallerSession.Stub { @@ -262,6 +263,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private final Object mLock = new Object(); + /** + * Used to detect and reject concurrent access to this session object to ensure mutation + * to multiple objects like {@link #addChildSessionId} are done atomically. + */ + private final AtomicBoolean mTransactionLock = new AtomicBoolean(false); + /** Timestamp of the last time this session changed state */ @GuardedBy("mLock") private long updatedMillis; @@ -3073,39 +3080,74 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } + private void acquireTransactionLock() { + if (!mTransactionLock.compareAndSet(false, true)) { + throw new UnsupportedOperationException("Concurrent access not supported"); + } + } + + private void releaseTransactionLock() { + mTransactionLock.compareAndSet(true, false); + } + @Override public void addChildSessionId(int childSessionId) { final PackageInstallerSession childSession = mSessionProvider.getSession(childSessionId); - if (childSession == null || !childSession.canBeAddedAsChild(sessionId)) { + if (childSession == null) { throw new IllegalStateException("Unable to add child session " + childSessionId - + " as it does not exist or is in an invalid state."); + + " as it does not exist."); } - synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); - assertPreparedAndNotSealedLocked("addChildSessionId"); - final int indexOfSession = mChildSessionIds.indexOfKey(childSessionId); - if (indexOfSession >= 0) { - return; + try { + acquireTransactionLock(); + childSession.acquireTransactionLock(); + + if (!childSession.canBeAddedAsChild(sessionId)) { + throw new IllegalStateException("Unable to add child session " + childSessionId + + " as it is in an invalid state."); } - childSession.setParentSessionId(this.sessionId); - addChildSessionIdLocked(childSessionId); + synchronized (mLock) { + assertCallerIsOwnerOrRootLocked(); + assertPreparedAndNotSealedLocked("addChildSessionId"); + + final int indexOfSession = mChildSessionIds.indexOfKey(childSessionId); + if (indexOfSession >= 0) { + return; + } + childSession.setParentSessionId(this.sessionId); + addChildSessionIdLocked(childSessionId); + } + } finally { + releaseTransactionLock(); + childSession.releaseTransactionLock(); } } @Override public void removeChildSessionId(int sessionId) { final PackageInstallerSession session = mSessionProvider.getSession(sessionId); - synchronized (mLock) { - final int indexOfSession = mChildSessionIds.indexOfKey(sessionId); + try { + acquireTransactionLock(); if (session != null) { - session.setParentSessionId(SessionInfo.INVALID_ID); + session.acquireTransactionLock(); } - if (indexOfSession < 0) { - // not added in the first place; no-op - return; + + synchronized (mLock) { + final int indexOfSession = mChildSessionIds.indexOfKey(sessionId); + if (session != null) { + session.setParentSessionId(SessionInfo.INVALID_ID); + } + if (indexOfSession < 0) { + // not added in the first place; no-op + return; + } + mChildSessionIds.removeAt(indexOfSession); + } + } finally { + releaseTransactionLock(); + if (session != null) { + session.releaseTransactionLock(); } - mChildSessionIds.removeAt(indexOfSession); } } From d16e9d3b5da739feb0cadb71b2470556135d1fae Mon Sep 17 00:00:00 2001 From: JW Wang Date: Mon, 27 Jul 2020 16:11:40 +0800 Subject: [PATCH 2/5] Fix #addChildSessionId (1/n) A single-session can't have child. Bug: 162286562 Test: Will be added in next CLs Change-Id: I17037a6a65599a74cffd12fffc741b924143dc1a --- .../java/com/android/server/pm/PackageInstallerSession.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 703a3ba4ce399..2a10a1959c433 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -3092,6 +3092,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @Override public void addChildSessionId(int childSessionId) { + if (!params.isMultiPackage) { + throw new IllegalStateException("Single-session " + sessionId + " can't have child."); + } + final PackageInstallerSession childSession = mSessionProvider.getSession(childSessionId); if (childSession == null) { throw new IllegalStateException("Unable to add child session " + childSessionId From 35da764125727eb1c14651b35b7dc15f1e199c8a Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 28 Jul 2020 12:49:30 +0800 Subject: [PATCH 3/5] Fix #addChildSessionId (2/n) A multi-session can't be a child. Bug: 162286562 Test: Will be added in next CLs Change-Id: I7e9957f0cd57fe924cfb5ba8f0cfb27b045f26dc --- .../java/com/android/server/pm/PackageInstallerSession.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 2a10a1959c433..ff96c9173d594 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -3101,6 +3101,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new IllegalStateException("Unable to add child session " + childSessionId + " as it does not exist."); } + if (childSession.params.isMultiPackage) { + throw new IllegalStateException("Multi-session " + childSessionId + + " can't be a child."); + } try { acquireTransactionLock(); From 40fc1cde8ecbe6fc51b25d33e4ef1f8e624f3def Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 28 Jul 2020 12:50:52 +0800 Subject: [PATCH 4/5] Fix #removeChildSessionId (3/n) Can't remove child from a sealed session. Bug: 162286562 Test: Will be added in next CLs Change-Id: I5fe20ede028f0e00e739316dab5c6a24b1546718 --- .../java/com/android/server/pm/PackageInstallerSession.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index ff96c9173d594..c56f744722565 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -3141,6 +3141,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } synchronized (mLock) { + assertCallerIsOwnerOrRootLocked(); + assertPreparedAndNotSealedLocked("removeChildSessionId"); final int indexOfSession = mChildSessionIds.indexOfKey(sessionId); if (session != null) { session.setParentSessionId(SessionInfo.INVALID_ID); From b1d2b9dc94b195c73755db8892ad2eb3f7614868 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 28 Jul 2020 12:53:29 +0800 Subject: [PATCH 5/5] Fix #removeChildSessionId (4/n) Removing a non-owning child should be no-op. The current implementation will set the parent id to -1 incorrectly. Bug: 162286562 Test: Will be added in next CLs Change-Id: I483c570079511d5f10dcfdc93e7daf006004b226 --- .../com/android/server/pm/PackageInstallerSession.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index c56f744722565..db8ecf764a3fe 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -3143,14 +3143,15 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotSealedLocked("removeChildSessionId"); + final int indexOfSession = mChildSessionIds.indexOfKey(sessionId); - if (session != null) { - session.setParentSessionId(SessionInfo.INVALID_ID); - } if (indexOfSession < 0) { // not added in the first place; no-op return; } + if (session != null) { + session.setParentSessionId(SessionInfo.INVALID_ID); + } mChildSessionIds.removeAt(indexOfSession); } } finally {