[jira] [Commented] (GROOVY-12440) closure on the right-hand side of a property assignment inside a closure corrupts NestMembers causing IllegalAccessError

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GROOVY-12440?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18125074#comment-18125074 ] 

ASF GitHub Bot commented on GROOVY-12440:
-----------------------------------------

paulk-asert opened a new pull request, #2959:
URL: https://github.com/apache/groovy/pull/2959

   … inside a closure corrupts NestMembers causing IllegalAccessError




>  closure on the right-hand side of a property assignment inside a closure corrupts NestMembers causing IllegalAccessError
> -------------------------------------------------------------------------------------------------------------------------
>
>                 Key: GROOVY-12440
>                 URL: https://issues.apache.org/jira/browse/GROOVY-12440
>             Project: Groovy
>          Issue Type: Bug
>          Components: class generator, Static compilation
>    Affects Versions: 5.0.0
>            Reporter: James Daugherty
>            Assignee: Paul King
>            Priority: Major
>
> *Affects:* every 5.x release tested, 5.0.0 through 5.1.3. 
> *Works in:* 4.0.33 (no NestMembers emitted) and 6.0.0. 
> *Related:* GROOVY-10687, GROOVY-11780.
> h3. Description
> Under {{{}@CompileStatic{}}}, a closure on the right-hand side of a property assignment made inside another closure is recorded twice in the enclosing class's {{NestMembers}} attribute. Two examples are shown below. One entry names the real class; the other names a class that is never written. Every nested closure compiled after it in the same class is then listed under the wrong name, one higher than the class actually written.
> {code:groovy}
> { -> holder.value = list.collect { it } }
> holder.with { value = list.collect { it } }
> {code}
> Those closures name the outer class as their {{{}NestHost{}}}, but the outer class doesn't list them, so the JVM rejects their direct calls to its private methods:
> {noformat}
> java.lang.IllegalAccessError: class Repro$_callPrivate_closure2$_closure4 tried to access private method
> 'int Repro.secret(java.lang.Integer)' ... (Type Repro$_callPrivate_closure2$_closure4 (loader: 'app') is not a
> nest member of type Repro (loader: 'app'): current type is not listed as a nest member)
> {noformat}
> The failing closure can be in any later method of the class, and the method containing the assignment never has to run. We hit this in a Grails 8 application (Groovy 5.1.3): a statically compiled service failed at startup because an unrelated method assigned a property inside a {{with}} block, with a {{collect}} closure on the right-hand side.
> h3. Reproducer
> Standalone, no build tool: [jdaugherty/groovy5-issue-nested-closures|https://github.com/jdaugherty/groovy5-issue-nested-closures]
> {code:bash}
> ./reproduce.sh 5.1.3   # fails
> ./reproduce.sh 6.0.0   # passes
> {code}
> {code:groovy}
> import groovy.transform.CompileStatic
> @CompileStatic
> class Repro {
>     static class Holder {
>         String value
>     }
>     // Never called. Compiling it is enough to corrupt the NestMembers attribute of Repro.
>     void assign(Holder holder) {
>         Closure assignment = { -> holder.value = ['a'].collect { it }.join(',') }
>         assignment.call()
>     }
>     List<Integer> callPrivate() {
>         Closure<List<Integer>> outer = { -> [1, 2].collect { Integer i -> secret(i) } }
>         outer.call()
>     }
>     private int secret(Integer i) {
>         i * 10
>     }
>     static void main(String[] args) {
>         println new Repro().callPrivate()
>     }
> }
> {code}
> *Expected:* prints {{{}[10, 20]{}}}, and {{NestMembers}} lists exactly the closure classes written.
> *Actual (5.1.3):* the {{IllegalAccessError}} above. Classes written versus the {{NestMembers}} attribute:
> {noformat}
> Classes written                          NestMembers of Repro
> Repro$_assign_closure1                   Repro$_assign_closure1
> Repro$_assign_closure1$_closure3         Repro$_assign_closure1$_closure3
>                                          Repro$_assign_closure1$_closure4         <- never written
> Repro$_callPrivate_closure2              Repro$_callPrivate_closure2
> Repro$_callPrivate_closure2$_closure4    Repro$_callPrivate_closure2$_closure5    <- written as ...$_closure4
> {noformat}
> h3. Scope
> Tested on 5.1.3, with the same {{callPrivate()}} method following each variant:
> {code:groovy}
> // Fails: whether or not the assignment is the closure's last statement.
> // Each assignment adds one phantom entry; two assignments shift later names by two.
> { -> holder.value = list.collect { it } }
> holder.with { value = list.collect { it } }
> holder.with { it.value = list.collect { it } }
> // Passes
> { -> holder.setValue(list.collect { it }) }      // explicit setter call
> { -> localVariable = list.collect { it } }       // local variable
> { -> fieldOfThisClass = list.collect { it } }    // field of the enclosing class
> holder.value = list.collect { it }               // same assignment directly in the method
> { -> holder.value = 'x' }                        // no closure on the right-hand side
> {code}
> The failing source compiled without {{@CompileStatic}} passes and produces a correct {{NestMembers}} attribute. Reproduced on JDK 21.0.12 and 25.0.4.
> h3. Analysis
> {{AsmClassGenerator.moreNestmateEntries}} doesn't record the closure classes that are actually generated. It predicts the names of closures nested in closures by walking each closure's {{doCall}} body with a {{CodeVisitorSupport}} and incrementing a counter for each {{ClosureExpression}} it meets. The prediction holds only if the walk visits each {{ClosureExpression}} exactly once.
> For an assignment whose value is kept, static compilation rewrites the property assignment into the following setter form:
> {code:groovy}
> def tmp = val; a.setX(tmp); tmp
> {code}
> {{StaticPropertyAccessHelper.transformToSetterCall}} builds that as {{{}PoppingListOfExpressionsExpression(tmp, setX(tmp)){}}}, so the same {{TemporaryVariableExpression}} appears twice in the tree. {{TemporaryVariableExpression.visit}} evaluates the wrapped expression only once when the visitor is an {{{}AsmClassGenerator{}}}. For any other visitor it delegates straight to the wrapped expression. The nest-member walker therefore counts the right-hand-side closure twice, while code generation writes it once.
> GROOVY-11780 (backported to 5.0.x) doesn't cover this case. {{moreNestmateEntries}} and {{TemporaryVariableExpression}} look unchanged on master, so we haven't identified the change that makes 6.0.0 pass. Possible fixes:
>  * Emit {{NestMembers}} from the closure classes actually generated rather than predicted names.
>  * Have the walker skip {{ClosureExpression}} nodes it has already counted (identity-based).
> h3. Workaround
> Compute the value outside the closure, or call the setter explicitly:
> {code:groovy}
> String joined = ['a'].collect { it }.join(',')
> holder.with { value = joined }
> holder.with { setValue(['a'].collect { it }.join(',')) }
> {code}



--
This message was sent by Atlassian Jira
(v8.20.10#820010)