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

[David Holmes]

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
>