From 80af54eeda161bde6b0a363a1eb9d08c5a578b3c Mon Sep 17 00:00:00 2001 From: Spottedleaf Date: Mon, 15 May 2023 11:00:49 -0700 Subject: [PATCH] Synchronize PaperPermissionManager Since multiple regions can exist, there are concurrent accesses in this class. To prevent deadlock, the monitor is not held when recalculating permissions, as Permissable holds its own lock. This fixes CMEs originating from this class. --- ...7-Synchronize-PaperPermissionManager.patch | 190 ++++++++++++++++++ 1 file changed, 190 insertions(+) create mode 100644 patches/server/0027-Synchronize-PaperPermissionManager.patch diff --git a/patches/server/0027-Synchronize-PaperPermissionManager.patch b/patches/server/0027-Synchronize-PaperPermissionManager.patch new file mode 100644 index 0000000..e849a94 --- /dev/null +++ b/patches/server/0027-Synchronize-PaperPermissionManager.patch @@ -0,0 +1,190 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Spottedleaf +Date: Mon, 15 May 2023 10:58:06 -0700 +Subject: [PATCH] Synchronize PaperPermissionManager + +Since multiple regions can exist, there are concurrent accesses +in this class. To prevent deadlock, the monitor is not held +when recalculating permissions, as Permissable holds its own +lock. + +This fixes CMEs originating from this class. + +diff --git a/src/main/java/io/papermc/paper/plugin/manager/PaperPermissionManager.java b/src/main/java/io/papermc/paper/plugin/manager/PaperPermissionManager.java +index 92a69677f21b2c1c035119d8e5a6af63fa19b801..6a239a3da5676cd781057d1b4af68aa97de27881 100644 +--- a/src/main/java/io/papermc/paper/plugin/manager/PaperPermissionManager.java ++++ b/src/main/java/io/papermc/paper/plugin/manager/PaperPermissionManager.java +@@ -32,7 +32,9 @@ abstract class PaperPermissionManager implements PermissionManager { + @Override + @Nullable + public Permission getPermission(@NotNull String name) { ++ synchronized (this) { // Folia - synchronized + return this.permissions().get(name.toLowerCase(java.util.Locale.ENGLISH)); ++ } // Folia - synchronized + } + + @Override +@@ -52,12 +54,24 @@ abstract class PaperPermissionManager implements PermissionManager { + private void addPermission(@NotNull Permission perm, boolean dirty) { + String name = perm.getName().toLowerCase(java.util.Locale.ENGLISH); + ++ Boolean recalc; // Folia - synchronized ++ synchronized (this) { // Folia - synchronized + if (this.permissions().containsKey(name)) { + throw new IllegalArgumentException("The permission " + name + " is already defined!"); + } + + this.permissions().put(name, perm); +- this.calculatePermissionDefault(perm, dirty); ++ recalc = this.calculatePermissionDefault(perm, dirty); ++ } // Folia - synchronized ++ // Folia start - synchronize this class - we hold a lock now, prevent deadlock by moving this out ++ if (recalc != null) { ++ if (recalc.booleanValue()) { ++ this.dirtyPermissibles(true); ++ } else { ++ this.dirtyPermissibles(false); ++ } ++ } ++ // Folia end - synchronize this class - we hold a lock now, prevent deadlock by moving this out + } + + @Override +@@ -80,42 +94,58 @@ abstract class PaperPermissionManager implements PermissionManager { + + @Override + public void recalculatePermissionDefaults(@NotNull Permission perm) { ++ Boolean recalc = null; // Folia - synchronized ++ synchronized (this) { // Folia - synchronized + // we need a null check here because some plugins for some unknown reason pass null into this? + if (perm != null && this.permissions().containsKey(perm.getName().toLowerCase(Locale.ENGLISH))) { + this.defaultPerms().get(true).remove(perm); + this.defaultPerms().get(false).remove(perm); + +- this.calculatePermissionDefault(perm, true); ++ recalc = this.calculatePermissionDefault(perm, true); // Folia - synchronized ++ } ++ } // Folia - synchronized ++ // Folia start - synchronize this class - we hold a lock now, prevent deadlock by moving this out ++ if (recalc != null) { ++ if (recalc.booleanValue()) { ++ this.dirtyPermissibles(true); ++ } else { ++ this.dirtyPermissibles(false); ++ } + } ++ // Folia end - synchronize this class - we hold a lock now, prevent deadlock by moving this out + } + +- private void calculatePermissionDefault(@NotNull Permission perm, boolean dirty) { ++ private Boolean calculatePermissionDefault(@NotNull Permission perm, boolean dirty) { // Folia - synchronize this class + if ((perm.getDefault() == PermissionDefault.OP) || (perm.getDefault() == PermissionDefault.TRUE)) { + this.defaultPerms().get(true).add(perm); + if (dirty) { +- this.dirtyPermissibles(true); ++ return Boolean.TRUE; // Folia - synchronize this class - we hold a lock now, prevent deadlock by moving this out + } + } + if ((perm.getDefault() == PermissionDefault.NOT_OP) || (perm.getDefault() == PermissionDefault.TRUE)) { + this.defaultPerms().get(false).add(perm); + if (dirty) { +- this.dirtyPermissibles(false); ++ return Boolean.FALSE; // Folia - synchronize this class - we hold a lock now, prevent deadlock by moving this out + } + } ++ return null; // Folia - synchronize this class + } + + + @Override + public void subscribeToPermission(@NotNull String permission, @NotNull Permissible permissible) { ++ synchronized (this) { // Folia - synchronized + String name = permission.toLowerCase(java.util.Locale.ENGLISH); + Map map = this.permSubs().computeIfAbsent(name, k -> new WeakHashMap<>()); + + map.put(permissible, true); ++ } // Folia - synchronized + } + + @Override + public void unsubscribeFromPermission(@NotNull String permission, @NotNull Permissible permissible) { + String name = permission.toLowerCase(java.util.Locale.ENGLISH); ++ synchronized (this) { // Folia - synchronized + Map map = this.permSubs().get(name); + + if (map != null) { +@@ -125,11 +155,13 @@ abstract class PaperPermissionManager implements PermissionManager { + this.permSubs().remove(name); + } + } ++ } // Folia - synchronized + } + + @Override + @NotNull + public Set getPermissionSubscriptions(@NotNull String permission) { ++ synchronized (this) { // Folia - synchronized + String name = permission.toLowerCase(java.util.Locale.ENGLISH); + Map map = this.permSubs().get(name); + +@@ -138,17 +170,21 @@ abstract class PaperPermissionManager implements PermissionManager { + } else { + return ImmutableSet.copyOf(map.keySet()); + } ++ } // Folia - synchronized + } + + @Override + public void subscribeToDefaultPerms(boolean op, @NotNull Permissible permissible) { ++ synchronized (this) { // Folia - synchronized + Map map = this.defSubs().computeIfAbsent(op, k -> new WeakHashMap<>()); + + map.put(permissible, true); ++ } // Folia - synchronized + } + + @Override + public void unsubscribeFromDefaultPerms(boolean op, @NotNull Permissible permissible) { ++ synchronized (this) { // Folia - synchronized + Map map = this.defSubs().get(op); + + if (map != null) { +@@ -158,11 +194,13 @@ abstract class PaperPermissionManager implements PermissionManager { + this.defSubs().remove(op); + } + } ++ } // Folia - synchronized + } + + @Override + @NotNull + public Set getDefaultPermSubscriptions(boolean op) { ++ synchronized (this) { // Folia - synchronized + Map map = this.defSubs().get(op); + + if (map == null) { +@@ -170,19 +208,24 @@ abstract class PaperPermissionManager implements PermissionManager { + } else { + return ImmutableSet.copyOf(map.keySet()); + } ++ } // Folia - synchronized + } + + @Override + @NotNull + public Set getPermissions() { ++ synchronized (this) { // Folia - synchronized + return new HashSet<>(this.permissions().values()); ++ } // Folia - synchronized + } + + @Override + public void clearPermissions() { ++ synchronized (this) { // Folia - synchronized + this.permissions().clear(); + this.defaultPerms().get(true).clear(); + this.defaultPerms().get(false).clear(); ++ } // Folia - synchronized + } + +