15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs

Mon Jun 29 02:37:58 UTC 2020

Hi Serguei,

These changes look good to me.

Note that I tweaked the bug synopsis to make it slightly more 
grammatically correct: that invoke -> to invoke

Thanks,
David

On 26/06/2020 2:17 am, serguei.spitsyn at oracle.com wrote:
> 
> 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.spitsyn at oracle.com wrote:
>> On 6/24/20 12:44, Mandy Chung wrote:
>>>
>>>
>>> On 6/24/20 12:26 PM, serguei.spitsyn at 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
>>
> 