From 4cdc28b0ba5427953862b1a697d43741954f7291 Mon Sep 17 00:00:00 2001 From: Varun Shah Date: Wed, 18 Nov 2020 19:59:44 +0000 Subject: 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) --- .../android/server/am/ActivityManagerService.java | 117 +++++++++++---------- 1 file changed, 59 insertions(+), 58 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 091c77ecaac3..7acb1bf47e78 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"); + + // 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: 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"); - } - } - - 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; + } + 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; } - // 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) { -- cgit v1.2.3 From bfc112baf14fa4cb86510640c167f8a2b5a2e7ef Mon Sep 17 00:00:00 2001 From: Mohammad Samiul Islam Date: Fri, 13 Nov 2020 17:32:05 +0000 Subject: 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) --- .../android/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 ea53132ae409..c9f09545c3e6 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 02597d548361..e67354982b05 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 9b432f7d0ca5..9ae4c2fc80ec 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(); -- cgit v1.2.3 From c6185ceabec501794edd79e526af56a212bd5e68 Mon Sep 17 00:00:00 2001 From: Mohammad Samiul Islam Date: Fri, 13 Nov 2020 19:08:31 +0000 Subject: 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) --- .../java/com/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 330f99523507..9f0efa5fad83 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 9ae4c2fc80ec..28a5424199a4 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 { -- cgit v1.2.3 From 80ac2173f1e1828071d79169c9191d51c8b1116c Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 28 Oct 2020 13:25:44 -0700 Subject: 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) --- .../com/android/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 233231cfcfdf..fadc15912dc1 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 { -- cgit v1.2.3 From be820f93ab81a7b04e3b5bfdfbff1ca02ff29ce3 Mon Sep 17 00:00:00 2001 From: Songchun Fan Date: Tue, 1 Dec 2020 12:02:44 -0800 Subject: [SettingsProvider] fix font size scale validator BUG: 156260178 Test: builds Change-Id: I32f3b7ece78ec8cc97c52a0484151a6a777aa9da (cherry picked from commit edeb081ad8b22b4a32ff90610f9f06182025e41c) --- .../provider/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 c5d4fa9f1b40..cb610fc61142 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, -- cgit v1.2.3