RFR(S) 7127066: Class verifier accepts an invalid class file

[harold seigel]

Thanks David!

Harold

On 3/25/2015 12:03 AM, David Holmes wrote:
> Hi Harold,
>
> On 25/03/2015 12:04 AM, harold seigel wrote:
>> Hi David,
>>
>> Thanks for the review.  I don't think we need to add test cases for all
>> *store variants because they use the same code path, but I did add a
>> test for a dstore opcode, because, like lstore, it affects two local 
>> slots.
>>
>> See BadMapDstore.jasm in new webrev:
>> http://cr.openjdk.java.net/~hseigel/bug_7127066.3/
>>
>> Does that seem okay?
>
> That seems fine.
>
> Thanks,
> David
>
>> Thanks, Harold
>>
>> On 3/24/2015 3:22 AM, David Holmes wrote:
>>> Hi Harold,
>>>
>>> On 21/03/2015 3:52 AM, harold seigel wrote:
>>>> Hi Karen,
>>>>
>>>> Thank you for your comments.
>>>>
>>>> Bytecode istore can change the type of a local slot.  So, here's a new
>>>> webrev that uses the incoming type state for all of the astore*,
>>>> dstore*, fstore*, istore*, and lstore* bytecodes.  The webrev also
>>>> contains a new test for the istore bytecode case, called
>>>> BadMapIstore.jasm.
>>>>
>>>> Could you please review this webrev, when you have a chance?
>>>>
>>>> New open webrev: http://cr.openjdk.java.net/~hseigel/bug_7127066.2/
>>>
>>> This seems more inline with the "astore and friends" comment in the
>>> bug report. Do we need to add test cases for all the *store variants?
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks! Harold
>>>>
>>>> On 3/19/2015 3:33 PM, Karen Kinnear wrote:
>>>>> Harold,
>>>>>
>>>>> I get it - this only affects instructions that set the type of a 
>>>>> local
>>>>> slot since the operand stack is cleared.
>>>>>
>>>>> So - what about istore?
>>>>> If we have a local slot that contains a long or double, Frederic
>>>>> reminded me that JVMS 2.6.1 does allow stores to those separate 
>>>>> slots,
>>>>> e.g. an istore. My mental model of this is that it would change the
>>>>> type of the local slot.
>>>>>
>>>>> Would that mean we would want to check any of the *store bytecodes?
>>>>>
>>>>> (I know the spec would change for all bytecodes which might be 
>>>>> cleaner
>>>>> - I suspect you are being extra cautious about making changes
>>>>> to reduce risk of breaking something in the field).
>>>>>
>>>>> thanks,
>>>>> Karen
>>>>>
>>>>> On Mar 18, 2015, at 2:18 PM, harold seigel wrote:
>>>>>
>>>>>> Hi Karen,
>>>>>>
>>>>>> The fix does not affect normal verifier type-state (stack map)
>>>>>> checking that is done when looping through the bytecodes. It only
>>>>>> affects whether the incoming or outgoing type-state is used when
>>>>>> comparing the type-state at a particular bytecode with the 
>>>>>> type-state
>>>>>> at the start of its potential exception handlers.
>>>>>>
>>>>>> In addition, (to paraphrase Keith's comment in the bug), it only
>>>>>> affects instructions that set the type of a local slot (astore and
>>>>>> friends) ... .  Instructions that affect the expression stack are 
>>>>>> not
>>>>>> a problem since the type-state's stack is cleared when type checking
>>>>>> an exception handler.
>>>>>>
>>>>>> So, other bytecodes such as aload and friends, getstatic and
>>>>>> getfield, etc.  are not an issue.
>>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>> On 3/16/2015 3:49 PM, Karen Kinnear wrote:
>>>>>>> Harold,
>>>>>>>
>>>>>>> Thanks for helping me walk through this in more detail.
>>>>>>>
>>>>>>> The way I read this, the fix would apply to all bytecodes - except
>>>>>>> for
>>>>>>> invokespecial <init> - which is handled I believe correctly inside
>>>>>>> the
>>>>>>> verify_invoke_init.
>>>>>>>
>>>>>>> So if you could possibly experiment with some additional
>>>>>>> instructions - I suspect
>>>>>>> you can make a conditional check where you put the beginning check
>>>>>>> and remove
>>>>>>> the check at the end.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Karen
>>>>>>>
>>>>>>> On Mar 15, 2015, at 8:58 PM, David Holmes wrote:
>>>>>>>
>>>>>>>> Hi Harold,
>>>>>>>> On 14/03/2015 4:06 AM, harold seigel wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review this fix for bug JDK-7127066.  The fix applies to
>>>>>>>>> astore*
>>>>>>>>> bytecodes because, when inside an exception handler, they can
>>>>>>>>> reference
>>>>>>>>> the thrown object and modify the number of stack locals,
>>>>>>>>> enabling the
>>>>>>>>> incorrect stack match.
>>>>>>>>>
>>>>>>>>> Open webrev:
>>>>>>>>> http://oklahoma.us.oracle.com/~hseigel/webrev/bug_7127066/
>>>>>>>>>
>>>>>>>>> JBS bug: https://bugs.openjdk.java.net/browse/JDK-7127066
>>>>>>>>>
>>>>>>>>> The fix was tested with JCK api, lang, and vm tests, jtreg 
>>>>>>>>> hotspot,
>>>>>>>>> java/lang, java/io, and java/util tests, and testbase quick and
>>>>>>>>> split
>>>>>>>>> verifier tests, and with the test case provided in the bug.
>>>>>>>> The new check looks okay, though I can't verify the exact 
>>>>>>>> placement
>>>>>>>> of it.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> Thanks! Harold
>>>>
>>