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 29/05/2020 3:28 am, Mandy Chung wrote:
> 
> 
> 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.

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.

Maybe the only real solution here is for ASM to be more specific with 
the exceptions it throws. :(

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

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.

That aside isn't the minor version, as set in java.class.version 
guaranteed to be zero?

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