RFR 8174725: JVM should throw ClassDefNotFoundError if ACC_MODULE is set in access_flags

[harold seigel]

Thanks David for all your help.

Harold


On 2/18/2017 1:04 AM, David Holmes wrote:
> On 17/02/2017 10:58 PM, harold seigel wrote:
>> Please review this new version of the fix for JDK-8174725:
>>
>> http://cr.openjdk.java.net/~hseigel/bug_8174725.3/webrev/index.html
>>
>> This fix moves the check for ACC_Module to
>> verify_legal_class_modifiers() as required by David.
>
> Thanks Harold, changes look good to me.
>
> David
> -----
>
>>>> That raises the question as to what classfile "format" checks are
>> unconditional and which are only carried out when "verification" is
>> enabled ??
>>
>> That is outside of the scope of this bug.  Feel free to open a new bug
>> for this issue.  Until then, just use common sense or ask Alex Buckley
>> what to do when adding a format check.
>>
>> Thanks, Harold
>>
>>
>> On 2/15/2017 5:38 PM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 15/02/2017 11:30 PM, harold seigel wrote:
>>>> Hi David,
>>>>
>>>> I did not put the check in verify_legal_class_modifiers() because it
>>>> only checks the modifiers if _need_verify is set.  However, we want to
>>>> always check for ACC_MODULE (in class files >= 53) regardless of
>>>> _need_verify's value.  So verify_legal_class_modifiers() would need a
>>>> special case to check for ACC_MODULE.
>>>
>>> That raises the question as to what classfile "format" checks are
>>> unconditional and which are only carried out when "verification" is
>>> enabled ??
>>>
>>>> Also, since a classfile version check is already needed in
>>>> parse_stream(), when fetching the access_flags from the stream, it was
>>>> convenient to just do the check and possible throw right there.
>>>> However, if you think it's important, I'll move the check to
>>>> verify_legal_class_modifiers().
>>>
>>> It seems like a good place for it (not withstanding it currently
>>> returns immediately) and avoid the need to duplicate the code. It is
>>> the duplication I disliked - but "verify_legal_class_modifiers"
>>> certainly sounds like the place that would check for ACC_MODULE.
>>>
>>>> Thanks for proposing a better error message.  Instead of saying 
>>>> 'claims
>>>> to be a module', how about:
>>>>
>>>>     "% is not a class because access_flag ACC_MODULE is set"
>>>
>>> Okay I guess.
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks, Harold
>>>>
>>>> On 2/14/2017 8:00 PM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 15/02/2017 9:53 AM, harold seigel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this updated webrev:
>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725.2/webrev/index.html
>>>>>
>>>>> I'm unclear why this logic is needed in two places instead of just
>>>>> putting it inside ClassFileParser::verify_legal_class_modifiers ?
>>>>>
>>>>> Also with regards to the error message ... I think it is expressed
>>>>> inappropriately for the NCDFE case. If we were throwing
>>>>> ClassFormatError then "Illegal ACC_MODULE class modifier ..." 
>>>>> would be
>>>>> appropriate. But for NCDFE we need to phrase it in terms of the
>>>>> inability to find the class in the given class representation - I
>>>>> suggest:
>>>>>
>>>>> "%s claims to be a module (ACC_MODULE is set)"
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> This webrev has 'return' statements after the calls to fthrow() 
>>>>>> and a
>>>>>> new test case for a class file in an InnerClasses attribute that has
>>>>>> ACC_MODULE set.
>>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>>
>>>>>> On 2/14/2017 10:32 AM, Alan Bateman wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 14/02/2017 14:52, harold seigel wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review this small change to throw a NoClassDefFoundError
>>>>>>>> exception, for class file versions >= 53, if a class's 
>>>>>>>> access_flags
>>>>>>>> have ACC_MODULE set.  This behavior will be required in the 
>>>>>>>> upcoming
>>>>>>>> JVM-9 Spec.
>>>>>>>>
>>>>>>>> Open Web:
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8174725/webrev/index.html
>>>>>>>>
>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8174725
>>>>>>>>
>>>>>>>>
>>>>>>>> The fix was tested with JPRT, the hotspot, java/lang, java/util,
>>>>>>>> java/io, JFR, and other JTReg tests, the JCK lang and VM tests, 
>>>>>>>> RBT
>>>>>>>> tier2 - tier5 tests on LinuxX64, and the colocated and 
>>>>>>>> non-colocated
>>>>>>>> NSK tests.
>>>>>>> This looks okay to me although I think I would name the test case
>>>>>>> "BadAccModule" rather than "badAccModule" as it's a bit unusual to
>>>>>>> have a class name starting with a lower case letter.
>>>>>>>
>>>>>>> -Alan
>>>>>>
>>>>
>>