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

[David Holmes]

On Wed, 16 Dec 2020 00:15:36 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Alan, David and Many, thank you for the comments!
>> I'll prepare an update according to the recent requests from you.
>> One question is if I need to clone this PR to the JDK 16 fork:
>>   https://github.com/openjdk/jdk16
>> It depends on our chances to finalize this before RDP2.
>> An alternate approach would be to continue reviewing this PR until all comment are resolved and then clone it to jdk16.
>
> 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.

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

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