RFR: JDK-8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier
David Holmes
david.holmes at oracle.com
Sun Feb 10 01:03:43 UTC 2019
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
>
>
More information about the serviceability-dev
mailing list