From 3f91f9392152c7d7a654096652dba9d89e4964c9 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 1 Jun 2020 13:56:28 -0600 Subject: [PATCH 1/2] Remove thread delegation when checking grants. This code was leftover from previous attempts at avoiding deadlock, and itself was causing deadlocks with the latest strategy. Reproduced deadlock in linked bugs, and confirmed that this CL fixes the underlying deadlock. Bug: 115619667, 157863128 Test: atest CtsAppSecurityHostTestCases:android.appsecurity.cts.AppSecurityTests#testPermissionDiffCert Change-Id: I9a8e5828090adebae1bfc306219c4b42d0c97432 --- .../server/am/ActivityManagerService.java | 26 ++----------------- 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index a4e45cd334bf2..caaa8371af53f 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -18900,30 +18900,8 @@ public class ActivityManagerService extends IActivityManager.Stub @Override public int checkContentProviderUriPermission(Uri uri, int userId, int callingUid, int modeFlags) { - // We can find ourselves needing to check Uri permissions while - // already holding the WM lock, which means reaching back here for - // the AM lock would cause an inversion. The WM team has requested - // that we use the strategy below instead of shifting where Uri - // grants are calculated. - - // Since we could also arrive here while holding the AM lock, we - // can't always delegate the call through the handler, and we need - // to delicately dance between the deadlocks. - if (Thread.currentThread().holdsLock(ActivityManagerService.this)) { - return ActivityManagerService.this.checkContentProviderUriPermission(uri, - userId, callingUid, modeFlags); - } else { - final CompletableFuture res = new CompletableFuture<>(); - mHandler.post(() -> { - res.complete(ActivityManagerService.this.checkContentProviderUriPermission(uri, - userId, callingUid, modeFlags)); - }); - try { - return res.get(); - } catch (InterruptedException | ExecutionException e) { - throw new RuntimeException(e); - } - } + return ActivityManagerService.this.checkContentProviderUriPermission(uri, + userId, callingUid, modeFlags); } @Override From 14c5c4d9974af798bbad6a6d5a5530f1c1f62d95 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Mon, 1 Jun 2020 13:57:12 -0600 Subject: [PATCH 2/2] Flip ENABLE_DYNAMIC_PERMISSIONS, attempt #4. Now that the underlying deadlock should be resolved, we can attempt to enable the dynamic permissions checking. Bug: 115619667 Test: atest android.appsecurity.cts.ExternalStorageHostTest Change-Id: Id0faea49bf8bfe798a8023784d267d1032c1317a --- .../java/com/android/server/uri/UriGrantsManagerService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/uri/UriGrantsManagerService.java b/services/core/java/com/android/server/uri/UriGrantsManagerService.java index 9476e9260c73f..3796c5fda411b 100644 --- a/services/core/java/com/android/server/uri/UriGrantsManagerService.java +++ b/services/core/java/com/android/server/uri/UriGrantsManagerService.java @@ -114,7 +114,7 @@ public class UriGrantsManagerService extends IUriGrantsManager.Stub { private static final String TAG = "UriGrantsManagerService"; // Maximum number of persisted Uri grants a package is allowed private static final int MAX_PERSISTED_URI_GRANTS = 128; - private static final boolean ENABLE_DYNAMIC_PERMISSIONS = false; + private static final boolean ENABLE_DYNAMIC_PERMISSIONS = true; private final Object mLock = new Object(); private final H mH;