RFR: JDK-8245432: Lookup::defineHiddenClass should throw UnsupportedClassVersionError if the given bytes are of an unsupported major or minor version

[David Holmes]

On 29/05/2020 1:52 pm, Mandy Chung wrote:
> On 5/28/20 5:44 PM, David Holmes wrote:
>>>
>>> This is to validate the given version.  The runtime will check if 
>>> preview feature is enabled when such class file is loaded. I will 
>>> make a comment to make it clear.
>>
>> Okay but I thought the intent here was to pre-validate the version 
>> information so that when these bytes get passed to ASM you don't have 
>> to worry about the IAE that will be thrown by ASM if there is actually 
>> a problem.
> 
> Yes it is.  ASM does not check if preview features are enabled or not 
> neither.  When a class file depending preview features is passed to VM, 
> the VM will throw an exception if preview features are not enabled.

Yes but will that VM exception propagate as-is, or will ASM catch it and 
turn it into IAE? If the latter then your original problem still exists.

>> Maybe the only real solution here is for ASM to be more specific with 
>> the exceptions it throws. :(
>>
> 
> This was discussed.
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066734.html

Sure that is why you made the current change. But if ASM catches the VM 
exceptions and adapts them then your problem is not solved by 
pre-inspecting the major/minor version, unless you can implement a full 
validity check. Otherwise the only place this can be properly fixed is 
in ASM.

>> Sure but we provide that kind of cross-package access all the time. We 
>> also have JAVA_MAX_SUPPORTED_VERSION in the ModuleInfo class. Seems 
>> messy to add yet a third place where we need to determine what the 
>> current major version number is.
>>
> 
> Ah, that's another place.  I think it's better to add 
> VM::isSupportedModuleDescriptorVersion and remove these constants.
> 
>> That aside isn't the minor version, as set in java.class.version 
>> guaranteed to be zero?
>>
> This is set at build time.   The minor version is zero for the current 
> versioning scheme.

So you are allowing for someone arbitrarily setting 
DEFAULT_VERSION_CLASSFILE_MINOR in make/autoconf/version-numbers? I 
guess it doesn't hurt to be flexible, but seems unlikely anyone would 
want to mess with that. If this were VM code I'd just assert the minor 
version is 0, but Java assertions are much more heavyweight.

Thanks,
David

> Mandy
> 
>> David
>> -----
>>
>>> Mandy
>>>> Cheers,
>>>> David
>>>>
>>>>> Mandy
>>>>>
>>>>> On 5/27/20 10:57 AM, Mandy Chung wrote:
>>>>>> I'm reconsidering this fix along with JDK-8245061 that may require 
>>>>>> to do its own checking (a similar issue w.r.t. ASM validation but 
>>>>>> in this case the constant pool entry of `this_class` item is not 
>>>>>> validated).
>>>>>>
>>>>>> thanks
>>>>>> Mandy
>>>>>>
>>>>>> On 5/27/20 10:39 AM, forax at univ-mlv.fr wrote:
>>>>>>> Hi Alan,
>>>>>>> We (the ASM team) recommend to our users to check the byte 6 (and 
>>>>>>> perhaps 7) instead of relying on ASM throwing an exception,
>>>>>>> because you may update the version of ASM but not the visitors 
>>>>>>> your are using in your code.
>>>>>>>
>>>>>>> It's less brittle than catching the IAE thrown by ASM.
>>>>>>>
>>>>>>> Rémi
>>>>>>>
>>>>>>> ----- Mail original -----
>>>>>>>> De: "Alan Bateman" <Alan.Bateman at oracle.com>
>>>>>>>> À: "mandy chung" <mandy.chung at oracle.com>, "core-libs-dev" 
>>>>>>>> <core-libs-dev at openjdk.java.net>, "Remi Forax"
>>>>>>>> <forax at univ-mlv.fr>
>>>>>>>> Envoyé: Mercredi 27 Mai 2020 18:16:33
>>>>>>>> Objet: Re: RFR: JDK-8245432: Lookup::defineHiddenClass should 
>>>>>>>> throw UnsupportedClassVersionError if the given bytes are
>>>>>>>> of an unsupported major or minor version
>>>>>>>> On 26/05/2020 22:46, Mandy Chung wrote:
>>>>>>>>> Lookup::defineHiddenClass currently throws IAE by ASM if the given
>>>>>>>>> bytes are of unsupported class file version.  The implementation
>>>>>>>>> should catch and throw UnsupportedClassVersionError instead.
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.00/ 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This patch also includes a spec clarification of @throws IAE if 
>>>>>>>>> the
>>>>>>>>> the bytes has ACC_MODULE flag set to fix JDK-8245596.
>>>>>>>> Rémi - has there ever been any discussion in ASM about throwing 
>>>>>>>> more
>>>>>>>> specific exceptions? Only asking to see if we could avoid 
>>>>>>>> needing to
>>>>>>>> depend on the exception message here.
>>>>>>>>
>>>>>>>> -Alan
>>>>>>
>>>>>
>>>
>