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

Fri Feb 17 16:11:06 UTC 2017

Code looks good.

In AccModuleTest, could you fix comment line 32: ClassFormatError -> NoClassDefFoundError
Thank you for testing inner classes as well.

Agree with Harold et al that this particular check should be unconditional. The meaning of this flag today could have been
expressed as a different magic number, so if this is a module-info.class file, we don’t want to look any further.

Part of the history of the _need_verify was for javaws which cached pre-verified files. It also is used for trusted class loaders.
Agreed that it is confusing, and if there is no performance benefit, totally worth removing the format checks on future
class file versions (so we don’t break code already in the field).

thanks,
Karen

> On Feb 17, 2017, at 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
>>>>>>> 
>>>>> 
>>> 
>> 
> 