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

[Mandy Chung]

On 5/28/20 12:44 AM, David Holmes wrote:
> Hi Mandy,
>
> On 28/05/2020 2:13 pm, Mandy Chung wrote:
>> Updated webrev:
>> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8245432/webrev.01/
>>
>> I modify this patch to check the class file version and throws CFE if 
>> unsupported before creating ClassReader.  This also fixes JDK-8245061 
>> that it reads the value of `this_class` as a constant (as ASM will 
>> throw an exception if it's not a constant) and also ensures that 
>> `this_class` is a CONSTANT_Class_info by checking the descriptor 
>> string from Type.
>
> I couldn't quite follow all the details so just a couple of comments:
>
>  src/java.base/share/classes/jdk/internal/misc/VM.java
>
> +     public static boolean isSupportedClassFileVersion(int major, int 
> minor) {
> +         if (major < 45 || major > classFileMajorVersion) return false;
> +         if (major < 56) return minor == 0;
> +         return minor == 0 || minor == PREVIEW_MINOR_VERSION;
> +     }
>
> that is only a partial validity test for preview versions - the major 
> version must match the current version as well.

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.
>
> +         s = props.get("java.class.version");
> +         int index = s.indexOf('.');
> +         try {
> +             classFileMajorVersion = Integer.valueOf(s.substring(0, 
> index));
> +             classFileMinorVersion = 
> Integer.valueOf(s.substring(index+1, s.length()));
> +         } catch (NumberFormatException e) {
> +             throw new InternalError(e);
> +         }
>
> Can you not access java.lang.VersionProps to get the version info?
>
VersionProps is package-private.

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