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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Fri Feb 17 15:37:01 UTC 2017

I agree.  We need a bug to figure out why there's this _need_verify flag 
in classFileParser.cpp.   I think it was an optimization for classes on 
the bootclasspath (formerly rt.jar) because they should never have any 
errors.  But classFileParser does format checks which I can't see why 
they'd be costly for performance (should be measured as part of the 
upcoming bug).   The main effect of this _needs_verify flag here is to 
confuse people looking at the code.

I've reviewed this and it looks like a good fix.  To me it makes no 
difference if checking for module in verify_class_modifiers() is in 
_needs_verify or not.

Thank you for adding comments in the .cod files in the tests.


On 2/17/17 9:20 AM, harold seigel wrote:
> To clarify about format checks:
> Conditional checks remain conditional because of backward 
> compatibility concerns.  New checks should be unconditional unless 
> there is an explicit reason for them to be conditional.  With 
> ACC_Module, there is no reason for that check to be unconditional.
> Thanks, Harold
> On 2/17/2017 7:58 AM, 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.
>> >> 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

More information about the hotspot-runtime-dev mailing list