-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs #1694
Conversation
…ass if it's public but implementation differs
👋 Welcome back sspitsyn! A progress list of the required criteria for merging this PR into |
Webrevs
|
/label remove core-libs |
@AlanBateman |
if (!m.canAccess(null)) { | ||
String msg = "method " + classname + "." + methodname + " must be declared public"; | ||
throw new IllegalAccessException(msg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Can you provide a link to the discussion? I'm mostly curious if there was some discussion as to why Instrument purposefully allowed non-public premain methods:
|
All the discussion is in the bug and CSR: |
Chris, I've added link to the jdk 15 review thread to the PR description. |
Alan, Mandy and Chris, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update @bug in the tests to include this bug ID.
All InheritAgentXXX tests are updated to have the InheritAgentXXX class public. However, I think you need to go through them individually for this behavioral change. The existing comments may not be applicable and please update them where appropriate. For example InheritAgent0100 previously verifies the invocation of the premain in its superclass. Does the modified test ensure that this fails to load? Per the comment in the new InheritAgent0100Super class, it expects the superclass' premain should be called.
// finally try the inherited 1-arg method | ||
try { | ||
m = javaAgentClass.getMethod(methodname, | ||
new Class<?>[] { String.class }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments in line 429-443 need to be updated.
Mandy, thank you for comments. |
… in tests with new expectations that premain methods in supers should non be called
I've updated the @bug in the tests to include this bug ID. |
if (!m.canAccess(null)) { | ||
String msg = "method " + classname + "." + methodname + " must be declared public"; | ||
throw new IllegalAccessException(msg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the updated patch looks much better.
If the agent class doesn't declare a premain then NoSuchMethodException will be thrown.
The only thing that I'm wondering about now is the exception message when the agent class is not public. If I read the changes correctly it means that IllegalAccessException will be thrown saying that .premain should be public. Is that correct? We might need to adjust to the exception message to make it clear, or put in an explicit then to ensure to test that the agent class is public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an explicit check for a public premain method is better to disambiguate the cases and provide appropriate error messages.
if (!m.canAccess(null)) { | ||
String msg = "method " + classname + "." + methodname + " must be declared public"; | ||
throw new IllegalAccessException(msg); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an explicit check for a public premain method is better to disambiguate the cases and provide appropriate error messages.
Alan, David and Many, thank you for the comments! |
Mailing list message from Mandy Chung on serviceability-dev: On 12/14/20 2:47 PM, Serguei Spitsyn wrote:
Since this already passes RDP1, this can be retarget to JDK 17 and Mandy |
Mandy, thank you for the suggestion. 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: With the m.canAccess check the following message is provided (it looks right): Without this check the message was (not sure, it is good enough): 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): Without this check the message is (it looks pretty confusing): So, it seems we also need an explicit check for agent class being public with a right message provided.
With the check above the message is: 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. But the
It seems, the IAE is thrown because the agent class is not public. |
Mandy, I've pushed updates with changes:
The following patch can be committed if you like as it does not harm:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sspitsyn Thanks for the update. I think it's good to add the unnamed module check before calling setAccessible
that becomes clear that this check only intends for unnamed module. Maybe make it clear in the comment something like this:
// if the java agent class is in an unnamed module, the java agent class can be non-public.
// suppress access check upon the invocation of the premain/agentmain method
Since the agent class can be non-public, it means that it's not necessary to modify the test agent class to public as you did in this patch.
I don't think @library /test
is needed, isn't it? The test library classes are under test/lib.
// The agent class must implement a public static premain method... | ||
setAccessible(m, true); | ||
// reject non-public premain or agentmain method | ||
if ((m.getModifiers() & java.lang.reflect.Modifier.PUBLIC) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative way: if (Modifier.isPublic(m.getModifiers()))
Similar can be applied for checking if the javaagentClass is public.
…dule; 2. minor tests cleanup
@mlchung Thank you for the suggestions. I've addressed them in the latest update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks okay with minor comments on the tests. Nit: it will be good to update @summary
in the InheritAgentxxxx tests that now fail to load the agent.
/* | ||
* @test | ||
* @bug 8165276 | ||
* @summary Test that agent with non-public premain method is rejected to load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment is incorrect. This tests a non-public agent class with a public premain method. The agent is not rejected.
@@ -38,7 +38,7 @@ | |||
import java.io.*; | |||
import asmlib.*; | |||
|
|||
class RetransformAgent { | |||
public class RetransformAgent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making RetransformAgent as public is not necessary. This fix does not change this test and so better to revert it.
@@ -23,7 +23,7 @@ | |||
|
|||
import java.lang.instrument.Instrumentation; | |||
|
|||
class SimpleAgent { | |||
public class SimpleAgent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no other change in this test except making SImpleAgent as public which is not necessary. So better to revert it.
@sspitsyn This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 30 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@mlchung All suggestions are right and addressed in the latest update. |
@@ -23,7 +23,7 @@ | |||
|
|||
/** | |||
* @test | |||
* @bug 6274264 6274241 5070281 | |||
* @bug 6274264 6274241 5070281 8165276 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright header and @bug
change should be reverted.
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved. | |||
* Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright header change should be reverted.
…in tests summary comment
@mlchung |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the change. Looks okay.
Thank you a lot, Mandy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for persevering on this one. The updated checks look good. At some point I think we re-examine the issue of allowing agents to be non-public, maybe emit a warning and eventually disallow it. If/when support is added for developing and deployed java agents as modules it might be the time to re-examine it.
Thank you for review, Alan! |
@sspitsyn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
This PR has been approved by Mandy and Alan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Serguei,
This seems good to go.
Thanks,
David
Thank you for review, David! |
/integrate |
@sspitsyn Since your change was applied there have been 48 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit c538cd8. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
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.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1694/head:pull/1694
$ git checkout pull/1694