From 69eaa90b0e4cc78fa2f518a50182bc9e4c9e87f3 Mon Sep 17 00:00:00 2001 From: Varun Shah Date: Wed, 18 Nov 2020 19:59:44 +0000 Subject: [PATCH 1/5] Ensure caller identity is restored in CP quick-path. Bug: 172935267 Test: PoC in bug Change-Id: Ib11a5d33be75e208bccda86fef55758d357ccebd Merged-In: I69d5c792cf442e694a5b5ab15ce5913279a5545a (cherry picked from commit 2f28b20e036ccdc9a5ef6680c394d4127075a259) --- .../server/am/ActivityManagerService.java | 113 +++++++++--------- 1 file changed, 57 insertions(+), 56 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 091c77ecaac3e..7acb1bf47e784 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -7142,67 +7142,68 @@ public class ActivityManagerService extends IActivityManager.Stub "getContentProviderImpl: after checkContentProviderPermission"); final long origId = Binder.clearCallingIdentity(); + try { + checkTime(startTime, "getContentProviderImpl: incProviderCountLocked"); - checkTime(startTime, "getContentProviderImpl: incProviderCountLocked"); - - // In this case the provider instance already exists, so we can - // return it right away. - conn = incProviderCountLocked(r, cpr, token, callingUid, callingPackage, callingTag, - stable); - if (conn != null && (conn.stableCount+conn.unstableCount) == 1) { - if (cpr.proc != null - && r != null && r.setAdj <= ProcessList.PERCEPTIBLE_LOW_APP_ADJ) { - // If this is a perceptible app accessing the provider, - // make sure to count it as being accessed and thus - // back up on the LRU list. This is good because - // content providers are often expensive to start. - checkTime(startTime, "getContentProviderImpl: before updateLruProcess"); - mProcessList.updateLruProcessLocked(cpr.proc, false, null); - checkTime(startTime, "getContentProviderImpl: after updateLruProcess"); + // Return the provider instance right away since it already exists. + conn = incProviderCountLocked(r, cpr, token, callingUid, callingPackage, + callingTag, stable); + if (conn != null && (conn.stableCount+conn.unstableCount) == 1) { + if (cpr.proc != null + && r != null && r.setAdj <= ProcessList.PERCEPTIBLE_LOW_APP_ADJ) { + // If this is a perceptible app accessing the provider, + // make sure to count it as being accessed and thus + // back up on the LRU list. This is good because + // content providers are often expensive to start. + checkTime(startTime, "getContentProviderImpl: before updateLruProcess"); + mProcessList.updateLruProcessLocked(cpr.proc, false, null); + checkTime(startTime, "getContentProviderImpl: after updateLruProcess"); + } } - } - checkTime(startTime, "getContentProviderImpl: before updateOomAdj"); - final int verifiedAdj = cpr.proc.verifiedAdj; - boolean success = updateOomAdjLocked(cpr.proc, true, - OomAdjuster.OOM_ADJ_REASON_GET_PROVIDER); - // XXX things have changed so updateOomAdjLocked doesn't actually tell us - // if the process has been successfully adjusted. So to reduce races with - // it, we will check whether the process still exists. Note that this doesn't - // completely get rid of races with LMK killing the process, but should make - // them much smaller. - if (success && verifiedAdj != cpr.proc.setAdj && !isProcessAliveLocked(cpr.proc)) { - success = false; - } - maybeUpdateProviderUsageStatsLocked(r, cpr.info.packageName, name); - checkTime(startTime, "getContentProviderImpl: after updateOomAdj"); - if (DEBUG_PROVIDER) Slog.i(TAG_PROVIDER, "Adjust success: " + success); - // NOTE: there is still a race here where a signal could be - // pending on the process even though we managed to update its - // adj level. Not sure what to do about this, but at least - // the race is now smaller. - if (!success) { - // Uh oh... it looks like the provider's process - // has been killed on us. We need to wait for a new - // process to be started, and make sure its death - // doesn't kill our process. - Slog.wtf(TAG, "Existing provider " + cpr.name.flattenToShortString() - + " is crashing; detaching " + r); - boolean lastRef = decProviderCountLocked(conn, cpr, token, stable); - if (!lastRef) { - // This wasn't the last ref our process had on - // the provider... we will be killed during cleaning up, bail. - return null; + checkTime(startTime, "getContentProviderImpl: before updateOomAdj"); + final int verifiedAdj = cpr.proc.verifiedAdj; + boolean success = updateOomAdjLocked(cpr.proc, true, + OomAdjuster.OOM_ADJ_REASON_GET_PROVIDER); + // XXX things have changed so updateOomAdjLocked doesn't actually tell us + // if the process has been successfully adjusted. So to reduce races with + // it, we will check whether the process still exists. Note that this doesn't + // completely get rid of races with LMK killing the process, but should make + // them much smaller. + if (success && verifiedAdj != cpr.proc.setAdj + && !isProcessAliveLocked(cpr.proc)) { + success = false; } - // We'll just start a new process to host the content provider - providerRunning = false; - conn = null; - dyingProc = cpr.proc; - } else { - cpr.proc.verifiedAdj = cpr.proc.setAdj; + maybeUpdateProviderUsageStatsLocked(r, cpr.info.packageName, name); + checkTime(startTime, "getContentProviderImpl: after updateOomAdj"); + if (DEBUG_PROVIDER) Slog.i(TAG_PROVIDER, "Adjust success: " + success); + // NOTE: there is still a race here where a signal could be + // pending on the process even though we managed to update its + // adj level. Not sure what to do about this, but at least + // the race is now smaller. + if (!success) { + // Uh oh... it looks like the provider's process + // has been killed on us. We need to wait for a new + // process to be started, and make sure its death + // doesn't kill our process. + Slog.wtf(TAG, "Existing provider " + cpr.name.flattenToShortString() + + " is crashing; detaching " + r); + boolean lastRef = decProviderCountLocked(conn, cpr, token, stable); + if (!lastRef) { + // This wasn't the last ref our process had on + // the provider... we will be killed during cleaning up, bail. + return null; + } + // We'll just start a new process to host the content provider + providerRunning = false; + conn = null; + dyingProc = cpr.proc; + } else { + cpr.proc.verifiedAdj = cpr.proc.setAdj; + } + } finally { + Binder.restoreCallingIdentity(origId); } - - Binder.restoreCallingIdentity(origId); } if (!providerRunning) { From 251dfbee7fb5986c7e93362556ea556785e4d0e4 Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 28 Oct 2020 13:25:44 -0700 Subject: [PATCH 2/5] Remove updateIntentVerificationStatusAsUser from ResolverActivity DO NOT CHERRY PICK ANYWHERE: Security issue This API is meant to grant an app complete verification over the domains it has declared, meaning it will always resolve the domains it declares for web links. This can allow an app to take over links that are unowned. Any time a user selects "Always" when resolving an Intent in the diambiguation dialog, this API would be called, and all subsequent resolutions of any domain declared by the app selected would be automatically directed to that app, with no prompt to the user. From a quick search, it's possible that all usages of this API are actually unintended and should be removed. Should be considered for deprecation in the future. Bug: 163358811 Test: none, this is not generally testable, see linked bug for context Merged-In: Iff7f788a83af68c7fbb1c6b9a8be7b47136be2b6 Change-Id: Iff7f788a83af68c7fbb1c6b9a8be7b47136be2b6 (cherry picked from commit 3ebeb4c490d1a36ff15f8f5767ef443c0bdac2b8) --- .../internal/app/ResolverActivity.java | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/core/java/com/android/internal/app/ResolverActivity.java b/core/java/com/android/internal/app/ResolverActivity.java index 233231cfcfdf0..fadc15912dc1e 100644 --- a/core/java/com/android/internal/app/ResolverActivity.java +++ b/core/java/com/android/internal/app/ResolverActivity.java @@ -1210,25 +1210,6 @@ public class ResolverActivity extends Activity implements if (TextUtils.isEmpty(packageName)) { pm.setDefaultBrowserPackageNameAsUser(ri.activityInfo.packageName, userId); } - } else { - // Update Domain Verification status - ComponentName cn = intent.getComponent(); - String packageName = cn.getPackageName(); - String dataScheme = (data != null) ? data.getScheme() : null; - - boolean isHttpOrHttps = (dataScheme != null) && - (dataScheme.equals(IntentFilter.SCHEME_HTTP) || - dataScheme.equals(IntentFilter.SCHEME_HTTPS)); - - boolean isViewAction = (action != null) && action.equals(Intent.ACTION_VIEW); - boolean hasCategoryBrowsable = (categories != null) && - categories.contains(Intent.CATEGORY_BROWSABLE); - - if (isHttpOrHttps && isViewAction && hasCategoryBrowsable) { - pm.updateIntentVerificationStatusAsUser(packageName, - PackageManager.INTENT_FILTER_DOMAIN_VERIFICATION_STATUS_ALWAYS, - userId); - } } } else { try { From 41484bba25aa9f658dcc0880f5c060eb92709907 Mon Sep 17 00:00:00 2001 From: Songchun Fan Date: Tue, 1 Dec 2020 12:02:44 -0800 Subject: [PATCH 3/5] [SettingsProvider] fix font size scale validator BUG: 156260178 Test: builds Change-Id: I32f3b7ece78ec8cc97c52a0484151a6a777aa9da (cherry picked from commit edeb081ad8b22b4a32ff90610f9f06182025e41c) --- .../settings/validators/SystemSettingsValidators.java | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java b/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java index c5d4fa9f1b409..cb610fc611421 100644 --- a/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java +++ b/packages/SettingsProvider/src/android/provider/settings/validators/SystemSettingsValidators.java @@ -89,15 +89,7 @@ public class SystemSettingsValidators { return value == null || value.length() < MAX_LENGTH; } }); - VALIDATORS.put( - System.FONT_SCALE, - value -> { - try { - return Float.parseFloat(value) >= 0; - } catch (NumberFormatException | NullPointerException e) { - return false; - } - }); + VALIDATORS.put(System.FONT_SCALE, new InclusiveFloatRangeValidator(0.85f, 1.3f)); VALIDATORS.put(System.DIM_SCREEN, BOOLEAN_VALIDATOR); VALIDATORS.put( System.DISPLAY_COLOR_MODE, From 7710def38c268ca2b35b21864e59835efb223cd8 Mon Sep 17 00:00:00 2001 From: Mohammad Samiul Islam Date: Fri, 13 Nov 2020 17:32:05 +0000 Subject: [PATCH 4/5] Clean up staged session data on validation failure Bug: 173132101 Test: atest StagedInstallInternalTest#testStagedInstallationShouldCleanUpOnValidationFailure Test: atest StagedInstallInternalTest#testStagedInstallationShouldCleanUpOnValidationFailureMultiPackage Change-Id: Idd6597cd0d2dda34a8a7626b585401eeee39c31f Merged-In: Idd6597cd0d2dda34a8a7626b585401eeee39c31f (cherry picked from commit b6dde00d038b899e5235b6a12d2ec6a2864f2022) (cherry picked from commit f54412183a19916f6ce704f113967b8b19ef6ed8) --- .../server/pm/PackageInstallerSession.java | 4 ++ .../StagedInstallInternalTest.java | 13 +++++++ .../host/StagedInstallInternalTest.java | 38 +++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index ea53132ae4095..c9f09545c3e6a 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -1581,6 +1581,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { destroyInternal(); // Dispatch message to remove session from PackageInstallerService. dispatchSessionFinished(error, detailMessage, null); + // TODO(b/173194203): clean up staged session in destroyInternal() call instead + if (isStaged() && stageDir != null) { + cleanStageDir(); + } } private void onStorageUnhealthy() { diff --git a/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java b/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java index 02597d5483617..e67354982b05d 100644 --- a/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java +++ b/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java @@ -96,6 +96,19 @@ public class StagedInstallInternalTest { assertSessionReady(sessionId); } + @Test + public void testStagedInstallationShouldCleanUpOnValidationFailure() throws Exception { + InstallUtils.commitExpectingFailure(AssertionError.class, "INSTALL_FAILED_INVALID_APK", + Install.single(TestApp.AIncompleteSplit).setStaged()); + } + + @Test + public void testStagedInstallationShouldCleanUpOnValidationFailureMultiPackage() + throws Exception { + InstallUtils.commitExpectingFailure(AssertionError.class, "INSTALL_FAILED_INVALID_APK", + Install.multi(TestApp.AIncompleteSplit, TestApp.B1, TestApp.Apex1).setStaged()); + } + private static void assertSessionReady(int sessionId) { assertSessionState(sessionId, (session) -> assertThat(session.isStagedSessionReady()).isTrue()); diff --git a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java index 9b432f7d0ca54..9ae4c2fc80ec7 100644 --- a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java +++ b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java @@ -21,6 +21,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertTrue; import com.android.ddmlib.Log; +import com.android.tradefed.device.DeviceNotAvailableException; import com.android.tradefed.testtype.DeviceJUnit4ClassRunner; import com.android.tradefed.testtype.junit4.BaseHostJUnit4Test; import com.android.tradefed.util.ProcessInfo; @@ -30,6 +31,10 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + @RunWith(DeviceJUnit4ClassRunner.class) public class StagedInstallInternalTest extends BaseHostJUnit4Test { @@ -87,6 +92,39 @@ public class StagedInstallInternalTest extends BaseHostJUnit4Test { runPhase("testSystemServerRestartDoesNotAffectStagedSessions_Verify"); } + @Test + public void testStagedInstallationShouldCleanUpOnValidationFailure() throws Exception { + List before = getStagingDirectories(); + runPhase("testStagedInstallationShouldCleanUpOnValidationFailure"); + List after = getStagingDirectories(); + assertThat(after).isEqualTo(before); + } + + @Test + public void testStagedInstallationShouldCleanUpOnValidationFailureMultiPackage() + throws Exception { + List before = getStagingDirectories(); + runPhase("testStagedInstallationShouldCleanUpOnValidationFailureMultiPackage"); + List after = getStagingDirectories(); + assertThat(after).isEqualTo(before); + } + + private List getStagingDirectories() throws DeviceNotAvailableException { + String baseDir = "/data/app-staging"; + try { + getDevice().enableAdbRoot(); + return getDevice().getFileEntry(baseDir).getChildren(false) + .stream().filter(entry -> entry.getName().matches("session_\\d+")) + .map(entry -> entry.getName()) + .collect(Collectors.toList()); + } catch (Exception e) { + // Return an empty list if any error + return Collections.EMPTY_LIST; + } finally { + getDevice().disableAdbRoot(); + } + } + private void restartSystemServer() throws Exception { // Restart the system server long oldStartTime = getDevice().getProcessByName("system_server").getStartTime(); From 2dfebdbfd521bd87dac323d3350733586e46ef52 Mon Sep 17 00:00:00 2001 From: Mohammad Samiul Islam Date: Fri, 13 Nov 2020 19:08:31 +0000 Subject: [PATCH 5/5] Delete orphaned staging directories for staged session on reboot Bug: 173132101 Test: StagedInstallInternalTest#testOrphanedStagingDirectoryGetsCleanedUpOnReboot Change-Id: If840f35245c2d049401d0d2f6539fe8c4625151e Merged-In: If840f35245c2d049401d0d2f6539fe8c4625151e (cherry picked from commit bfbf9608015ba6ff740639625b9a3ea26db60b76) (cherry picked from commit 37daf1ba806e4b2f952a7cc4e1e2babfbea8d8b1) --- .../android/server/pm/PackageInstallerService.java | 4 ++++ .../host/StagedInstallInternalTest.java | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/services/core/java/com/android/server/pm/PackageInstallerService.java b/services/core/java/com/android/server/pm/PackageInstallerService.java index 330f995235071..9f0efa5fad838 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerService.java +++ b/services/core/java/com/android/server/pm/PackageInstallerService.java @@ -299,6 +299,10 @@ public class PackageInstallerService extends IPackageInstaller.Stub implements final ArraySet unclaimedStages = newArraySet( stagingDir.listFiles(sStageFilter)); + // We also need to clean up orphaned staging directory for staged sessions + final File stagedSessionStagingDir = Environment.getDataStagingDirectory(volumeUuid); + unclaimedStages.addAll(newArraySet(stagedSessionStagingDir.listFiles())); + // Ignore stages claimed by active sessions for (int i = 0; i < mSessions.size(); i++) { final PackageInstallerSession session = mSessions.valueAt(i); diff --git a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java index 9ae4c2fc80ec7..28a5424199a4b 100644 --- a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java +++ b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java @@ -109,6 +109,19 @@ public class StagedInstallInternalTest extends BaseHostJUnit4Test { assertThat(after).isEqualTo(before); } + @Test + public void testOrphanedStagingDirectoryGetsCleanedUpOnReboot() throws Exception { + //create random directories in /data/app-staging folder + getDevice().enableAdbRoot(); + getDevice().executeShellCommand("mkdir /data/app-staging/session_123"); + getDevice().executeShellCommand("mkdir /data/app-staging/random_name"); + getDevice().disableAdbRoot(); + + assertThat(getStagingDirectories()).isNotEmpty(); + getDevice().reboot(); + assertThat(getStagingDirectories()).isEmpty(); + } + private List getStagingDirectories() throws DeviceNotAvailableException { String baseDir = "/data/app-staging"; try {