New wevrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.2/
Now the InstrumentationImpl.java has this new check to throw IAE with
the meaningful error message:
+ // reject non-public premain or agentmain method
+ if (!m.canAccess(null)) {
+ String msg = "method " + classname + "." + methodname + " must be
declared public";
+ throw new IllegalAccessException(msg);
+ }
It also includes a new negative test for non-public premain method:
test/jdk/java/lang/instrument/NonPublicPremainAgent.java
I've tested the non-public agentmain as well with one of the hacked
JVMTI aod tests.
But I gave up to make it a stand alone test as this testing framework
is tricky to use for negative testing.
The implementation is common for premain and agentmain cases, so
probably, one test
Also, I had to fix all impacted java/lang/instrument tests to make
the Agent classes public.
The following tests required a refactoring:
|| test/jdk/java/lang/instrument/PremainClass/InheritAgent0100.java
test/jdk/java/lang/instrument/PremainClass/InheritAgent1000.java
test/jdk/java/lang/instrument/PremainClass/InheritAgent1100.java
They define an agent as extending an agent super class which has the
premain methods defined:
37 class InheritAgent0101 extends InheritAgent0101Super {
38
39 //
40 // This agent has a single argument premain() method which
41 // is the one that should be called.
42 //
43 public static void premain (String agentArgs) {
44 System.out.println("Hello from Single-Arg
InheritAgent0101!");
45 }
46
47 // This agent does NOT have a double argument premain()
method.
48 }
49
50 class InheritAgent0101Super {
51
52 //
53 // This agent has a single argument premain() method which
54 // is NOT the one that should be called.
55 //
56 public static void premain (String agentArgs) {
57 System.out.println("Hello from Single-Arg
InheritAgent0101Super!");
58 throw new Error("ERROR: THIS AGENT SHOULD NOT HAVE BEEN
CALLED.");
59 }
60
61 // This agent does NOT have a double argument premain()
method.
62 }
Above, just one class can be made public.
But the InheritAgent0101Super has to be public as well as has the
premain method defined.
These agent super classes are separated to their own files.
To make this refactoring to work new || customized script is introduced:
test/jdk/java/lang/instrument/PremainClass/MakeJAR.sh
The java/lang/instrument tests are passed locally.
I'll submit a mach5 test jobs to make sure nothing is broken.
Thanks,
Serguei
On 6/24/20 13:07, serguei.spit...@oracle.com wrote:
On 6/24/20 12:44, Mandy Chung wrote:
On 6/24/20 12:26 PM, serguei.spit...@oracle.com wrote:
On 6/24/20 05:25, David Holmes wrote:
Ah! The test class SimpleAgent is what is not public. That seems
a bug in the test.
There are many such tests.
We can break some of the existing agents by rejecting non-public
agent classes.
I'm inclined to continue using the setAccessible and just add an
extra check for non-public premain/agentmain methods.
There is only one non-public SimpleAgent which is shared by
j.l.instrument tests.
test/jdk/java/lang/instrument/SimpleAgent.java
There are many such tests:
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/TestLambdaFormRetransformation.java:class
Agent implements ClassFileTransformer {
test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java:class
NativeMethodPrefixAgent {
test/jdk/java/lang/instrument/PremainClass/NoPremainAgent.java:class
NoPremainAgent {
test/jdk/java/lang/instrument/SimpleAgent.java:class SimpleAgent {
test/jdk/java/lang/instrument/RetransformAgent.java:class
RetransformAgent {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0001.java:class
InheritAgent0001 extends InheritAgent0001Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0001.java:class
InheritAgent0001Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0010.java:class
InheritAgent0010 extends InheritAgent0010Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0010.java:class
InheritAgent0010Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0011.java:class
InheritAgent0011 extends InheritAgent0011Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0011.java:class
InheritAgent0011Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0100.java:class
InheritAgent0100 extends InheritAgent0100Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0100.java:class
InheritAgent0100Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0101.java:class
InheritAgent0101 extends InheritAgent0101Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0101.java:class
InheritAgent0101Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0110.java:class
InheritAgent0110 extends InheritAgent0110Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0110.java:class
InheritAgent0110Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0111.java:class
InheritAgent0111 extends InheritAgent0111Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent0111.java:class
InheritAgent0111Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1000.java:class
InheritAgent1000 extends InheritAgent1000Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1000.java:class
InheritAgent1000Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1001.java:class
InheritAgent1001 extends InheritAgent1001Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1001.java:class
InheritAgent1001Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1010.java:class
InheritAgent1010 extends InheritAgent1010Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1010.java:class
InheritAgent1010Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1011.java:class
InheritAgent1011 extends InheritAgent1011Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1011.java:class
InheritAgent1011Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1100.java:class
InheritAgent1100 extends InheritAgent1100Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1100.java:class
InheritAgent1100Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1101.java:class
InheritAgent1101 extends InheritAgent1101Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1101.java:class
InheritAgent1101Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1110.java:class
InheritAgent1110 extends InheritAgent1110Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1110.java:class
InheritAgent1110Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1111.java:class
InheritAgent1111 extends InheritAgent1111Super {
test/jdk/java/lang/instrument/PremainClass/InheritAgent1111.java:class
InheritAgent1111Super {
But is is not a big problem - all can be fixed.
test/hotspot/jtreg/runtime/cds/appcds/jvmti/dumpingWithAgent
implements the agent properly (a public class and a public static
void premain method).
As the popular Java agents are conforming the spec (publicly
accessible premain method), the compatibility risk is low.
Unless such a java agent exists and finds a strong compelling
reason to argue that its premain method must be allowed non-public,
I do not see the argument to change the spec to allow non-public
agent classes.
A bad test case is not a representative existing java agent.
Okay, thanks.
I'll prepare a fix with a removed setAccessible.
Thanks,
Serguei
Mandy
