Repository navigation
GROOVY-12569: SC: do not name a package-private class of another GroovyClassLoader compilation in bytecode - #3098
codeconsole wants to merge 27 commits into
Conversation
…h new directly only within the compilation unit Under static compilation, new A(...) of a protected or package-private constructor of a class with the same package name was compiled to NEW and INVOKESPECIAL. The JVM allows that call only within the same run-time package, which is the package name and the class loader, so it failed with IllegalAccessError when A came from another class loader: a class of an earlier parse of the same GroovyShell, which GroovyClassLoader defines with another InnerLoader, or a precompiled class used by a script. Dynamic and type-checked code create such an object through the meta-class and work. Such a constructor is now called directly only when its class is compiled in the same compilation unit, which defines all its classes with one class loader, as for a constructor reference. Otherwise the object is created through the meta-class, which selects the constructor at run time. This also applies to the no-argument constructor that a map-style construction such as new A(name: 'x') calls before it sets the properties. Public constructors, and constructors of classes of the same compilation unit, are still called directly. The constructor reference writer uses the same check.
… only within the compilation unit Under static compilation, a call to a protected or package-private method of a class with the same package name was compiled to a direct INVOKEVIRTUAL, INVOKESTATIC or INVOKESPECIAL. As for a constructor, the JVM allows that call only within the same run-time package, so it failed with IllegalAccessError when the class came from another class loader, for example a class of an earlier parse of the same GroovyShell. This also hit a protected or package-private getter or setter used as a property, such as a.name or a.name = value. Such a method is now called directly only when its class is compiled in the same compilation unit, or when the JVM allows the call from any package: a protected method called from a subclass on this, super or a receiver of the subclass type, or a protected static method called from a subclass. Otherwise the method is selected at run time through the meta-class, as dynamic code does. Public methods, and methods of classes of the same compilation unit, are still called directly.
…nother compilation unit through a synthetic method A statically compiled method reference such as m::pm, M::pm or M::spm gives LambdaMetafactory a method handle for the method. For a protected or package-private method of a class with the same package name, the JVM resolves that handle only within the same run-time package, so the reference failed with IllegalAccessError when the class came from another class loader, for example a class of an earlier parse of the same GroovyShell. For such a method of a class of another compilation unit, the reference now targets a synthetic static method of the current class that calls the method. The invocation writer compiles that call directly when the JVM allows it from any package (a protected method called through the subclass), or else selects the method at run time. Method references to methods of classes of the same compilation unit, and to public methods, still use a method handle for the method itself.
…ly within the compilation unit AsmClassGenerator.isFieldDirectlyAccessible decided by the package name if a protected or package-private field can be read or written with GETFIELD, PUTFIELD, GETSTATIC or PUTSTATIC. As for methods and constructors, the JVM allows that only within the same run-time package, so the access failed with IllegalAccessError when the class came from another class loader, for example a class of an earlier parse of the same GroovyShell. Under static compilation this hit a.field, a.@field, Type.field, an implicit field of a superclass, compound assignments and increments. Dynamic code hit it for super.field, super.@field and this.@field of a superclass. A protected field of a superclass with the same package name but of another compilation unit was also read through a receiver of the superclass type, which fails verification ("Bad access to protected data in getfield") when the classes are in different run-time packages. Such a field is now accessed directly only when its class is compiled in the same compilation unit, or through the subclass rule for a protected field (with a receiver of the current class). Otherwise it is read and written through the meta-class, as dynamic code does. Public fields, and fields of classes of the same compilation unit, are still accessed directly.
…ckage instead of failing verification
Outside its package, a protected constructor can only be called by
super(...) or by creating an anonymous inner class (JLS 6.6.2.2). The type
checker accepts new A(...) of such a constructor in a subclass of A, as it
accepts other protected members there, and type-checked code creates the
object through the meta-class. Under static compilation, the call was
compiled to NEW and INVOKESPECIAL, and the class failed verification when
it was loaded: "VerifyError: Bad access to protected <init> method". The
same happened for a map-style construction such as new A([:]), which calls
the protected no-argument constructor.
The static compiler now reports "Cannot call protected constructor for A
from class B", as it reports a protected method called on a receiver that
is not the subclass (GROOVY-7325). super(...), this(...) and
new A(...) {} still call the constructor directly. A constructor reference
A::new to such a constructor still selects it at run time.
… through a superclass receiver Outside its package, a protected instance method or field can be used only through a receiver of the subclass (JLS 6.6.2.1, JVMS 4.10.1.8). For a method, the static compiler meant to report such a call (GROOVY-7325), but its check compared the types the wrong way round: it tested whether the current class is a subtype of the receiver type instead of the reverse. So a.m() with a receiver of the superclass type, in a subclass in another package, was compiled to a direct call and the class failed verification: "VerifyError: Bad access to protected data in invokevirtual". This also hit a protected getter or setter used as a property (a.name, a.name = value). A method reference such as a::m or A::m to such a method failed with BootstrapMethodError, and an attribute assignment a.@field = value failed verification in putfield. The check now requires the receiver to be of the current class (or this, super or a static method), so such a call or method reference is reported as "Cannot access method", as GROOVY-7325 intended and as javac does. An attribute assignment through such a receiver is now made through the meta-class, as the attribute read a.@field already was. Calls through this, super or a receiver of the subclass type are still direct.
…amic cannot access the constructor A dynamic new of a protected constructor of another package, in a subclass, is accepted by Groovy and by the type checker. With invokedynamic, the constructor selector unreflected the selected constructor with the lookup of the calling class and turned an IllegalAccessException into GroovyBugError. For a constructor that the JVM refuses to the caller, such as the protected ObjectInputStream() of the encapsulated java.io package called by new in a subclass, the call failed with "BUG! UNCAUGHT EXCEPTION: member is protected". As the property selectors do since GROOVY-12314, the constructor selector now treats the refusal as an access-control outcome and leaves the call to the meta-class, which reports "could not access constructor", as the call does without invokedynamic. A constructor that reflection can reach, such as a protected constructor of a class of another package in the unnamed module, is still called.
…ethod of another run-time package For a dynamic super.m() call, the class gets a synthetic super$N$m method that calls the superclass method with INVOKESPECIAL. For a package-private method of a superclass with the same package name but of another compilation unit, the classes may be in different run-time packages, for example when the superclass comes from an earlier parse of the same GroovyShell, and the JVM refuses the call: super.m() failed with IllegalAccessError. Such a method, and a package-private method of a superclass of another package, no longer gets a super$ method, and the meta-class calls the superclass method reflectively, as it does for a statically compiled super.m() of such a method.
…ugh the meta-class only for classes defined in memory The previous commits accessed a protected or package-private member of a class with the same package name through the meta-class whenever that class was compiled by another compilation unit. That also hit class files: a Java class of a joint compilation, a main class used by a test, a class left over from an incremental compilation. Such classes are loaded by one class loader at run time, as javac assumes, so the direct access worked; the meta-class path was slower, followed meta-class semantics, needed reflection metadata in a native image, and made an incremental build give other bytecode than a full one. The run-time package of such a class only differs when the classes are defined in memory by a GroovyClassLoader, which defines each compilation (a GroovyShell script, parseClass, JSR-223) with an InnerLoader of its own. GroovyClassLoader now records that on the compilation unit, and AsmClassGenerator.isInSameRuntimePackage treats a class of another compilation unit as another run-time package only then. Constructors, map-style construction, methods, getters and setters, method references, fields, dynamic super and this.@ access and super$ methods all use this check. Compiling to class files keeps direct access, as before these changes and as javac does. (The rest of the change: new, map-style construction, methods, getters and setters, method references to methods, fields, dynamic super and this.@ access and super$ methods. The GroovyClassLoader marker, isInSameRuntimePackage and constructor references come first.)
The tests for protected and package-private members of a class of another compilation now compile it through a GroovyClassLoader, which defines it with another class loader; classes compiled to class files keep direct access. Name the tests accordingly.
…rules of constructor references - isInSameRuntimePackage: isPrimaryClassNode() stands for "defined by the class loader of the accessing class", not for "compiled from source". - StaticInvocationWriter.isDirectlyAccessible: unlike isMemberDirectlyAccessible it has no subclass rule, since the verifier rejects NEW and INVOKESPECIAL of a protected constructor of another package in a subclass too. - findConstructorReferenceTarget: a selected constructor is called directly, so a meta-class constructor override is not used, as for a statically compiled new. The guard comment now says why enums are left to the meta-class: the type checker accepts an enum constructor reference whose arguments match the generated constructor, but Java code cannot instantiate an enum. - addSyntheticMethod: the ACC_FINAL rule of JVMS 4.6 applies to every helper method of a method reference in an interface. (The part on statically compiled new; the rest comes with constructor references.)
… interface All helper methods that a method reference adds to the current class are created by addSyntheticMethod, which leaves out ACC_FINAL in an interface. The interface tests covered the constructor-reference and variadic helpers; they now also cover the array constructor reference, the static extension method helper (Thread::startDaemon), the Class method helper (String::getName) and the helper that calls a package-private method of a class of another class loader (M::spm, M::pm), in static and default methods, and check that every helper is private and not final. With ACC_FINAL put back, both tests fail with ClassFormatError (illegal modifiers 0x101A). (The case of a method of another run-time package; the other helper methods are tested with constructor references.)
…AbstractBytecodeTestCase MethodReferenceTest and StaticCompileRuntimePackageTest each had a copy of the helper that compiles a script against the classes of an earlier parse of the same GroovyClassLoader, and of the code that keeps only the instructions of one method. AbstractBytecodeTestCase now provides them as compileAgainstLibrary, extractMethodSequence and selectMethod, and both tests use them. Test code only; the tests are unchanged. (Here StaticCompileRuntimePackageTest; MethodReferenceTest was moved to the helpers with constructor references.)
…errors COMPATIBILITY.md describes how statically compiled code of a compilation that a GroovyClassLoader defines in memory reaches protected and package-private members of classes of other compilations, and the new compile errors for new of a protected constructor of another package and for a protected method of another package called through a superclass receiver. StaticInvocationWriter notes that the constructor error is reported during class generation, so @TypeChecked code is not checked for it and works dynamically.
…ass of another class loader Since GROOVY-12454, new A(map) with a Map variable, under static compilation, calls the no-argument constructor and then sets the properties of the map at run time. For a class of an earlier parse of the same GroovyShell whose no-argument constructor is package-private, the constructor is called through the meta-class, as for a map literal.
…s loader in bytecode A class that is not public is accessible only within its run-time package (JVMS 5.4.4), and the JVM checks that whenever an instruction names the class: NEW, CHECKCAST, INSTANCEOF, the owner of a method or field, and the classes of a method handle or invokedynamic descriptor. Dynamic code uses a package-private class of an earlier parse of the same GroovyShell, or a precompiled class used by a script, without naming it, so new Hidden(), Hidden.sm(), Hidden.FIELD, def h = new Hidden(); h.run(); h.name and Runnable r = new Hidden() all work. Statically compiled code named the class in each of them and failed with IllegalAccessError. For a class that AsmClassGenerator.isClassDirectlyAccessible rejects (not public, same package name, but another run-time package, which is only known for classes defined in memory by a GroovyClassLoader), the static writers now avoid naming it: - new Hidden(...), also map-style, creates the object through the meta-class on the Class object, which is loaded by name; - a static method, property or field of the class is used through the meta-class; - a value of the class is typed for bytecode as its nearest superclass that can be named (or Object): local variables declared with def, method results, field values, the elements of a spread or a list property, so storing, passing or assigning it to an interface type needs no cast to the class; - a method of the class is called directly through a public superclass or interface that declares it, otherwise through the meta-class, and a method with a parameter of the class through the meta-class; - a method pointer keeps the class for method selection, and a method reference that would need a method handle naming the class is a method closure, as in dynamic code. What names the class in the source still names it in bytecode and fails as in dynamic code: a variable declared with the class, a cast to it and instanceof. Classes compiled to class files, classes of the same compilation and public classes are named directly, as before.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3098 +/- ##
==================================================
+ Coverage 74.1563% 74.1958% +0.0395%
- Complexity 43738 43861 +123
==================================================
Files 1692 1692
Lines 149549 149720 +171
Branches 28371 28445 +74
==================================================
+ Hits 110900 111086 +186
+ Misses 28437 28413 -24
- Partials 10212 10221 +9
🚀 New features to boost your workflow:
|
daniellansun
left a comment
There was a problem hiding this comment.
GROOVY-12569 is the right companion to #3097: even when the member is public, bytecode must not name a non-public class of another run-time package (NEW / CHECKCAST / owner type).
isClassDirectlyAccessible + getDirectlyAccessibleSuperClass, rewriting ctor / method / field / method-pointer / type-chooser paths to a namable superclass (or meta-class construction), and the Hidden usage matrix in StaticCompileRuntimePackageTest look solid. Map-style construction via DYNAMIC_RESOLUTION is covered.
Blocking for approve right now:
- CI:
lts (17, windows-latest, 1)60m timeout — please re-run. - Codecov patch ~79% with 53 uncovered/partial lines across
StaticTypesMethodReferenceExpressionWriter,StaticInvocationWriter,StaticTypesCallSiteWriter,AsmClassGenerator,StaticTypesTypeChooser,MethodPointerExpressionWriter. That is a lot of new control flow for a class-naming edge; please add cases that hit the missing branches (or point at existing tests Codecov missed).
Depends on #3097 tip; land after #3097. Sibling of #3099 (12570), not a linear stack.
…ed through public classes
|
# Conflicts: # src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesMethodReferenceExpressionWriter.java
…to the language rule For a public or private field the language rule of isMemberDirectlyAccessible decides. For a protected or package-private field, JVMS 5.4.4 allows access from the run-time package of the declaring class, or to a protected field from a subclass. That condition implies isMemberDirectlyAccessible, so it replaces it there instead of being checked after it.
# Conflicts: # src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
…class # Conflicts: # COMPATIBILITY.md
…class # Conflicts: # COMPATIBILITY.md # src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesMethodReferenceExpressionWriter.java
✅ All tests passed ✅🏷️ Commit: dff8b88 Learn more about TestLens at testlens.app/docs. |
A class that is not public is accessible only within its run-time package, and the JVM checks that wherever an instruction names the class. Dynamic code uses a package-private class of an earlier
GroovyShellparse, or of a library used by a script, without naming it. Statically compiled code named it and failed withIllegalAccessError; it now does what dynamic code does, for compilations that aGroovyClassLoaderdefines in memory:new, also map-style with a literal or aMapvariable, creates the object through the meta-class; a static member of the class is used through the meta-class; a method of the class is called through a public superclass or interface that declares it, or else through the meta-class; a value of the class is held as its nearest public superclass, so storing, passing or assigning it to an interface type needs no cast to the class. A method pointer keeps the class for method selection, and a method reference that would need a method handle naming the class is a method closure, as in dynamic code. A variable declared with the class, a cast to it andinstanceofstill fail, as they do in dynamic code. Classes compiled to class files, classes of the same compilation and public classes are named directly, as before.This uses the run-time package rule of #3097 (
AsmClassGenerator.isInSameRuntimePackage, the dynamic constructor call inStaticInvocationWriter), and includes its commits until that is merged.Tests
StaticCompileRuntimePackageTest, marked// GROOVY-12569: each use of a package-private class of another class loader, from aGroovyShellparse and from a library defined by another class loader, compared with the same code compiled dynamically; map-style construction; the uses that name the class fail as in dynamic code; bytecode tests pin that no instruction names the class, and that the class is named directly within the same compilation and in class files.The
org.codehaus.groovy.classgen,org.codehaus.groovy.transformandgroovy.transform.stcpackages andbugstests pass (6,387 tests);GenericsStaticCompileTest.testSelfReferentialTypeParameter5failed once on the@Grabrace of GROOVY-12527 and passes when rerun. The Checkstyle and CodeNarc gates pass.