fix: detect use-after-destroy on parameters (#1273)

Author: Frotty

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/lua/translation/ExprTranslation.java

@@ -69,7 +69,7 @@ public static LuaExpr translate(ImFuncRef e, LuaTranslator tr) {
                             LuaAst.LuaAssignment(LuaAst.LuaExprVarAccess(tempRes),
                                 LuaAst.LuaExprFunctionCall(tr.luaFunc.getFor(e.getFunc()), LuaAst.LuaExprlist(LuaAst.LuaExprVarAccess(dots.copy())))))
                     ),
-                    LuaAst.LuaLiteral("function(err) if err == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"lua callback error: \" .. tostring(err)) xpcall(function() " + callErrorFunc(tr, "tostring(err)") + " end, function(err2) if err2 == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"error reporting error: \" .. tostring(err2)) BJDebugMsg(\"while reporting: \" .. tostring(err))  end) end"),
+                    LuaAst.LuaLiteral("function(err) if err == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"lua callback error: \" .. tostring(err)) xpcall(function() " + callErrorFunc(tr, "tostring(err)", "in lua callback error handler") + " end, function(err2) if err2 == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"error reporting error: \" .. tostring(err2)) BJDebugMsg(\"while reporting: \" .. tostring(err))  end) end"),
                     LuaAst.LuaExprVarAccess(dots.copy())
                 )
             ));
@@ -83,7 +83,7 @@ public static LuaExpr translate(ImFuncRef e, LuaTranslator tr) {
                             LuaAst.LuaExprFunctionCall(tr.luaFunc.getFor(e.getFunc()), LuaAst.LuaExprlist(LuaAst.LuaExprVarAccess(dots.copy())))
                         )
                     ),
-                    LuaAst.LuaLiteral("function(err) if err == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"lua callback error: \" .. tostring(err)) xpcall(function() " + callErrorFunc(tr, "tostring(err)") + " end, function(err2) if err2 == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"error reporting error: \" .. tostring(err2)) BJDebugMsg(\"while reporting: \" .. tostring(err))  end) end"),
+                    LuaAst.LuaLiteral("function(err) if err == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"lua callback error: \" .. tostring(err)) xpcall(function() " + callErrorFunc(tr, "tostring(err)", "in lua callback error handler") + " end, function(err2) if err2 == \"" + WURST_ABORT_THREAD_SENTINEL + "\" then return end BJDebugMsg(\"error reporting error: \" .. tostring(err2)) BJDebugMsg(\"while reporting: \" .. tostring(err))  end) end"),
                     LuaAst.LuaExprVarAccess(dots.copy())
                 )
             ));
@@ -92,13 +92,14 @@ public static LuaExpr translate(ImFuncRef e, LuaTranslator tr) {
     }
 
     static String callErrorFunc(LuaTranslator tr, String msg) {
+        return callErrorFunc(tr, msg, "<lua error>");
+    }
+
+    static String callErrorFunc(LuaTranslator tr, String msg, String stackPos) {
         LuaFunction ef = tr.getErrorFunc();
         if (ef != null) {
             if (ef.getParams().size() == 2) {
-                // StackTraceInjector adds the second parameter to ErrorHandling.error.  Lua's
-                // xpcall handler already has the real failing call stack, so pass it through
-                // instead of the old placeholder (which made -stacktraces ineffective in Lua).
-                return ef.getName() + "(" + msg + ", debug.traceback(" + msg + ", 2))";
+                return ef.getName() + "(" + msg + ", \"" + stackPos + "\")";
             }
             return ef.getName() + "(" + msg + ")";
         }

de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/validation/controlflow/DataflowAnomalyAnalysis.java

@@ -19,16 +19,22 @@
 //w(11)->{r(17), r(19)} & w(19)->{r(17), r(19)} & w(22)
 class VarStates {
     final ImmutableMap<LocalVarDef, VState> states;
+    final ImmutableSet<NameDef> destroyedParameters;
     final boolean thisDestroyed;
 
     public VarStates(ImmutableMap<LocalVarDef, VState> states, boolean thisDestroyed) {
+        this(states, ImmutableSet.of(), thisDestroyed);
+    }
+
+    public VarStates(ImmutableMap<LocalVarDef, VState> states, ImmutableSet<NameDef> destroyedParameters, boolean thisDestroyed) {
         this.states = states;
+        this.destroyedParameters = destroyedParameters;
         this.thisDestroyed = thisDestroyed;
     }
 
     VarStates merge(VarStates other) {
         ImmutableMap<LocalVarDef, VState> merged = Utils.mergeMaps(states, other.states, VState::merge);
-        return new VarStates(merged, thisDestroyed || other.thisDestroyed);
+        return new VarStates(merged, ImmutableSet.<NameDef>builder().addAll(destroyedParameters).addAll(other.destroyedParameters).build(), thisDestroyed || other.thisDestroyed);
     }
 
     @Override
@@ -37,12 +43,13 @@ public boolean equals(Object o) {
         if (o == null || getClass() != o.getClass()) return false;
         VarStates varStates = (VarStates) o;
         return thisDestroyed == varStates.thisDestroyed &&
-                Objects.equals(states, varStates.states);
+                Objects.equals(states, varStates.states) &&
+                Objects.equals(destroyedParameters, varStates.destroyedParameters);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(states, thisDestroyed);
+        return Objects.hash(states, destroyedParameters, thisDestroyed);
     }
 
     public static VarStates initial(Set<LocalVarDef> r) {
@@ -55,7 +62,7 @@ public static VarStates initial(Set<LocalVarDef> r) {
 
     public boolean destroyed(NameDef v) {
         VState s = states.get(v);
-        return s != null && s.mightBeDestroyed;
+        return (s != null && s.mightBeDestroyed) || destroyedParameters.contains(v);
     }
 
     public boolean uninitialized(NameDef v) {
@@ -78,7 +85,7 @@ public VarStates addRead(LocalVarDef v, Element r) {
         ImmutableMap<LocalVarDef, VState> rs = builder
                 .put(v, s)
                 .build();
-        return new VarStates(rs, thisDestroyed);
+        return new VarStates(rs, destroyedParameters, thisDestroyed);
     }
 
     public ImmutableSet<WStatement> getUnreadWrites(NameDef var) {
@@ -107,7 +114,7 @@ public VarStates addWrite(LocalVarDef var, WStatement s) {
         }
         vState = vState.addWrite(s);
         res.put(var, vState);
-        return new VarStates(res.build(), thisDestroyed);
+        return new VarStates(res.build(), destroyedParameters, thisDestroyed);
     }
 
     public VarStates addDestroy(LocalVarDef var) {
@@ -118,7 +125,26 @@ public VarStates addDestroy(LocalVarDef var) {
             }
         }
         res.put(var, VState.destroyed);
-        return new VarStates(res.build(), thisDestroyed);
+        return new VarStates(res.build(), destroyedParameters, thisDestroyed);
+    }
+
+    public VarStates addDestroyParameter(NameDef var) {
+        ImmutableSet.Builder<NameDef> destroyed = ImmutableSet.builder();
+        destroyed.addAll(destroyedParameters).add(var);
+        return new VarStates(states, destroyed.build(), thisDestroyed);
+    }
+
+    public VarStates clearDestroyParameter(NameDef var) {
+        if (!destroyedParameters.contains(var)) {
+            return this;
+        }
+        ImmutableSet.Builder<NameDef> remaining = ImmutableSet.builder();
+        for (NameDef destroyed : destroyedParameters) {
+            if (destroyed != var) {
+                remaining.add(destroyed);
+            }
+        }
+        return new VarStates(states, remaining.build(), thisDestroyed);
     }
 
 
@@ -144,7 +170,7 @@ public boolean isThisDestroyed() {
     }
 
     public VarStates withThisDestroyed(boolean thisDestroyed) {
-        return new VarStates(states, thisDestroyed);
+        return new VarStates(states, destroyedParameters, thisDestroyed);
     }
 
 
@@ -300,6 +326,9 @@ VarStates calculate(WStatement s, VarStates incoming) {
                 if (isLocalVarDef(destroyedVar)) {
                     return incoming.addDestroy((LocalVarDef) destroyedVar);
                 }
+                if (destroyedVar instanceof WParameter || destroyedVar instanceof WShortParameter) {
+                    return incoming.addDestroyParameter(destroyedVar);
+                }
             } else if (destr.getDestroyedObj() instanceof ExprThis) {
                 return incoming.withThisDestroyed(true);
             }
@@ -318,6 +347,9 @@ VarStates calculate(WStatement s, VarStates incoming) {
                 LocalVarDef lv = (LocalVarDef) n;
                 return incoming.addWrite(lv, s);
             }
+            if (n instanceof WParameter || n instanceof WShortParameter) {
+                return incoming.clearDestroyParameter(n);
+            }
         }
         return incoming;
     }
@@ -509,7 +541,9 @@ void checkFinal(VarStates fin) {
                 @Nullable ExprClosure exprClosure = errorPos.attrNearestExprClosure();
                 @Nullable ExprClosure exprClosure1 = var.attrNearestExprClosure();
                 if (exprClosure != null && exprClosure != exprClosure1) {
-                    errorPos.addWarning("This assignment to the closure-captured variable " + Utils.printElement(var) + " has no effect outside the closure.");
+                    errorPos.addWarning("This assignment to the closure-captured variable " + Utils.printElement(var)
+                        + " does not propagate outside the closure because closures capture locals by value. "
+                        + "If you want to update the outer value, use reference(" + var.getName() + ") deliberately.");
                 } else {
                     errorPos.addWarning("The assignment to " + Utils.printElement(var) + " is never read.");
                 }

de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/BugTests.java

@@ -928,7 +928,7 @@ public void unreadVarWarning2() { // #380
 
     @Test
     public void unreadVarWarning3() { // #380
-        testAssertErrorsLines(true, "closure-captured variable",
+        testAssertErrorsLines(true, "does not propagate outside the closure because closures capture locals by value",
             "package test",
             "@annotation public function annotation()",
             "@annotation public function extern()",

de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/FlowAnalysisTests.java

@@ -120,6 +120,38 @@ public void destroyDataflowTest() {
         );
     }
 
+    @Test
+    public void destroyParameterThenUseIsReported() {
+        testAssertErrorsLines(false, "Variable a may have been destroyed already",
+                "package test",
+                "class A",
+                "    function foo()",
+                "function consume(A a)",
+                "    destroy a",
+                "    a.foo()",
+                "init",
+                "    consume(new A())"
+        );
+    }
+
+    @Test
+    public void destroyShortParameterThenUseIsReported() {
+        testAssertErrorsLines(false, "Variable a may have been destroyed already",
+                "package test",
+                "class A",
+                "    function foo()",
+                "interface Consumer",
+                "    function accept(A a)",
+                "function apply(Consumer c)",
+                "    c.accept(new A())",
+                "init",
+                "    apply((A a) -> begin",
+                "        destroy a",
+                "        a.foo()",
+                "    end)"
+        );
+    }
+
     @Test
     public void destroyThisDataflowTest() {
         testAssertErrorsLines(false, "Cannot access 'this' because it might already have been destroyed.",

de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaTranslationTests.java

@@ -277,7 +277,7 @@ public void stacktraceStringsNotInjectedIntoNumericComparisons() {
     }
 
     @Test
-    public void luaErrorWrappersPassNativeTracebackToStacktracedErrorHandler() throws IOException {
+    public void luaErrorWrappersAvoidUnavailableNativeTraceback() throws IOException {
         String compiled = compileLuaWithRunArgs(
             "LuaTranslationTests_luaErrorWrappersPassNativeTracebackToStacktracedErrorHandler",
             false,
@@ -289,7 +289,7 @@ public void luaErrorWrappersPassNativeTracebackToStacktracedErrorHandler() throw
             "init",
             "    fail()"
         );
-        assertTrue(compiled.contains("debug.traceback"));
+        assertFalse(compiled.contains("debug.traceback"));
     }
 
     @Test
@@ -2423,6 +2423,29 @@ public void luaFunctionRefWrapperForwardsVarargs() throws IOException {
         assertFalse(compiled.contains("ForForce(f, function (...) \n\t\t\tlocal tempRes"));
     }
 
+    @Test
+    public void luaFunctionRefStacktraceHandlerUsesWurstStackPosition() throws IOException {
+        CU errorHandling = new CU("ErrorHandling.wurst", String.join("\n",
+            "package ErrorHandling",
+            "public function error(string msg)",
+            "    skip"));
+        String compiled = compileLuaWithCUs(
+            "LuaTranslationTests_luaFunctionRefStacktraceHandlerUsesWurstStackPosition",
+            false,
+            Collections.singletonList(errorHandling),
+            "package Test",
+            "import ErrorHandling",
+            "native apply(code c)",
+            "init",
+            "    apply(() -> error(\"callback\"))"
+        );
+        assertFalse(compiled.contains("debug.traceback"));
+        assertContainsRegex(compiled,
+            "function\\s+error1\\([^\\)]*__wurst_stackPos");
+        assertContainsRegex(compiled,
+            "error1\\(tostring\\(err\\), \\\"in lua callback error handler\\\"\\)");
+    }
+
     @Test
     public void forForceIsRemappedToWurstHelperInLua() throws IOException {
         test().testLua(true).withStdLib().lines(