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

[harold seigel]

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
>>