RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]

[Serguei Spitsyn]

On Wed, 16 Dec 2020 00:50:04 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Mandy, thank you for the suggestion.
>> I'll retarget the bug and CSR to jdk 17 as nobody is objecting.
>> 
>> Also, wanted to make it clear about Exception messages that are provided for two different cases.
>> 
>> The test java/lang/instrument/NonPublicPremainAgent.java defines a non-public premain method:
>> `    static void premain(String agentArgs, Instrumentation inst) {`
>> 
>> With the m.canAccess check the following message is provided (it looks right):
>> `  Exception in thread "main" java.lang.IllegalAccessException: method NonPublicPremainAgent.premain must be declared public `
>> 
>> Without this check the message was (not sure, it is good enough):
>> `  Exception in thread "main" java.lang.IllegalAccessException: class sun.instrument.InstrumentationImpl (in module java.instrument) cannot access a member of class NonPublicPremainAgent with modifiers "static"`
>> 
>> Also, I've added a new test java/lang/instrument/NonPublicAgent.java which defines a non-public agent class with a public premain method.
>> 
>> With the m.canAccess check the following message is provided (it does not look right):
>> `  Exception in thread "main" java.lang.IllegalAccessException: method NonPublicPremainAgent.premain must be declared public `
>> 
>> Without this check the message is (it looks pretty confusing):
>> `  Exception in thread "main" java.lang.IllegalAccessException: class sun.instrument.InstrumentationImpl (in module java.instrument) cannot access a member of class NonPublicAgent with modifiers "public static"`
>> 
>> So, it seems we also need an explicit check for agent class being public with a right message provided.
>> I've added the following check:
>> @@ -426,6 +426,12 @@ public class InstrumentationImpl implements Instrumentation {
>>          NoSuchMethodException firstExc = null;
>>          boolean twoArgAgent = false;
>>  
>> +        // reject non-public owner agent class of premain or agentmain method
>> +        if ((javaAgentClass.getModifiers() & java.lang.reflect.Modifier.PUBLIC) == 0) {
>> +            String msg = "agent class of method " + classname + "." +  methodname + " must be declared public";
>> +            throw new IllegalAccessException(msg);
>> +        }
>> +
>>          // The agent class must have a premain or agentmain method that
>>          // has 1 or 2 arguments. We check in the following order:
>>          //
>> 
>> With the check above the message is:
>> `  Exception in thread "main" java.lang.IllegalAccessException: agent class of method NonPublicAgent.premain must be declared public`
>> 
>> Please, let me know what you think.
>> 
>> I've done some tests refactoring motivated by some private requests from Mandy. Will publish the update as soon as it is ready. Hopefully, today.
>
> The agent class doesn't have to be public it just has to be accessible.
> 
> The premain method should be queried for public modifier rather than just relying on a failed invocation request.

David, thank you for catching this. I'm probably missing something here.
If the agent class is not public then the `m.canAccess(null)` check is not passed and IAE is thrown with the message:
  `Exception in thread "main" java.lang.IllegalAccessException: method NonPublicAgent.premain must be declared public`

But the `NonPublicAgent.premain` is declared public as below:
    public static void premain(String agentArgs, Instrumentation inst) {
        System.out.println("premain: NonPublicAgent was loaded");
    }
It seems, the IAE is thrown because the agent class is not public.
Does it mean the `m.canAccess(null)` check is not fully correct?

-------------

PR: https://git.openjdk.java.net/jdk/pull/1694