On Wed, 16 Dec 2020 02:23:33 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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?
>
> Thanks, David!
> Yes, I was also thinking that the setAccessible has to be remained after all
> the checks.
I've pushed an update with the following changes:
- InstrumentationImpl.java update with the process sequence suggested by David.
More specifically: the premain/agentmain method public modifier is checked
instead of m.canAccess, after that if the agent class is not public then the
setAccessible is called to make the method invokeable;
- The tests are refactored (tried to implement suggestion from Mandy to
simplify the tests). It includes:
- new helper class are added to build agent jar file:
`test/jdk/java/lang/instrument/AgentJarBuilder.java`
- new helper class to run negative tests which are expected to throw an
exception: `test/jdk/java/lang/instrument/NegativeAgentRunner.java`
- introduced in the first push new shell script is removed:
`test/jdk/java/lang/instrument/PremainClass/MakeJAR.sh`
- the test runners with names *Test.java are removed, all the tests use
jtreg commands instead (negative tests now use the NegativeAgentRunner)
- the test `test/jdk/java/lang/instrument/NonPublicPremainAgent.java` is
moved to the PremainClass sub-folder
It might make sense to remove previously added public modifiers to several
agent classes. However, I've decided to skip it for now. Please, let me know if
you think it is desired thing to do.
Mandy also suggested to consider using the exception message format produced by
thrown by `jdk.internal.reflect.Reflection::newIllegalAccessException`. It
looks like this: `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"`.
Please, let me know if this format would be better.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1694
