jenkinsci / groovy-sandbox

(Deprecated) Compile-time transformer to run Groovy code in a restrictive sandbox
MIT License
122 stars 60 forks source link

Prefix/postfix increment operators broken within closures #30

Closed sdudley closed 4 years ago

sdudley commented 8 years ago

When trying to use a script containing a prefix/postfix operator within a closure, such as this:

def myClosure = {
    def i=0
    ++i
}
myClosure()

I get the following error when invoking the closure while the sandbox is in use:

Caused by: groovy.lang.MissingMethodException: No signature of method: Script4.next() is applicable for argument types: () values: []

Disabling the sandbox works fine, as does using the same statement outside of a closure:

def i=0
++i

This is broken in 1.10 but it was previously working in 1.7. Bisecting the code shows that the problem was introduced in eec9c437f2 (#16). On a casual review, that commit effectively transforms expressions such as "++i" into "i.next()". Curiously, replacing the "++i" above with "i = i.next()" works just fine.

sdudley commented 8 years ago

I've added my attempted fix for the problem as a pull request, but it could definitely use some review. I added test cases to confirm that this specific problem is solved, but I am not necessarily confident that I have handled all of the edge cases (for example, that incrementing or decrementing other types of objects will always be handled correctly).

I arrived at this particular solution after looking at the difference in the MethodCallExpressions constructed by "i.next()" (which worked) and "++i" (which failed), and by realizing that the former had implicitThis=false while the other had implicitThis=true.

dwnusbaum commented 4 years ago

I tried to reproduce this today, and could not. The following test passes when added to SandboxTransformerTest:

@Test public void prefixOpInClosure() throws Exception {
    assertIntercept(
            "def myClosure = {\n" +
            "    def i=0\n" +
            "    ++i\n" +
            "}\n" +
            "myClosure()",
            1,
            "Script1$_run_closure1.call()", "Integer.next()");
}

Maybe it was accidentally fixed by 18c9f7457bac9db6dbb555ca3e0328decc5a5c72? I am going to go ahead and close this issue, but feel free to reopen it if I am mistaken.