[8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Jun 9 07:40:20 UTC 2016
Hi Shafi,
On 6/8/16 22:29, Shafi Ahmad wrote:
> Hi Serguei, Jiangli,
>
> I am sorry for my mistake , the below code will get executed and it is not unreachable code.
It does not make sense to run this code anyway.
Thanks,
Serguei
>
> // jmethodIDs are supposed to be weak handles in the class loader data,
> // but that can be expensive so check it last
> else if (!Method::is_method_id(method_id)) {
> ReportJNIFatalError(thr, fatal_non_weak_method);
> }
>
> Regards,
> Shafi
>
> -----Original Message-----
> From: Shafi Ahmad
> Sent: Thursday, June 09, 2016 10:52 AM
> To: Serguei Spitsyn; Jiangli Zhou
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: RE: [8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)
>
> Hi Serguei,
>
>
>
> Thanks for the review comment.
>
>
>
> I will send the updated webrev link after removing the suggested unreachable code.
>
>
>
> Regards,
>
> Shafi
>
>
>
> From: Serguei Spitsyn
> Sent: Thursday, June 09, 2016 3:03 AM
> To: Jiangli Zhou; Shafi Ahmad
> Cc: hotspot-runtime-dev at openjdk.java.net; Coleen Phillimore
> Subject: Re: [8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)
>
>
>
> Hi Jiangli,
>
> Nice catch!
>
> The check_resolve_jmethod_id() is used in the jniCheck contexts, so it can be Ok not to be so light-weight.
> It seems, the new is_method_id() is a nice check to have there.
> Besides the whitebox.cpp the validate_jmethod_id() function is the only caller of it.
>
> But it does not look right to call the is_method_id in the validate_jmethod_id().
> I'd suggest to remove the lines:
>
> // jmethodIDs are supposed to be weak handles in the class loader data,
>
> // but that can be expensive so check it last
>
> else if (!Method::is_method_id(method_id)) {
>
> ReportJNIFatalError(thr, fatal_non_weak_method);
>
> }
>
>
> The only impact is that a different JNI fatal error will be reported.
> Not sure, it is important though.
>
> Thanks,
> Serguei
>
>
> On 6/8/16 11:24, Jiangli Zhou wrote:
>
> Hi Shafi,
>
>
>
> The following in src/share/vm/prims/jniCheck.cpp calls Method::checked_resolve_jmethod_id() as a quick check before calling the Method::is_method_id(). It seems Method::checked_resolve_jmethod_id() is expected to be a light-weight check. Your change calling is_method_id() from check_resolve_jmethod_id() changes that. Also, is_method_id() will be called twice in the jniCheck::validate_jmethod_id() call path.
>
>
>
> Method* jniCheck::validate_jmethod_id(JavaThread* thr, jmethodID method_id) {
>
> ASSERT_OOPS_ALLOWED;
>
> // do the fast jmethodID check first
>
> Method* moop = Method::checked_resolve_jmethod_id(method_id);
>
> if (moop == NULL) {
>
> ReportJNIFatalError(thr, fatal_wrong_class_or_method);
>
> }
>
> // jmethodIDs are supposed to be weak handles in the class loader data,
>
> // but that can be expensive so check it last
>
> else if (!Method::is_method_id(method_id)) {
>
> ReportJNIFatalError(thr, fatal_non_weak_method);
>
> }
>
> return moop;
>
> }
>
>
>
> Thanks,
>
> Jiangli
>
>
>
> On Jun 8, 2016, at 2:08 AM, Shafi Ahmad <HYPERLINK "mailto:shafi.s.ahmad at oracle.com"shafi.s.ahmad at oracle.com> wrote:
>
>
>
> Hi,
>
>
>
> Thanks Serguei for looking into it.
>
>
>
> Please find the updated webrev with suggested code change.
>
> Updated webrev link: HYPERLINK "http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.01/"http://cr.openjdk.java.net/~shshahma/8147451/webrev.01/
>
>
>
> Regards,
>
> Shafi
>
>
>
> From: Serguei Spitsyn
> Sent: Wednesday, June 08, 2016 11:40 AM
> To: HYPERLINK "mailto:hotspot-runtime-dev at openjdk.java.net"hotspot-runtime-dev at openjdk.java.net; Shafi Ahmad
> Cc: Coleen Phillimore
> Subject: Re: [8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)
>
>
>
> Once again with Shafi's email address added ...
>
> Hi Shafi,
>
> You did not reply on my email above.
> It can be because your email address was not directly included into the list.
>
> Thanks,
> Serguei
>
>
> On 6/3/16 17:32, HYPERLINK "mailto:serguei.spitsyn at oracle.com"HYPERLINK "mailto:serguei.spitsyn at oracle.com"serguei.spitsyn at oracle.com wrote:
>
> Hi Shafi,
>
> I agree that this change is safe.
> However, there are still two more spots that need to be fixed in the jdk8:
>
> // During class unloading the methods are cleared, which is different
> // than freed.
> void clear_all_methods() {
> for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
> for (int i = 0; i< number_of_methods; i++) {
> - _methods[i] = NULL;
> + b->_methods[i] = NULL;
> }
> }
> }
> @@ -1799,7 +1811,7 @@
> int count = 0;
> for (JNIMethodBlock* b = this; b != NULL; b = b->_next) {
> for (int i = 0; i< number_of_methods; i++) {
> - if (_methods[i] != _free_method) count++;
> + if (b->_methods[i] != _free_method) count++;
> }
> }
> return count;
> @@ -1871,6 +1883,10 @@
> return o;
> };
>
>
> You can find this information in one of the bug report comments.
>
> Thanks,
> Serguei
>
>
>
> On 6/3/16 15:18, Coleen Phillimore wrote:
>
> This seems like a safe change. Coleen On 5/24/16 4:34 AM, Shafi Ahmad wrote:
>
> Hi, Please review the small code change for bug: "JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)" on jdk8u-dev Summary: resolve_jmethod_id() is getting called with invalid jmethodID and there is no check for validity of the method id inside this function. So before calling resolve_jmethod_id() we should check its validity. This code change add this check. Also inside Method::is_method_id() we are not checking return value of method_holder(). It may return NULL if method id is not valid so I have added null check for this too. Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.00/"http://cr.openjdk.java.net/~shshahma/8147451/webrev.00/ Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8147451 Test: Run jprt Regards, Shafi
>
>
>
>
More information about the hotspot-runtime-dev
mailing list