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

[harold seigel]

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?

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