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

[Mikael Gerdin]

Serguei,

On Wednesday 30 October 2013 02.35.08 serguei.spitsyn at oracle.com wrote:
> Hi Mikael,
> 
> Interesting investigation!
> The fix looks good.

Thanks for the review!
/Mikael

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