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

[serguei.spitsyn at oracle.com]

Hi David,


On 6/23/20 22:50, David Holmes wrote:
> Hi Serguei,
>
> On 24/06/2020 3:37 pm, serguei.spitsyn at oracle.com wrote:
>> Hi Larry,
>>
>> Thank you for looking at this!
>>
>>
>> On 6/23/20 21:32, Laurence Cable wrote:
>>> should we not consider some form of depreciation here, and continue 
>>> to support non-public pre-main invocation for some time while 
>>> issuing a warning???
>>
>> I'm not sure what form of deprecation we can use as it has to be a 
>> deprecation of a spec non-compliant implementation. :)
>
> There's obviously no TCK test for this. :)
>
> You could just issue a warning if the premain is not public and say 
> this will be disallowed in a future release; then disallow it in 17.
>
> But I'm not sure it's worth it.

Yes, it is not clear it is worth it.


>>>
>>> while we have a sample of agents that will not be affected there may 
>>> be some agent that will fail terminally with this change
>>
>> There is more important problem now.
>> A big number or j.l.instrument started to fail with my fix with 
>> messages like this:
>>   Exception in thread "main" java.lang.IllegalAccessException: class 
>> sun.instrument.InstrumentationImpl
>>   (in module java.instrument) cannot access a member of class 
>> SimpleAgent with modifiers "public static"
>
> It sounds like the use of setAccessible was hiding the need to disable 
> some module related access checks.

It sounds so.
In this particular case, the setAccessible was used just to disable some 
module related access checks.
A side affect is that non-public premain methods became allowed as well.

>
> This will have a much bigger compatibility problem if agents with a 
> public premain suddenly stop working.

One approach would be to continue using the setAccessible and add extra 
check for non-public premain method.
Something like should probably work:
         if (!(Modifier.isPublic(m.getModifiers())) {
             throw new IllegalAccessException("premain method is not 
public");
         }

Thanks,
Serguei

>
> David
> -----
>
>> I'm not sure if there can be a version of the 
>> Method.setAccessible(boolean flag) api that works for public methods 
>> only.
>> One alternate approach is to relax the current spec to allow premain 
>> methods to be non-public.
>>
>> Thanks,
>> Serguei
>>
>>
>>>
>>> just a thought
>>>
>>> - Larry
>>>
>>> On 6/23/20 8:42 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Mandy,
>>>>
>>>> Thank you for looking at this!
>>>>
>>>>
>>>> On 6/23/20 20:21, Mandy Chung wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> I'm glad that you have a patch for this.
>>>>>
>>>>> On 6/23/20 7:05 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Please, review a fix for:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8165276
>>>>>>
>>>>>>
>>>>>> CSR draft (one CSR reviewer is needed before finalizing it):
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8248189
>>>>>>
>>>>>
>>>>> The compatibility risk should be low (rather than minimal).
>>>>
>>>> I was not sure if it has to be minimal or low.
>>>> Made it low now.
>>>>
>>>>
>>>>> It says "All known Java agents define the premain method as 
>>>>> public". It'd be useful to add a comment in the JBS issue to list 
>>>>> the Java agents you have checked.
>>>>
>>>> I'm relying on the Alan's comments posted in the bug report:
>>>>  "I checked a number of popular java agents and their premain 
>>>> methods are public, I haven't found any where the premain was not 
>>>> public."
>>>>  "I think we should just bite the bullet on this so that the 
>>>> premain must be public as originally intended."
>>>>
>>>> Probably, my statement in the CSR is too strong.
>>>> I've changed it to:
>>>>  "No popular Java agent that defines the premain method as a 
>>>> non-public was found."
>>>>
>>>> Does it looks better or you think we have to investigate existing 
>>>> popular Java agents?
>>>>
>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.1/ 
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Looks okay.  Can you add a test to verify this fix?
>>>>
>>>> Yes, I can add a test but it will be trivial.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>>>
>>>>> Mandy
>>>>
>>>
>>