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

[David Holmes]

On 24/06/2020 4:24 pm, serguei.spitsyn at oracle.com wrote:
> 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.

The other way around. The setAccessible has been there long before the 
module system existed, to allow a non-public premain. As a side-effect 
when the module system came along it also disabled some module access 
check (I'm not sure exactly what).

>>
>> 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");
>          }

Yes an explicit modifier check would seem best.

Thanks,
David
-----

> 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
>>>>>
>>>>
>>>
>