RFR (S) 8229900: RedefineDeleteJmethod.java fails with -Xcheck:jni

Thu Oct 3 11:29:53 UTC 2019

Thank you for the comments, David.

On 10/3/19 3:04 AM, David Holmes wrote:
> Hi Coleen,
>
> On 2/10/2019 11:36 pm, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 10/2/19 12:53 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> Sorry rather long-winded reply ...
>>>
>>> On 1/10/2019 11:52 pm, coleen.phillimore at oracle.com wrote:
>>>> Summary: Remove RedefineClasses adjustment and test, but improve 
>>>> checking for method/class matching.
>>>>
>>>> Tested with tier1 with -Xcheck:jni locally, and tier1 on Oracle 
>>>> platforms.
>>>>
>>>> open webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2019/8229900.01/webrev
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8229900
>>>
>>> I was troubled by this change because it made me think more deeply 
>>> about what should reasonably happen with respect to jmethodIDs and 
>>> class redefinition.
>>>
>>> I had a (simplistic?) mental model that a jmethodID is a "pointer" 
>>> to a specific method in a specific class (the defining class of the 
>>> method). As such a jmethodID should only be used when operating on 
>>> that class (or subclass thereof) or an instance of that class (or 
>>> subclass thereof). With that mental model it makes sense for 
>>> Xcheck:jni to validate the defining class is the target class, or a 
>>> superclass thereof. But it doesn't then make sense for class 
>>> redefinition to update jmethodIDs as the redefined class is not the 
>>> original class! If a class is redefined then it should, in my mental 
>>> model, invalidate all existing jmethodIDs that pertained to the 
>>> original class.
>> Your simplistic model is correct.  The jmethodID is a pointer to a 
>> specific method in a class.  In the call, we check that it applies to 
>> a class or subclass.
>>
>> The current behavior is that if the method is "obsolete", meaning 
>> that the bytecodes are new, the methodID is not replaced:
>>
>>        // obsolete methods need a unique idnum so they become new 
>> entries in
>>        // the jmethodID cache in InstanceKlass
>>        assert(old_method->method_idnum() == 
>> new_method->method_idnum(), "must match");
>>        u2 num = InstanceKlass::cast(_the_class)->next_method_idnum();
>>        if (num != ConstMethod::UNSET_IDNUM) {
>>          old_method->set_method_idnum(num);
>>        }
>>
>> If the redefined method is the same *bytecodes* as the old method 
>> "emcp", the methodID *is* replaced.   This is good.
>
> Okay I don't have a good enough understanding for how this is all 
> supposed to work to continue discussing this aspect. Seems we don't 
> actually invalidate jmethodIDs they either keep referring to the old 
> method implementation, or they are updated to the new one if it is 
> equivalent to the old one.

We do refer to the old Method, but if nothing else refers to the old 
Method, it can be cleaned up with deallocated metadata.  I was looking 
into how this used to work before permgen elimination, to see if this is 
a bug.

The jmethodID used to point to a methodOop, but it was a weak global 
handle.  Which explains the comment that I fixed.  It also means that if 
nothing else referred to the methodOop it would be gc'ed. So I don't 
think it's broken now.
>
>>>
>>> However, that's not very user friendly in the face of redefinition 
>>> of a superclass as code that only works with the subclass may 
>>> reasonably expect jmethodIDs to remain valid even if they refer to 
>>> an inherited method that has been redefined. So we update them to 
>>> refer to the redefined method implementation.
>>
>> Not sure I follow this.  We only update them for emcp methods, but I 
>> don't see why calling through a subclass is a distinction.
>
> Let's say I am working with a subclass and obtained jmethodIDs that 
> refer to methods in the superclass. If the superclass is redefined 
> outside the knowledge of the code using the subclass and jmethodIDs, 
> then suddenly making those jmethodIDs be invalid would come as a bit 
> of a surprise to that code. But I see now that they never become 
> invalid so the point is moot.
>
>>>
>>> So in that regards the update to jniCheck::validate_call_class seems 
>>> correct. Though I wonder if we also need to check that obj->class is 
>>> a subtype of clazz? As far as I can see we never actually validate 
>>> that! We use the jmethodID to find the method and we then find the 
>>> vtable index wrt. the method->holder class, and then we use that 
>>> vtable index to lookup a method in the receiver object's class - 
>>> which could lead to a random method being selected in a different 
>>> class!
>>
>> We could add that check in a new RFE.
>>
>>>
>>> Continuing on, if we do expect jmethodIDs to get updated upon class 
>>> redefinition then it makes sense to me to keep the logic that 
>>> handles deleted methods, by redirecting them to a method that throws 
>>> NSME. The fact that method is in Unsafe is unfortunate but it is 
>>> what we do elsewhere in the VM. I'm assuming the problem here is 
>>> that the augmented jniCheck::validate_call_class will fail in such 
>>> cases? That is a problem, but I think I'd rather see it 
>>> special-cased than change the existing behaviour:
>>>
>>>   if (obj != NULL) {
>>>     jniCheck::validate_object(thr, obj);
>>>   }
>>> + if (m == Universe::throw_no_such_method_error())
>>> +   return;  // skip class checks in this case
>>>
>>> then the test could also remain.
>>>
>>> Also note that while we generally require JNI programmers to ensure 
>>> everything is called correctly, jmethodIDs are also used by JVM TI 
>>> and we tend to want JVM TI to have well defined semantics. I'm 
>>> unclear now what happens if we invoke a deleted method through JVM TI ?
>>
>> JVMTI semantics should follow JNI semantics.   If JVMTI uses 
>> jmethodIDs and invoke a deleted method, it should get undefined 
>> behavior.   Not sure what you mean here either.
>
> I hold JVM TI to a higher standard than JNI when it comes to dealing 
> with these kinds of errors. But I was under the mistaken impression 
> that JVM TI provided an API to invoke methods via jmethodIDs, but 
> seems I was wrong and that it is only available in JNI.
>
> I still dislike the idea that there is "undefined behaviour" here, and 
> I would expect any jmethodID that referred to a now deleted method to 
> actually still refer to it as it is the "old version". But that's not 
> related to this change so ...
>
> I don't know the exact context under which the NSME handling was put 
> in, so can't really comment on its removal. Throwing the NSME doesn't 
> seem unreasonable to me but that's besides the point.

I put the NSME in as part of a different change because I'd noticed 
this, and was trying to be helpful.  I don't think it helped anyone.  
People have to be careful with JNI and redefinition.  Maybe it would be 
more helpful to NULL the obsolete or deleted method slot, so that users 
can find their errors sooner.  This change just reverts it back to the 
status quo.

Thanks,
Coleen
>
> Thanks,
> David
>
>
>> When looking at this change and trying to decide whether to keep this 
>> replacement with the special cases in jniCheck.cpp above or remove 
>> it, these things led me to decide to remove it.
>>
>> 1. This is JNI and we don't generally protect people from undefined 
>> behavior.   In fact, if you're using JNI, I think a crash is a better 
>> debugging tool than a Java NSME exception, which will propagate out 
>> of the JNI context.  That would be my preference if I were a developer.
>> 2. Adding/deleting methods in redefinition has been removed, except 
>> with a command line switch to enable old behavior, so adding 
>> something helpful for something deprecated seemed silly.
>> 3. This "helpful" NSME for jmethodID was a recent addition for a 
>> different problem.  Nobody relies on this.
>> 4. It looks like the "helpful" NSME replacement unintentionally 
>> affected more than deleted methods, so old code might get different 
>> results (see 1 above).
>> 5. We have so many special cases in the jvm for so many different 
>> things, we don't need another.
>>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> David
>>> ----
>>>
>>>> Thanks,
>>>> Coleen
>>