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

[Mandy Chung]

On 5/29/20 4:31 PM, Johannes Kuhn wrote:
> Thanks.
>
> Noted that readInt can read outside of the byte array when it is 
> shorter than 4 bytes, throwing an ArrayIndexOutOfBoundsException.
> Same applies for readUnsignedShort.
>
> Test cases for malformedClassFiles:
> new byte[3], new byte[] {0xCA, 0xFE, 0xBA, 0xBE, 0x00}
>

Oops.  My bad.  The range check should be fixed to:

    if ((offset+4) > bytes.length) {
        throw new ClassFormatError("Invalid ClassFile structure");
    }

similarly the check for readUnsignedShort is:
     if ((offset+2) > bytes.length)

Mandy
> - Johannes
>
>
> On 29-May-20 21:23, Mandy Chung wrote:
>> It's fixed.  Thanks
>> Mandy
>>
>> On 5/28/20 4:35 PM, Johannes Kuhn wrote:
>>> Hi,
>>>
>>> just noticed a small thing:
>>> The magic is 4 bytes, but readUnsignedShort reads two bytes.
>>>
>>> - Johannes
>>>
>>> On 28-May-20 19:25, Mandy Chung wrote:
>>>>
>>>>
>>>> On 5/28/20 6:55 AM, Alan Bateman wrote:
>>>>> On 28/05/2020 05:13, 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 don't want to complicate this further but the --enable-preview 
>>>>> handling in the VM doesn't seem to expose an internal property 
>>>>> that allows code in the libraries know if preview feature are 
>>>>> enabled or not. I was assuming that isSupportedClassFileVersion 
>>>>> would need to check that.
>>>>>
>>>>
>>>> The runtime will ensure if --enable-preview is set if a class file 
>>>> with minor is loaded.   I will prefer to keep 
>>>> VM::isSupportedClassFileVersion to validate the given major/minor 
>>>> version.  At runtime, it will fail when such class file is loaded 
>>>> if preview is not enabled.
>>>>
>>>> I'll add a comment to describe that.
>>>>
>>>>> Will readUnsignedShort throw AIOBE if the offset is beyond the 
>>>>> length? 
>>>>
>>>> Good catch.  I add the check to throw CFE.
>>>> +            private static int readUnsignedShort(byte[] bytes, int 
>>>> offset) {
>>>> +                if (offset >= bytes.length) {
>>>> +                    throw new ClassFormatError("Invalid ClassFile 
>>>> structure");
>>>> +                }
>>>> +                return ((bytes[offset] & 0xFF) << 8) | 
>>>> (bytes[offset + 1] & 0xFF);
>>>> +            }
>>>>
>>>>> Also are you sure about "& 0xCAFEBABE", I assumed it would just 
>>>>> check the magic number is equal to that.
>>>>
>>>> It should check ==.  Fixed.
>>>>
>>>> thanks
>>>> Mandy
>>>
>>>
>>
>