From da88348f728d90515b513c4b39bbe4db01e2333d Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 26 Jun 2020 11:14:20 -0600 Subject: [PATCH] Nuanced Uri handling related to WM/AM locking. As part of detangling WM/AM locks, we added a Slog.wtf() to identify and lurking cases where we tried resolveActivity() with the WM lock held. This helped us uncover startActivityFromRecents(), but the logic there is unfortunately too risky to refactor. Instead, this change narrows our locking detection to only consider cases where we'd actually risk a deadlock; that is, when the thread is holding the WM lock and we're just about to acquire the AM lock. This change now avoids deadlock completely by assuming that we can't check Uri permissions in these cases where we know a deadlock is imminent; we instead use a safe-default that the Uri permission is denied, and Slog.wtf() to identify which paths are triggering it. The only path we've identified so far that triggers this logic is startActivityFromRecents() which can be reproduced by re-launching a recent task after a device reboot, which when combined with Uris that require "forceUriPermissions" is a very rare situation. Bug: 159937485, 115619667 Test: manual Change-Id: I4ed1c402703e0772c0164ac0acc64f3a754512f3 --- .../com/android/server/am/ActivityManagerService.java | 6 ++++++ .../core/java/com/android/server/wm/ActivityStarter.java | 9 +++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index e77b361c8c06e..c9dbacda368cc 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -8086,6 +8086,12 @@ public class ActivityManagerService extends IActivityManager.Stub } int checkContentProviderUriPermission(Uri uri, int userId, int callingUid, int modeFlags) { + if (Thread.holdsLock(mActivityTaskManager.getGlobalLock())) { + Slog.wtf(TAG, new IllegalStateException("Unable to check Uri permission" + + " because caller is holding WM lock; assuming permission denied")); + return PackageManager.PERMISSION_DENIED; + } + final String name = uri.getAuthority(); final long ident = Binder.clearCallingIdentity(); ContentProviderHolder holder = null; diff --git a/services/core/java/com/android/server/wm/ActivityStarter.java b/services/core/java/com/android/server/wm/ActivityStarter.java index 31712ef85addf..b3786212ea720 100644 --- a/services/core/java/com/android/server/wm/ActivityStarter.java +++ b/services/core/java/com/android/server/wm/ActivityStarter.java @@ -640,13 +640,10 @@ class ActivityStarter { } // If the caller hasn't already resolved the activity, we're willing - // to do so here, but because that may require acquiring the AM lock - // as part of calculating the NeededUriGrants, we must never hold - // the WM lock here to avoid deadlocking. + // to do so here. If the caller is already holding the WM lock here, + // and we need to check dynamic Uri permissions, then we're forced + // to assume those permissions are denied to avoid deadlocking. if (mRequest.activityInfo == null) { - if (Thread.holdsLock(mService.mGlobalLock)) { - Slog.wtf(TAG, new IllegalStateException("Caller must not hold WM lock")); - } mRequest.resolveActivity(mSupervisor); }