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

Mon Mar 6 14:27:04 UTC 2017

Hi David,

Thanks for the review!

  I would like to leave it as "case 19:" and "case 20:" until the JVM 
fully accepts JVM_CONSTANT_Module and JVM_CONSTANT_Package. Otherwise, I 
think it is confusing why they are not included in methods in class 
ConstantPool such as verify_on(), print_on(), and others.

I'll remove the unnecessary comments and assert() before pushing the change.

Thanks! Harold

On 3/5/2017 8:02 PM, David Holmes wrote:
> Hi Harold,
>
> On 4/03/2017 12:22 AM, harold seigel 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.
>
> A couple of nits - feel free to ignore:
>
>  311       case 19:
>  312       case 20: {
>
> Shouldn't these have symbolic names defined?
>
> ---
>
>  388   short bad_constant = class_bad_constant_seen();
>  389   if (bad_constant != 0) {
>  390     // Either a CONSTANT_Module or CONSTANT_Package entry was 
> found in the constant
>  391     // pool.  So, stop parsing and return.  The caller will 
> decide whether to throw
>  392     // NCDFE if it finds ACC_MODULE in the class's access_flags 
> or throw CFE for
>  393     // the bad constant pool entry.
>  394     assert((bad_constant == 19 || bad_constant == 20) && 
> _major_version >= JAVA_9_VERSION,
>  395            "Unexpected bad constant pool entry");
>  396     return;
>  397   }
>
> I don't think we need the detailed comments or the assertion here. As 
> part of generalizing this it could just be:
>
> if (class_bad_constant_seen() != 0) {
>   // a bad CP entry has been detected previously so
>   // stop parsing and just return;
>   return;
> }
>
> Thanks,
> David
> -----
>
>> 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
>>>>>
>>>
>>