Let's proactively catch the stack overflow issue

GlowingInk · Feb 12, 2024 · 66d4269 · 66d4269
1 parent e5e656e
commit 66d4269
Show file tree

Hide file tree

Showing 9 changed files with 33 additions and 19 deletions.
diff --git a/reactions/src/main/java/fun/reactions/model/activators/ActivationContext.java b/reactions/src/main/java/fun/reactions/model/activators/ActivationContext.java
@@ -66,7 +66,7 @@ public ActivationContext(@Nullable Player player, boolean async) {
     @Contract(pure = true)
     public final @NotNull Environment createEnvironment(@NotNull ReActions.Platform platform, @NotNull String activatorName) {
         initialize();
-        return new Environment(platform, activatorName, variables, player);
+        return new Environment(platform, activatorName, variables, player, 0);
     }
 
     public final void initialize() {

diff --git a/reactions/src/main/java/fun/reactions/model/environment/Environment.java b/reactions/src/main/java/fun/reactions/model/environment/Environment.java
@@ -10,22 +10,25 @@
  * Context created per activator
  */
 public final class Environment {
+    private static final int MAX_DEPTH = 256;
+
     private final ReActions.Platform platform;
     private final String activatorName;
     private final Player player;
-
     private final Variables variables;
+    private final int depth;
     private final boolean async; // TODO
 
-    public Environment(@NotNull ReActions.Platform platform, @NotNull String activator, @NotNull Variables variables, @Nullable Player player) {
-        this(platform, activator, variables, player, false);
+    public Environment(@NotNull ReActions.Platform platform, @NotNull String activator, @NotNull Variables variables, @Nullable Player player, int depth) {
+        this(platform, activator, variables, player, depth, false);
     }
 
-    public Environment(@NotNull ReActions.Platform platform, @NotNull String activator, @NotNull Variables variables, @Nullable Player player, boolean async) {
+    public Environment(@NotNull ReActions.Platform platform, @NotNull String activator, @NotNull Variables variables, @Nullable Player player, int depth, boolean async) {
         this.platform = platform;
         this.variables = variables;
         this.activatorName = activator;
         this.player = player;
+        this.depth = depth;
         this.async = async;
     }
 
@@ -49,6 +52,14 @@ public boolean hasPlayer() {
         return variables;
     }
 
+    public int getDepth() {
+        return depth;
+    }
+
+    public boolean isStepAllowed() {
+        return depth < MAX_DEPTH;
+    }
+
     public boolean isAsync() {
         return this.async;
     }

diff --git a/reactions/src/main/java/fun/reactions/module/basic/actions/RunFunctionAction.java b/reactions/src/main/java/fun/reactions/module/basic/actions/RunFunctionAction.java
@@ -27,18 +27,19 @@ public boolean proceed(@NotNull Environment env, @NotNull String paramsStr) {
             env.warn("Failed to run FUNCTION activator " + id + ". Wrong activator type.");
             return false;
         }
-        try {
+        if (env.isStepAllowed()) {
             activator.getLogic().execute(new Environment(
                     env.getPlatform(),
                     id,
                     env.getVariables(),
                     env.getPlayer(),
+                    env.getDepth() + 1,
                     env.isAsync()
             ));
-        } catch (StackOverflowError error) {
+        } else {
             env.getPlatform().logger().error(
-                    "RUN_FUNCTION action failed in '" + env.getActivatorName() + "' due to stack overflow. " +
-                    "Consider limiting the usage of looped RUN_FUNCTION actions and FUNCTION placeholders " +
+                    "RUN_FUNCTION action in '" + id + "' was stopped at the depth '" + env.getDepth() + "' to prevent stack overflow. " +
+                    "Consider limiting the usage of recursive RUN_FUNCTION actions and FUNCTION placeholders " +
                     "or try using EXECUTE actions."
             );
             return false;

diff --git a/reactions/src/main/java/fun/reactions/module/basic/placeholders/FunctionPlaceholder.java b/reactions/src/main/java/fun/reactions/module/basic/placeholders/FunctionPlaceholder.java
@@ -19,18 +19,19 @@ public class FunctionPlaceholder implements Placeholder {
         }
         String id = activator.getLogic().getName();
         Variables vars = new Variables();
-        try {
+        if (env.isStepAllowed()) {
             activator.getLogic().execute(new Environment(
                     env.getPlatform(),
                     id,
                     vars,
                     env.getPlayer(),
+                    env.getDepth() + 1,
                     env.isAsync()
             ));
-        } catch (StackOverflowError error) {
+        } else  {
             env.getPlatform().logger().error(
-                    "FUNCTION placeholder (" + params + ") failed in '" + env.getActivatorName() + "' due to stack overflow. " +
-                    "Consider limiting the usage of looped FUNCTION placeholders and RUN_FUNCTION actions."
+                    "FUNCTION placeholder in '" + id + "' was stopped at the depth '" + env.getDepth() + "' to prevent stack overflow. " +
+                    "Consider limiting the usage of recursive RUN_FUNCTION actions and FUNCTION placeholders."
             );
         }
         return vars.getStringUnsafe("return");

diff --git a/reactions/src/main/java/fun/reactions/module/papi/external/RaPapiExpansion.java b/reactions/src/main/java/fun/reactions/module/papi/external/RaPapiExpansion.java
@@ -50,6 +50,7 @@ public String onRequest(OfflinePlayer player, @NotNull String param) {
                 "",
                 new Variables(),
                 player instanceof Player onlinePlayer ? onlinePlayer : null,
+                0,
                 true // We don't know if we're in async, so let's consider we are
         ), param);
     }

diff --git a/reactions/src/main/java/fun/reactions/time/wait/WaitTask.java b/reactions/src/main/java/fun/reactions/time/wait/WaitTask.java
@@ -27,7 +27,7 @@ public boolean isTime() {
 
     public void execute(@NotNull ReActions.Platform platform) {
         Player player = playerId == null ? null : Bukkit.getPlayer(playerId);
-        Logic.executeActions(new Environment(platform, "", variables, player), actions);
+        Logic.executeActions(new Environment(platform, "", variables, player, 0), actions);
     }
 
     @Override

diff --git a/reactions/src/test/java/fun/reactions/module/basic/flags/EitherFlagTest.java b/reactions/src/test/java/fun/reactions/module/basic/flags/EitherFlagTest.java
@@ -22,15 +22,15 @@ public void testProceed() {
         EitherFlag flag = new EitherFlag();
         // TODO Use DataProvider
         assertTrue(flag.proceed(
-                new Environment(platform, "", new Variables(), null),
+                new Environment(platform, "", new Variables(), null, 0),
                 "test1:{some value} test2:other test1:repeat"
         ));
         assertFalse(flag.proceed(
-                new Environment(platform, "", new Variables(), null),
+                new Environment(platform, "", new Variables(), null, 0),
                 "test1:{some value} test1:repeat"
         ));
         assertTrue(flag.proceed(
-                new Environment(platform, "", new Variables(), null),
+                new Environment(platform, "", new Variables(), null, 0),
                 "!test1:inverted"
         ));
     }

diff --git a/reactions/src/test/java/fun/reactions/placeholders/LegacyPlaceholdersManagerTest.java b/reactions/src/test/java/fun/reactions/placeholders/LegacyPlaceholdersManagerTest.java
@@ -17,7 +17,7 @@ public void testParsePlaceholders() {
         vars.set("test", "y\\ay");
         vars.set("another", "%test%\\,");
         assertEquals(
-                mgr.parse(new Environment(null, "", vars, null), "Foo %another% bar %ignored%"),
+                mgr.parse(new Environment(null, "", vars, null, 0), "Foo %another% bar %ignored%"),
                 "Foo y\\ay\\, bar %ignored%"
         );
     }

diff --git a/reactions/src/test/java/fun/reactions/placeholders/ModernPlaceholdersManagerTest.java b/reactions/src/test/java/fun/reactions/placeholders/ModernPlaceholdersManagerTest.java
@@ -17,7 +17,7 @@ public void testParsePlaceholders() {
         vars.set("test", "y\\ay");
         vars.set("another", "%[test]\\,");
         assertEquals(
-                mgr.parse(new Environment(null, "", vars, null), "Foo %[another] bar %[ignored]"),
+                mgr.parse(new Environment(null, "", vars, null, 0), "Foo %[another] bar %[ignored]"),
                 "Foo y\\ay\\, bar %[ignored]"
         );
     }