Review/Comment request (S) 8027252: Crash in interpreter because get_unsigned_2_byte_index_at_bcp reads 4 bytes

[serguei.spitsyn at oracle.com]

Hi Mikael,

Interesting investigation!
The fix looks good.

Thanks,
Serguei

On 10/30/13 1:45 AM, Mikael Gerdin wrote:
> Hi,
>
> So far I've only gotten a review from Coleen on this fix.
> Unless anyone is opposed to pushing this fix, regardless of what we do about
> the possible triggering changes for this, I'd still like to push this.
>
> /Mikael
>
> On Friday 25 October 2013 17.55.10 Mikael Gerdin wrote:
>> Hi all,
>>
>> Initially I'd like to hear people's opinions on this issue. It appears to
>> have surfaced after we've reduced the unnecessary alignment "cushions" for
>> metaspace together with the fact that we are now always able to use the
>> very last bit of a VirtualSpace (in Metaspace).
>>
>> The issue is extremely difficult to reproduce, because it relies on the
>> exact order and size of all metaspace allocations for a run to trigger this
>> problem.
>>
>> Description of the problem:
>>
>> We recently had some mysterious crashes in the GC testing, and I narrowed
>> them down to the following scenario:
>>
>> Imagine that we have a VirtualSpace (for Metaspace)
>> [                     |        bb]
>>                         ConstMethod
>>
>> And that the very last object allocated in such a space is a ConstMethod,
>> and that it has its embedded bytecodes filling up to the very end.
>>
>> Consider the case when the second last bytecode (the one before "Xreturn")
>> is one with an embedded 2-byte index (a constant pool or local variable
>> index) for example:
>> - (bcp) checkcast #6 0xc0 0x00 0x06
>> - (bcp+3) areturn 0xb0
>> - (bcp+4) UNMAPPED MEMORY
>>
>> The template interpreter here uses a 4-byte load on intel to read the index
>> embedded in the checkcast:
>> mov    0x1(%esi),%ebx
>> this will read the bytes [bcp+1,bcp+4] and cause a SEGV.
>>
>> I looked through the code for places where we do similar loads of 2-byte
>> indexes and found the same problem for iload_w:
>>     0xe73f7fbb:  mov    0x2(%esi),%ebx
>>     0xe73f7fbe:  bswap  %ebx
>>     0xe73f7fc0:  shr    $0x10,%ebx
>>
>> I looked through most places where we emit
>> __ movl
>> __ bswap
>> and changed the places I could find which were affected.
>>
>> Suggested fix:
>> Replace movl with load_[un]signed_short in the places where we load 2-byte
>> values from the byte code stream.
>> I've verified that load_unsigned_short (which boils down to movzwl, opcodes
>> 0x0f 0xb7 only reads 2 bytes).
>>
>> Buglink: https://bugs.openjdk.java.net/browse/JDK-8027252
>> Webrev: http://cr.openjdk.java.net/~mgerdin/8027252/webrev.0/
>>
>> Testing:
>> vm.quick.testlist -Xint on linux-i586 and x86_64(running)
>>
>> Thanks
>> /Mikael