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

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

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

Open webrev:
http://cr.openjdk.java.net/~hseigel/bug_8175383/webrev/index.html

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

John Rose-3
On Mar 1, 2017, at 1:20 PM, harold seigel <[hidden email]> wrote:

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

+1 on keeping the code simple.  If the simple logic breaks something we can make the code more complex.

The bug here is that we want to throw a NCDFE instead of a CFE for modules, but modules are likely to contain odd constants..

The basic constraints are:  1. the JVM class-file loader must (somehow) refuse to "load" ACC_MODULE files (as a JVMS condition), 2. the JVM should not be required to perform semantic processing of 19 and 20 constants (because YAGNI, and unused code makes bugs).

(Maybe in the future we will make CP tags 19 and 20 semantically significant in normal class files.  When that day comes, we must fully process them.  But this is not that day.  This day we throw!)

That implies we should try to fail-fast as soon as we see 19 or 20 CP constants.  If some external spec. forces us to fail later, after getting to where we can test for ACC_MODULE, then we have to keep going through the class files, using only minimal logic to skip (not store) the strange constants, as we do with annotations.

I think the bug JDK-8175383 should give the reasons (either JVMS language or use case) why the more convenient CFE would be a bad outcome.  In other words, why are we doing this?

— John
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

John Rose-3
On Mar 2, 2017, at 1:05 PM, John Rose <[hidden email]> wrote:
>
> I think the bug JDK-8175383 should give the reasons (either JVMS language or use case) why the more convenient CFE would be a bad outcome.  In other words, why are we doing this?

P.S.  I have a specific comment on the source code style:

Please return the extra result not by replacing the "void" return value, but by passing an out-parameter and/or a field in the CFP object.

You can have a boolean "_bad_constant_seen".  This is much clearer and less disruptive than taking over the return value.  Your patch size will collapse.

— John
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
Thanks John!

I'll add a field to the CFP object, hopefully without increasing its size.

Harold


On 3/2/2017 8:12 AM, John Rose wrote:

> On Mar 2, 2017, at 1:05 PM, John Rose <[hidden email]
> <mailto:[hidden email]>> wrote:
>>
>> I think the bug JDK-8175383 should give the reasons (either JVMS
>> language or use case) why the more convenient CFE would be a bad
>> outcome.  In other words, why are we doing this?
>
> P.S.  I have a specific comment on the source code style:
>
> Please return the extra result not by replacing the "void" return
> value, but by passing an out-parameter and/or a field in the CFP object.
>
> You can have a boolean "_bad_constant_seen".  This is much clearer and
> less disruptive than taking over the return value.  Your patch size
> will collapse.
>
> — John

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
In reply to this post by harold seigel
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
>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

George Triantafillou
Hi Harold,

One typo:

src/share/vm/classfile/classFileParser.cpp:
313         // Record that an error occured in these two cases but keep
parsing so
                                       should be "occurred".

Otherwise, it looks good.

-George

On 3/3/2017 9: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.
>
> 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
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
Thanks George!

I"ll fix the typo before pushing the change.

Harold


On 3/3/2017 10:37 AM, George Triantafillou wrote:

> Hi Harold,
>
> One typo:
>
> src/share/vm/classfile/classFileParser.cpp:
> 313         // Record that an error occured in these two cases but
> keep parsing so
>                                       should be "occurred".
>
> Otherwise, it looks good.
>
> -George
>
> On 3/3/2017 9: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.
>>
>> 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
>>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Karen Kinnear
In reply to this post by harold seigel
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 <[hidden email]> 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
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Lois Foltan
In reply to this post by harold seigel
Looks good!
Lois

On 3/3/2017 9: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.
>
> 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
>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
In reply to this post by Karen Kinnear
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 <[hidden email]> 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
>>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

harold seigel
In reply to this post by Lois Foltan
Hi Lois,

Thanks for the review!

Harold


On 3/3/2017 10:46 AM, Lois Foltan wrote:

> Looks good!
> Lois
>
> On 3/3/2017 9: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.
>>
>> 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
>>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by harold seigel
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
>>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

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

Loading...