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 17:44:38 UTC 2020

Hi David,

Thank you a lot for review and tweaking the bug title.
I've re-targeted this to 16 as was suggested by Joe.

Thanks,
Serguei

On 6/28/20 19:37, David Holmes wrote:
> 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
>>>
>>