[8u] RFR: JDK-8147451: Crash in Method::checked_resolve_jmethod_id(_jmethodID*)
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Jun 9 16:08:34 UTC 2016
I may have missed some things on this thread.
From
http://cr.openjdk.java.net/~shshahma/8147451/webrev.02/src/share/vm/oops/method.cpp.udiff.html
InstanceKlass* ik = m->method_holder();
+ if (ik == NULL) {
+ return false;
+ }
The method_holder should never be null. Did you find a case where it was?
http://cr.openjdk.java.net/~shshahma/8147451/webrev.02/src/share/vm/prims/jniCheck.cpp.udiff.html
This seems like a good change. As Serguei says, we don't need to
optimize this for slight performance with -Xcheck:jni.
Thanks for figuring this out and fixing this bug!
Coleen
On 6/9/16 3:53 AM, Shafi Ahmad wrote:
>
> Thanks Serguei for the review.
>
> Regards,
>
> Shafi
>
> *From:*Serguei Spitsyn
> *Sent:* Thursday, June 09, 2016 1:12 PM
> *To:* Shafi Ahmad; Jiangli Zhou
> *Cc:* hotspot-runtime-dev at openjdk.java.net; Coleen Phillimore
> *Subject:* Re: [8u] RFR: JDK-8147451: Crash in
> Method::checked_resolve_jmethod_id(_jmethodID*)
>
> It looks good.
>
> Thanks,
> Serguei
>
>
> On 6/8/16 22:48, Shafi Ahmad wrote:
>
> Updated webrev link:
> http://cr.openjdk.java.net/~shshahma/8147451/webrev.02/
> <http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.02/>
>
> 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
> <mailto: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
>
> elseif(!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
>
> elseif(!Method::is_method_id(method_id)) {
>
> ReportJNIFatalError(thr, fatal_non_weak_method);
>
> }
>
> returnmoop;
>
> }
>
> Thanks,
>
> Jiangli
>
> On Jun 8, 2016, at 2:08 AM, Shafi Ahmad
> <shafi.s.ahmad at oracle.com
> <mailto: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:
> http://cr.openjdk.java.net/~shshahma/8147451/webrev.01/
> <http://cr.openjdk.java.net/%7Eshshahma/8147451/webrev.01/>
>
>
>
> Regards,
>
> Shafi
>
>
>
> From: Serguei Spitsyn
> Sent: Wednesday, June 08, 2016 11:40 AM
> To: hotspot-runtime-dev at openjdk.java.net
> <mailto: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"serguei.spitsyn at oracle.com
> <mailto: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/
> <http://cr.openjdk.java.net/%7Eshshahma/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