On Wed, 9 Dec 2020 06:40:25 GMT, Serguei Spitsyn <sspit...@openjdk.org> 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