On Tue, 8 Dec 2020 19:28:29 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> This change have been already reviewed by Mandy, Sundar, Alan and David. >> Please, see the jdk 15 review thread: >> >> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html >> >> Now, the PR approval is needed. >> The push was postponed because the CSR was not approved at that time (it is >> now): >> https://bugs.openjdk.java.net/browse/JDK-8248189 >> Investigation of existing popular java agents was requested by Joe. >> ---------- >> >> The java.lang.instrument spec: >> >> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html >> >> Summary: >> The java.lang.instrument spec clearly states: >> "The agent class must implement a public static premain method >> similar in principle to the main application entry point." >> Current implementation of sun/instrument/InstrumentationImpl.java >> allows the premain method be non-public which violates the spec. >> This fix aligns the implementation with the spec. >> >> Testing: >> A mach5 run of jdk_instrument tests is in progress. > > 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) ------------- PR: https://git.openjdk.java.net/jdk/pull/1694