[9] RFR(S): 8076445: Array accesses using sun.misc.Unsafe cause data corruption or SIGSEGV

[Zoltán Majó]

Hi,


please review the following patch.

Bug: https://bugs.openjdk.java.net/browse/JDK-8076445

Problem: The errors listed in the bug description appear because the C1 
compiler can generate incorrect memory addressing if unsafe intrinsics 
are inlined.

The problem appears if a Unsafe.putInt is followed by an Unsafe.getInt 
to the same location, for example if the following Java code is compiled 
(some details omitted):

1: for (long index = 0; index < arraySize; index++) {
2:     long offset = pointer + (index * SIZE_OF_INT_IN_BYTES);
3:     unsafe.putInt(offset, (int)index);
4:     int readback = unsafe.getInt(offset);
5: }

Here is the C1 compiler's HIR for lines 3 and 4:

_p__bci__use__tid__result____instruction________________________________________ 
(HIR)
...
  .  20   1    a23  [R194|L]  a17._104 (L) unsafe
     25   1    i24  [R195|I]  l2i(l6)
  .  26   0    a25            null_check(a23)
  .  26   0    v26            UnsafePutRaw.(base l18, index l6, 
log2_scale 2, value i24)
  .  29   1    a28  [R196|L]  a17._104 (L) unsafe
  .  33   0    a29            null_check(a28)
  .  33   1    i30  [R197|I]  UnsafeGetRaw.(base l18, index l6, 
log2_scale 2)
...

On the HIR level, both UnsafePutRaw and UnsafeGetRaw use the same (and 
correct) base, index, and scale. On the LIR level, however, the 
following is generated:

_nr___instruction_______________________________________________________________ 
(LIR)
...
  92   move [Base:[R188|L] Disp: 104|L] [R194|L]
  94   null_check [R194|L]   [bci:26]
  96   convert [l2i] [R180|J] [R195|I]
  98   shift_left [R180|J] [int:2|I] [R180|J]
  100  move [R195|I] [Base:[R189|J] Index:[R180|J] Disp: 0|I]
  102  move [Base:[R188|L] Disp: 104|L] [R196|L]
  104  null_check [R196|L]   [bci:33]
  106  move [Base:[R189|J] Index:[R180|J] * 4 Disp: 0|I] [R197|I]
...

At instruction #96, the long index (R180) is converted to an int and is 
stored into R195.

At instruction #100, the int index (R195) is stored into the memory 
region allocated with unsafe. However, as we store an int, the index 
used to address the destination of the store must be scaled by 4. As a 
result, at instruction #98 the long index (from R180) is scaled by 4, 
stored back into R180, and is then used in instruction #100.

At instruction #106, a load from the same memory location is attempted 
as where we have stored before. R180 is used as scale, but the compiler 
scales R180 once more by 4 (in addition to the scaling that was already 
done at #98).

As a result, the UnsafeGetRaw accesses an incorrect memory address, 
which result in data corruption and eventually a SIGSEGV.

HIR instruction #96 is generated by LIRGenerator::do_UnsafePutRaw (lines 
2190--2193 in c1_LIRGenerator.cpp): index_op is scaled by log2_scale and 
is stored back into index_op.

HIR instruction #106 is generated by LIRGenerator::do_UnsafeGetRaw (line 
2106 in c1_LIRGenerator.cpp): The previously scaled index_op is reused 
and an additional scaling by log2_scale is performed.


Solution:

- change the scaling code in do_UnsafePutRaw to store result into a new 
temporary register tmp and not back into index_op (as index_op is 
possibly reused later);
- change the scaling code in do_UnsafePutRaw to embed the scale directly 
into the instruction on Intel architectures (as it is done by 
do_UnsafeGetRaw);
- do_UnsafePutRaw and do_UnsafeGetRaw function similarly, so refactor 
both methods to make the similarities more obvious from the source code;
- give new names to temporary registers to improve readability of the 
code, for example at places like:

2053     base_op = new_register(T_INT);
2054     __ convert(Bytecodes::_l2i, base.result(), base_op); # base op 
is originally base.result()

becomes

2059     LIR_Opr tmp = new_register(T_INT);
2060     __ convert(Bytecodes::_l2i, base_op, tmp);
2061     base_op = tmp;


Webrev: http://cr.openjdk.java.net/~zmajo/8076445/webrev.00/

Testing:
- add new test: CheckC1OptimizedUnsafeIntrinsics.java;
- CheckC1OptimizedUnsafeIntrinsics.java: fails on 5/9 JPRT platforms if 
executed without the fix;
- JPRT: all tests pass (including CheckC1OptimizedUnsafeIntrinsics.java).

Thank you and best regards,


Zoltan