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
JDK-8268088: Clarify Method::clear_jmethod_ids() related comments in ClassLoaderData::~ClassLoaderData() #4383
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we have any use cases of what a real world application can do with a NULL'ed out jmethodID (other than JVM TI will give an JVMTI_ERROR_INVALID_METHODID for it)?
All or most of the JNI functions will crash with a NULL'ed out jmethodID. You can catch this error by using -Xcheck:jni. Maybe we could deprecate this feature of the JVMTI spec and release the memory?
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.
This comment looks like an expanded version of the one I wrote a long time ago and correct. We were wondering if this is a feature that we actually need.
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 JVMTI agent use case that ran into issues (with releasing memory) calls GetMethodDeclaringClass for a stale jmethod_id (with unloaded class and loader) and crashes in Method::checked_resolve_jmethod_id when it's dereferencing the 'mid'.
The agent native code does check for JVMTI_ERROR_NONE after calling GetMethodDeclaringClass in this case, but it's too late.
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 confirming the correctness!
That seems to be the case. I learned this the hard way with crashes observed in production when attempting to release memory for stale jmethod_ids from VM side. Clarifying the comments hopefully will benefit others and avoid similar issues in the future.
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 issue is specific to JVMTI code only, the comment should reflect that. Currently it seems to suggest that all possible JNI code could use an invalid jmethodID. However, using an invalid jmethodID with JNI APIs such as CallStaticVoidMethod will lead to a crash. I don't think the JNI spec allows invalid jmethodID to be used.
Furthermore, we should consider freeing the JNIMethodBlock when JVMTI is not enabled, so we can avoid the memory leak.
We may even want to update the JVMTI spec to disallow invalid jmethodID, but will that introduce incompatibility with 3rd party code?
Lastly, do we have any existing test cases that cover the use of invalid jmethodIDs?
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 added comments to both of these bugs JDK-8268088 and JDK-8268364.
There are places in the JNI spec that don't specify a "valid method ID" and some places just say "a method ID". We might be able to fix those. We will crash for invalid method ID in all the JNI entry points. There is -Xcheck:jni that does check the method ID though. Guess I'm repeating my comment above.
As far as the new comment goes, for now, it should include JNI until which time we clarify the spec. I don't see the harm in that.
I was trying to think of a way to check jmethodID with SafeFetch or something to to return JVMTI_ERROR_INVALID_METHODID without running into ABA problems. Maybe that would work and we can deallocate the jmethodID block.
That said, I never saw the PR 66ef04a and I'd want to review that carefully. My first quick click-through was not positive. I hid JMethodIDBlocks in method.cpp on purpose! Plus it seems like a huge change for JDK 11.
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 had read the first quoted text in "Accessing Fields and Methods" when looking into the spec. It states that the method ID becomes invalid after class unloading, but the VM&JNI implementation has tolerated the invalid ID usages after unloading for long time. The second one that you quoted from "Reporting Programming Errors" has stronger language regarding the usages of illegal pointers. It would be good to clarify the language in "Accessing Fields and Methods" to not allow the usage of the invalid IDs after class unloading.
Today's implementation can tolerate the 'invalid' jemthod_ids usages after unloading (by not releasing the memory).
Agreed, that would become unsafe when we change to release memory for JNIMethodBlock and JNIMethodBlockNodes with unloaded classes. Thanks for filing JDK-8268364.
I would be very cautious to recommend this practice. I added some comments in JDK-8268364:
That would keep more memory for longer time. The agent behavior would dictate the memory usages of the VM and prevent classes from being unloaded unnecessarily. If an agent only processes all jmethod_ids at the end, it would keep all loaders alive without being able to be unloaded. Also, a bug (fail to release any of the GoabalRef) in the agent might cause memory leaks.
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.
-Xcheck:jni is not enabled commonly during testing due to other issues. Catching the issue with -Xcheck:jni before production would help (this case does make it more important to enable -Xcheck:jni).
Agreed.
I opened PR 66ef04a mainly for reference purpose for now. Please don't spend much time reviewing yet :), since we need to address the invalid jmethod_id usages first.
I moved JNIMethodBlock and JNIMethodBlockNodes class declarations to method.hpp to trigger destructor properly from classLoaderData.cpp in 66ef04a. I made the change as an internal fix and intended to only contribute it to JDK head if there was no issue.
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.
Today's JNI implementation in HotSpot does not in any way tolerate such invalid jmethodIDs. If an invalid jmethodID is passed to any APIs declared in jni.h, the VM will crash immediately. Please see JDK-8268406 for more details.
It's only JVMTI that tolerates invalid jmethodIDs.
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 checking into that! That narrows the issue, which is good.
I also created JDK-8268416 for clarifying the spec.