RFR 8175383: JVM should throw NCDFE if ACC_MODULE and CONSTANT_Module/Package are set

[harold seigel]

Hi Karen,

Thanks for the review!

Harold


On 3/3/2017 10:45 AM, Karen Kinnear wrote:
> Harold,
>
> Looks good. Thank you for the simplified fix and for refactoring to make a single test. Having our tests run faster means
> we can do more testing, so very much appreciated.
>
> thanks,
> Karen
>
>> On Mar 3, 2017, at 9:22 AM, harold seigel <harold.seigel at oracle.com> wrote:
>>
>> Hi,
>>
>> Please review this updated webrev for JDK-8175383: http://cr.openjdk.java.net/~hseigel/bug_8175383.2/webrev/index.html
>>
>> This webrev contains a simplified fix and simplified tests based on reviewers' previous suggestions on how to improve this fix.
>>
>> Thanks, Harold
>>
>>
>> On 3/1/2017 8:20 AM, harold seigel wrote:
>>> Hi David,
>>>
>>> Thanks for your input.  Please see comments in-line.
>>>
>>>
>>> On 2/28/2017 10:50 PM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> On 27/02/2017 11:48 PM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this fix to throw NoClassDefFoundError exceptions, instead
>>>>> of ClassFormatError exceptions, for version 53 or greater class files
>>>>> that have ACC_MODULE set in their access_flags and one or more constant
>>>>> pool entries of CONSTANT_Module or CONSTANT_Package (19 or 20). The JVM
>>>>> parses the constant pool before parsing the access_flags.  So, it needs
>>>>> to save the fact that the constant pool contained an entry of 19 or 20.
>>>>> Then, after checking for ACC_Module in the access_flags, decide which
>>>>> exception to throw.
>>>> That is truly awful! - and that's not a reflection on you or your code :)
>>> I'll discuss this with Alex.  Perhaps the JVM Spec requirement is more flexible than what I implemented.
>>>> This kind of approach doesn't scale - what happens when we have new rules in 10 for classfile version >=54? There are too many special cases.
>>>>
>>>> Really - and this is I concede out-of-scope for this stage of 9 and this bug - we need to separate the structural parsing errors from the logical errors, so we can do the main parsing and then all the convoluted "if version >= 53 and has_constant_module and has_acc_module ..." logic.
>>> This sounds like a good enhancement for JDK-10.
>>>>> To summarize the desired behavior:
>>>>>
>>>>>    For class file versions < 53 throw CFE for any bad constant pool
>>>>>    entry regardless of access_flags.
>>>>>
>>>>>    For class file versions >= 53 throw NCDFE for a bad constant pool
>>>>>    entry of 19 or 20 and ACC_Module set in access_flags.
>>>>>
>>>>>    For class file versions >= 53 throw CFE for a bad constant pool
>>>>>    entry of 19 or 20 and ACC_Module not set in access_flags.
>>>>>
>>>>>    For class file versions >= 53 throw CFE for a bad constant pool
>>>>>    entry other than 19 or 20 regardless of access_flags.
>>>>>
>>>>> The reasoning behind this is that constant pool entries 19 and 20 are
>>>>> only valid when ACC_Module is set.  So, if ACC_Module is set then
>>>>> constant pool entries 19 and 20 are technically valid and CFE should not
>>>>> be thrown.  Instead, a NCDFE is thrown because of ACC_Module.
>>>>>
>>>>> I moved the check for ACC_MODULE out of verify_legal_class_modifiers()
>>>>> for the following reason.  Suppose there's a CONSTANT_Module entry in
>>>>> the constant pool and suppose verify_legal_class_modifiers() throws a
>>>>> CFE for a reason unrelated to ACC_MODULE.  That could be considered a
>>>>> bug because the CFE should be thrown with a message describing the bad
>>>>> CONSTANT_Module entry, not the bad access_flags.  If it does not matter
>>>>> which CFE gets thrown then the ACC_MODULE check could be moved back into
>>>>> verify_legal_class_modifiers().
>>>> I prefer the ACC_MODULE check where it was so that we don't duplicate the logic. I'm not aware of anything that gives precedence to one CFE over another, or how anyone could legitimately complain about the behaviour either way.
>>> If no one objects then I'll move the ACC_MODULE check back into verify_legal_class_modifiers().  It will simplify the change.
>>>>> Open webrev:
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8175383/webrev/index.html
>>>> Functional changes okay modulo what I already said.
>>>>
>>>> For the tests ... I find the use of asm very cumbersome, and prefer using jcods myself. That aside, can you not factor out the custom classloader so that it only has to be written once? It can take the name of the class to treat specially as a constructor arg, and the exception message checking can go around the loadClass call instead of the defineClass call.
>>> I agree that asm is very cumbersome but I couldn't use jcod or jasm because they don't support constant pool items of 19 or 20. I'll look into factoring out the custom classloader.
>>>
>>> Thanks, Harold
>>>> Thanks,
>>>> David
>>>>
>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8175383
>>>>>
>>>>> The fix was tested with the JCK lang and vm tests, the JTreg hotspot,
>>>>> java/io, java/lang, java/util and other tests, the RBT tier2 -tier5
>>>>> tests, the colocated and non-colocated NSK tests, and with JPRT.
>>>>>
>>>>> Thanks, Harold
>>>>>