On Wed, 16 Dec 2020 00:50:04 GMT, David Holmes <dhol...@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
