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

[David Holmes]

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.

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

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