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

[harold seigel]

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.

 >> 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
>>>>
>>