RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier

[David Holmes]

On 9/02/2019 11:29 pm, gary.adams at oracle.com wrote:
> I'm willing to do the leg work on cleaning up the tests, but I'll need 
> some help
> identifying where the redefine operations are missing the 
> inner_class_flags checks.

src/hotspot/share/prims/jvmtiRedefineClasses.cpp

This code needs expanding:

// Check whether class modifiers are the same.
   jushort old_flags = (jushort) the_class->access_flags().get_flags();
   jushort new_flags = (jushort) scratch_class->access_flags().get_flags();
   if (old_flags != new_flags) {
     return JVMTI_ERROR_UNSUPPORTED_REDEFINITION_CLASS_MODIFIERS_CHANGED;
   }

to check if the class being redefined is a nested class, and if so to 
check the inner_class_flags. I don't know how to determine if the 
class/interface is nested though - I find the inner-class-attribute very 
confusing.

Note you'll have to check the tests carefully to ensure they are in fact 
redefining a nest class with a nested class. Depending on the mechanism 
used they may just be defining a top-level class with a $ in its name, 
and treating it as a nested class.

> Is spec change or a CSR required when we change the current behavior to 
> be more strict?

A CSR request for the expanded behaviour is probably in order. There's 
no spec change here  - the spec says modifiers must not be changed.

Thanks,
David
-----

> On 2/8/19 9:24 PM, David Holmes wrote:
>> Gary,
>>
>> I have filed
>>
>> https://bugs.openjdk.java.net/browse/JDK-8218703
>>
>> against JVM TI. But in doing so I realized these current test changes 
>> are actually still incorrect. The problem is that these tests are 
>> changing the modifiers on nested types and in that case it is the 
>> inner_class_access_flags that JVM TI should be examining (which can 
>> encode all four access levels), not the top-level access flag (which 
>> only handles public or not-public).
>>
>> This is all rather a mess.
>>
>> David
>> -----
>>
>> On 9/02/2019 7:48 am, David Holmes wrote:
>>> On 8/02/2019 11:38 pm, Gary Adams wrote:
>>>> On 2/5/19, 4:59 PM, David Holmes wrote:
>>>>> On 5/02/2019 10:17 pm, Gary Adams wrote:
>>>>>> On 2/4/19, 8:04 PM, David Holmes wrote:
>>>>>>> Hi Gary,
>>>>>>>
>>>>>>> On 5/02/2019 12:01 am, Gary Adams wrote:
>>>>>>>> Two of the redefine classes tests (021, 023) have been on the 
>>>>>>>> ProblemList since they
>>>>>>>> were first brought into the open repos. Both tests made an 
>>>>>>>> incorrect assumption
>>>>>>>> about the access modifiers in class files. There is a single bit 
>>>>>>>> in the binary class file that
>>>>>>>> defines if an interface is public or not public (See JVMS Table 
>>>>>>>> 4.1-B ACC_PUBLIC).
>>>>>>>> When the test classes are compiled for use in the redefine 
>>>>>>>> operation, if a source
>>>>>>>> used public or protected the ACC_PUBLIC bit was set.
>>>>>>>>
>>>>>>>> Several modifiers are used in these tests to confirm 
>>>>>>>> UnsupportedOperationException
>>>>>>>> is thrown or not thrown. This proposed change simply corrects 
>>>>>>>> the expected results
>>>>>>>> according to the JVM Specification.
>>>>>>>>
>>>>>>>>    Webrev: 
>>>>>>>> http://cr.openjdk.java.net/~gadams/8065773/webrev.00/index.html
>>>>>>>
>>>>>>> test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/redefineClasses/redefineclasses021.java 
>>>>>>>
>>>>>>>
>>>>>>> 62  * Test cases 3 (private) and 4 (static) map to not public, so 
>>>>>>> will
>>>>>>>
>>>>>>> "static" is not related to access modifiers so the ACC_PUBLIC bit 
>>>>>>> is not relevant. "static" can only be applied to (nested) member 
>>>>>>> types and is represented by the ACC_STATIC bit as per JVMS "Table 
>>>>>>> 4.7.6-A. Nested class access and property flags".
>>>>>>>
>>>>>>> What this testcase does is add "static" to a nested interface and 
>>>>>>> expects JVM TI to check if the class modifier has changed. But 
>>>>>>> the "static" is not encoded in the class "access flags", but in 
>>>>>>> the inner class attribute (inner_class_access_flags). To me this 
>>>>>>> testcase should throw UOE, but JVM TI is not checking the right 
>>>>>>> flags.
>>>>>>>
>>>>>>> David
>>>>>> I think the test is demonstrating that static does not change the 
>>>>>> ACC_PUBLIC bit.
>>>>>
>>>>> And what is that a demonstration of? static has nothing to do with 
>>>>> access control so if it did change the ACC_PUBLIC bit we'd have a 
>>>>> serious issue.
>>>>>
>>>>>>
>>>>>> This changeset does not change the handling of that testcase.
>>>>>
>>>>> The test is adding a new class modifier "static" which it 
>>>>> originally expected to trigger a UOE but it doesn't because JVM TI 
>>>>> is not checking for that kind of change correctly.
>>>>>
>>>>> You modified the test to no longer expect UOE so that it passes but:
>>>>> a) that's wrong as it shouldn't pass; and
>>>>> b) it's now documented as passing because it doesn't change the 
>>>>> public-ness of the class but that's completely irrelevant.
>>>>>
>>>>> So your change to the static testcase is not correct and should not 
>>>>> be applied.
>>>>>
>>>>> David
>>>>> -----
>>>> I think you misread the webrev.
>>>> The 4th test case was already being allowed to not throw UOE.
>>>>
>>>> The change I proposed allows the 3rd testcase to not throw UOE.
>>>> e.g. redefining package to private accessibility is still not 
>>>> ACC_PUBLIC.
>>>
>>> Okay in regards to your changeset this comment is misleading for the 
>>> static case (public/not-public is not the issue as we aren't changing 
>>> an access modified - changing the static modifier simply leaves the 
>>> class modifiers unchanged):
>>>
>>> 62  * Test cases 3 (private) and 4 (static) map to not public, so will
>>>    63  * not throw <code>UnsupportedOperationException</code> during 
>>> the redefine
>>>    64  * class operation.
>>>
>>> but the whole test is wrong for the "static" case and JVM TI is 
>>> broken for the static case so I'll file a new bug to get JVM TI and 
>>> the test fixed.
>>>
>>> David
> 
>