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

[David Holmes]

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