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

[David Holmes]

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