On Wed, 9 Dec 2020 06:40:25 GMT, Serguei Spitsyn <[email protected]> wrote:
>> src/java.instrument/share/classes/sun/instrument/InstrumentationImpl.java
>> line 501:
>>
>>> 499: String msg = "method " + classname + "." + methodname + "
>>> must be declared public";
>>> 500: throw new IllegalAccessException(msg);
>>> 501: }
>>
>> If canAccess fails then it means that javaAgentClass is not public or the
>> premain method is not public. The invoke will fail but I agree eat explicit
>> canAccess check means we get finer control on the exception message.
>>
>> (I can't help feeling that we should do a bit more cleanup here and not use
>> getDeclaredMethod or be concerned with inheritance. This is because the
>> Premain-Class is meant to specify the agent class. Also can't have a public
>> static premain in super class and a non-public static premain in a
>> sub-class).
>
> My gut feeling is that it should be possible to get rid of the
> getDeclaredMethod calls with the reasoning you provided above. The canAccess
> check is not really needed in such a case as the getMethod returns public
> methods only (is my understanding correct?). This would simplify the
> implementation. Two disadvantages of this approach are:
> - less fine control on the exception message: NoSuchMethodException instead
> of IllegalAccessException for non-public premain methods
> - some compatibility risk is involved (most of agents define premain method
> correctly, so the only java agents that define premain methods with unusual
> tricks are impacted)
If the java agent class declares a non-public premain method but its
superclass declares a public premain method, what should be the expected
behavior? The proposed fix will choose the java agent class's non-public
premain and therefore it will fail to load the agent class with IAE.
If we get rid of the `getDeclaredMethod` call and find premain using
`getMethod`, it will choose the public premain method defined directly in the
java agent class or indirectly in its superclass.
As Alan stated, `Premain-Class` attribute is meant to specify the agent class.
Also specified in the package spec of `java.lang.instrument', the agent class
must implement a public static premain method similar in principle to the main
application entry point. Also under "Manifest Attributes" section,
Premain-Class
When an agent is specified at JVM launch time this attribute specifies
the agent class. That is, the class containing the premain method.
It expects that there is a public premain method declared in the agent class,
not in its superclass.
So I think it's best to fix this as well in this PR by dropping line 469-495
(i.e. keep `getDeclaredMethod` case and not search for inherited premain).
Throw if the premain method is not found or it is not accessible. This needs
to update the CSR for behavioral change.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1694
