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

[harold seigel]

Thanks Coleen!

Harold


On 2/17/2017 10:37 AM, coleen.phillimore at oracle.com wrote:
>
> 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.
>
> Thanks,
> Coleen
>
> 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
>>>>>>>
>>>>>
>>>
>>
>