Missing property exception for closure parameter when used in while statement

[robinfriedli]

Hi,

I have the following sample code which works fine when running it in a groovy class:

char somec = 'a'
def list = new ArrayList<String>()
list.add('dew')
list.add('eff')
list.add('fde')
while (list.stream().noneMatch({s -> s.indexOf((int) somec) >= 0})) {
    somec++
}

However, when trying to execute this code via the GroovyShell, a MissingPropertyException is thrown for the closure parameter 's'. It seems that org.kohsuke.groovy.sandbox.impl.Checker is trying to find the property on the Script object, rather than within the closure. When changing the while to an if statement everything seems to work and org.kohsuke.groovy.sandbox.impl.Checker#checkedGetProperty is never called in the first place.

Full stack trace:

groovy.lang.MissingPropertyException: No such property: s for class: Script1
	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.unwrap(ScriptBytecodeAdapter.java:65)
	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.getProperty(ScriptBytecodeAdapter.java:470)
	at org.kohsuke.groovy.sandbox.impl.Checker$6.call(Checker.java:293)
	at org.kohsuke.groovy.sandbox.GroovyInterceptor.onGetProperty(GroovyInterceptor.java:68)
	at org.kohsuke.groovy.sandbox.impl.Checker$6.call(Checker.java:291)
	at org.kohsuke.groovy.sandbox.impl.Checker.checkedGetProperty(Checker.java:295)
	at org.kohsuke.groovy.sandbox.impl.Checker$checkedGetProperty$4.callStatic(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:55)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:196)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:232)
	at Script1$_run_closure1.doCall(Script1.groovy:6)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:101)
	at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:323)
	at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:263)
	at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1041)
	at groovy.lang.Closure.call(Closure.java:405)
	at org.codehaus.groovy.runtime.ConvertedClosure.invokeCustom(ConvertedClosure.java:50)
	at org.codehaus.groovy.runtime.ConversionHandler.invoke(ConversionHandler.java:122)
	at com.sun.proxy.$Proxy127.test(Unknown Source)
	at java.base/java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90)
	at java.base/java.util.ArrayList$ArrayListSpliterator.tryAdvance(ArrayList.java:1631)
	at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:127)
	at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:502)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:488)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
	at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
	at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.noneMatch(ReferencePipeline.java:538)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.codehaus.groovy.runtime.callsite.PlainObjectMetaMethodSite.doInvoke(PlainObjectMetaMethodSite.java:43)
	at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap.invoke(PojoMetaMethodSite.java:200)
	at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call(PojoMetaMethodSite.java:53)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:47)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:115)
	at org.kohsuke.groovy.sandbox.impl.Checker$1.call(Checker.java:160)
	at org.kohsuke.groovy.sandbox.GroovyInterceptor.onMethodCall(GroovyInterceptor.java:23)
	at net.robinfriedli.botify.scripting.GroovyWhitelistInterceptor.super$2$onMethodCall(GroovyWhitelistInterceptor.groovy)
	at jdk.internal.reflect.GeneratedMethodAccessor316.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:101)
	at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:323)
	at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1217)
	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.invokeMethodOnSuperN(ScriptBytecodeAdapter.java:144)
	at net.robinfriedli.botify.scripting.GroovyWhitelistInterceptor.onMethodCall(GroovyWhitelistInterceptor.groovy:46)
	at org.kohsuke.groovy.sandbox.impl.Checker$1.call(Checker.java:158)
	at org.kohsuke.groovy.sandbox.impl.Checker.checkedCall(Checker.java:162)
	at org.kohsuke.groovy.sandbox.impl.Checker$checkedCall$1.callStatic(Unknown Source)
	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:55)
	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:196)
	at Script1.run(Script1.groovy:6)
	at groovy.lang.GroovyShell.evaluate(GroovyShell.java:443)
	at groovy.lang.GroovyShell.evaluate(GroovyShell.java:481)
	at groovy.lang.GroovyShell.evaluate(GroovyShell.java:452)

[robinfriedli]

My current workaround is to do the following, which also seems to work

def noneMatch = list.stream().noneMatch({s -> s.indexOf((int) somec) >= 0})
while (noneMatch) {
    somec++
    noneMatch = list.stream().noneMatch({s -> s.indexOf((int) somec) >= 0})
}

[dwnusbaum]

Thanks for the bug report! I looked into this today to try to understand whether the bug could be used to bypass the sandbox by confusing the transformer. I think it's safe, since the problem appears to be that the expression in question is being transformed by the sandbox multiple times.

This happens because ClassCodeExpressionTransformer.visitWhileLoop first transforms the condition expression for the loop, then visits the body by calling super.visitWhileLoop, which ends up visiting the condition expression again. Because of this, the same problem probably also affects do-while loops, and maybe also for loops, since the visitor code for them uses similar logic.

Explanation of the bug and the patch I came up with while testing:

When ClassCodeExpressionTransformer.visitWhileLoop is called, everything works fine in the call to transform the condition expression. Inside of SandboxTransformer$VisitorImpl.innerTransform, the parameter for the closure is declared correctly, and so when the variable is transformed it gets ignored here because it is a local variable.

The call to super.visitWhileLoop is where things go wrong. That method ends up calling CodeVisitorSupport.visitWhileLoop, which visits the condition expression again, but through visit instead of transform. The condition expression does not go through transform until visitExpressionStatement is called on the body of the closure, which means that the parameters of the closure are not declared in the current scope, and so we go through the else branch here when transforming s, which ends up causing the problem.

I think the fix is relatively straightforward. ScopeTrackingClassCodeExpressionTransformer.visitClosureExpression needs to declare the closure parameters before visiting the body so that they are in scope when s is transformed for the second time. Similar things are being done there for other AST elements that can declare new parameters, such as in visitMethod, visitForLoop, and visitCatchStatement.

diff --git a/src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java b/src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java
index 8893207..59d8b81 100644
--- a/src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java
+++ b/src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java
@@ -1,6 +1,7 @@
 package org.kohsuke.groovy.sandbox;
 
 import org.codehaus.groovy.ast.ClassCodeExpressionTransformer;
+import org.codehaus.groovy.ast.ClassHelper;
 import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.MethodNode;
 import org.codehaus.groovy.ast.Parameter;
@@ -154,6 +155,14 @@ abstract class ScopeTrackingClassCodeExpressionTransformer extends ClassCodeExpr
     @Override
     public void visitClosureExpression(ClosureExpression expression) {
         try (StackVariableSet scope = new StackVariableSet(this)) {
+            Parameter[] parameters = expression.getParameters();
+            if (parameters != null && parameters.length > 0) {
+                for (Parameter p : parameters) {
+                    declareVariable(p);
+                }
+            } else {
+                declareVariable(new Parameter(ClassHelper.DYNAMIC_TYPE, "it"));
+            }
             super.visitClosureExpression(expression);
         }
     }
diff --git a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java b/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java
index 9cf1de8..5f030da 100644
--- a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java
+++ b/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java
@@ -336,4 +336,21 @@ public class SandboxTransformerTest {
                 "System:getProperties()");
     }
 
+    @Test public void closureVariables() throws Exception {
+        isolate(() -> assertIntercept(
+                "while ([false].stream().noneMatch({s -> s})) {\n" +
+                "    return true\n" +
+                "}\n" +
+                "return false\n",
+                true,
+                "ArrayList.stream()", "ReferencePipeline$Head.noneMatch(Script1$_run_closure1)"));
+        isolate(() -> assertIntercept(
+                "while ([false].stream().noneMatch({it})) {\n" +
+                "    return true\n" +
+                "}\n" +
+                "return false\n",
+                true,
+                "ArrayList.stream()", "ReferencePipeline$Head.noneMatch(Script2$_run_closure1)"));
+    }
+
 }

[robinfriedli]

Has this been forgotten? Or waiting for approval?

[ashenp]

Hi:
Is any realeased version which has solved this bug?

[GeoPakas]

Any resolution for this bug nowadays?