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

[Mikael Gerdin]

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