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

[David Holmes]

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

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.

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

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

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
>