JDK-8202414 : Unsafe write after primitive array creation may result in array length change
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 8,9,10,11,12
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2018-04-27
  • Updated: 2019-09-13
  • Resolved: 2019-05-01
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 11 JDK 13 JDK 8 Other
11.0.4Fixed 13 b19Fixed 8u231Fixed openjdk8u222Fixed
Related Reports
Relates :  
Description
ADDITIONAL SYSTEM INFORMATION :
CentOS 7.2 / Ubuntu 14.04

A DESCRIPTION OF THE PROBLEM :
Generally speaking, this a problem that the primitive array length changes when Unsafe.putInt is following the array creation.

This problem happens in all our test jvm (7u91, 8u132 (openjdk), 8u172, 10.0.1)

The core problem code can be simplified as:

byte[] buf = new byte[397];
THE_UNSAFE.putInt(buf, BYTE_ARRAY_BASE_OFFSET + 1, buf.length);

After these code, buf.length "should" never be changed.
But the length really changes on some running of multiple executions.

Under default settings with hotspot jvm (no user defined options provided),
the problem can be reproduced occasionally.

Under extra option provided (-XX:CompileOnly=JvmTest.serBytes), 
the problem can always be reproduces.

With openjdk hotspot jvm with ASSERT enabled, (my jdk8 build is 8u132) 
the jvm crash with following information:

# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/root/lty/jdk8/openjdk/hotspot/src/share/vm/opto/memnode.cpp:2883), pid=19749, tid=140692759176960
#  assert((end_offset % BytesPerInt) == 0) failed: odd end offset
#


For your convenience, our analysis shows the problem may relate to array InitializeNode logic.
It `capture_store` the the memory write of Unsafe.putInt.
Since the putInt occupied offset range [17, 21] from the array pointer,
then it decided to `clear_memory` of offset range [16, 17] of the array pointer.
This range actually cannot pass the assert "assert((end_offset % BytesPerInt) == 0, "odd end offset")".
While in jvm product mode, without the assert, the compiler falsely calculated to clear range [13, 17],
which will clear the three most significant bytes of the `length` of this array.

In the example code, byte array with length 397 (0x0000018d) becomes length 141 (0x0000008d).

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Run the java code multiple times

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
(Output nothing)
ACTUAL -
Occasionally output multiple lines:
"array length internal error, expected: 397 (0x18d) actual: 141 (0x8d)"

---------- BEGIN SOURCE ----------
import sun.misc.Unsafe;

import java.lang.reflect.Field;
import java.security.AccessController;
import java.security.PrivilegedAction;

public class JvmTest {

  public static void main(String[] args) {
    System.err.close();
    int count = 0;
    while (count++ < 120000) {
      test();
    }
  }

  public static void test() {
    byte[] newBuf = serBytes(397);

    if (newBuf.length != 397) {
      System.out.println("array length internal error, expected: " +
              397 + " (0x" + Integer.toHexString(397) + ")"
              + " actual: " + newBuf.length
              + " (0x" + Integer.toHexString(newBuf.length) + ")");
    }
  }

  public static byte[] serBytes(int bufLen) {
    byte[] buf = new byte[bufLen];
    THE_UNSAFE.putInt(buf, BYTE_ARRAY_BASE_OFFSET + 1, buf.length);
    System.err.println("ref " + buf);
    return buf;
  }


  /*
    Unsafe fields and initialization
   */
  static final Unsafe THE_UNSAFE;
  static final long BYTE_ARRAY_BASE_OFFSET;
  static {
    THE_UNSAFE = (Unsafe) AccessController.doPrivileged(
            new PrivilegedAction<Object>() {
              @Override
              public Object run() {
                try {
                  Field f = Unsafe.class.getDeclaredField("theUnsafe");
                  f.setAccessible(true);
                  return f.get(null);
                } catch (NoSuchFieldException | IllegalAccessException e) {
                  throw new Error();
                }
              }
            }
    );
    BYTE_ARRAY_BASE_OFFSET = THE_UNSAFE.arrayBaseOffset(byte[].class);
  }
}

---------- END SOURCE ----------

FREQUENCY : occasionally



Comments
Fix Request (11u, 8u) This patch resolves the corner case in C2 that breaks Java programs using Unsafe. Patch applies cleanly to 11u and with reshuffles to 8u. New regression test fails in 11u and 8u without the product patch, and passes with the full patch. Patched 11u passes tier1 tests. Patched 8u passes all hotspot tests (there are unrelated failures).
27-05-2019

<webrev.04> - http://cr.openjdk.java.net/~rraghavan/8202414/webrev.04/ http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-April/033527.html
30-04-2019

I tried a different fix, then realized it was almost the same as Vladimir's, so I merged them: diff -r 7d5a4a48e876 src/hotspot/share/opto/memnode.cpp --- a/src/hotspot/share/opto/memnode.cpp Wed Mar 27 14:40:36 2019 -0700 +++ b/src/hotspot/share/opto/memnode.cpp Wed Mar 27 19:06:26 2019 -0700 @@ -4167,10 +4167,11 @@ // See if this store needs a zero before it or under it. intptr_t zeroes_needed = st_off; - if (st_size < BytesPerInt) { + if (st_size < BytesPerInt || (zeroes_needed % BytesPerInt) != 0) { // Look for subword stores which only partially initialize words. // If we find some, we must lay down some word-level zeroes first, // underneath the subword stores. + // Do the same for unaligned stores. // // Examples: // byte[] a = { p,q,r,s } => a[0]=p,a[1]=q,a[2]=r,a[3]=s @@ -4192,8 +4193,8 @@ // z's_needed 12 16 16 16 16 16 16 // zsize 0 0 0 0 4 0 4 if (next_full_store < 0) { - // Conservative tack: Zero to end of current word. - zeroes_needed = align_up(zeroes_needed, BytesPerInt); + // Conservative tack: Zero to end of written word(s). + zeroes_needed = align_up(next_init_off, BytesPerInt); } else { // Zero to beginning of next fully initialized word. // Or, don't zero at all, if we are already in that word.
28-03-2019

<webrev.02> - http://cr.openjdk.java.net/~rraghavan/8202414/webrev.02/
27-03-2019

<webrev.01> - http://cr.openjdk.java.net/~rraghavan/8202414/webrev.01/ <RFR> - http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-March/033215.html
25-03-2019

http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-September/030536.html
13-03-2019

Okay. It means this fix does not work and we have to avoid collecting unaligned stores by Initialize node as I said.
12-09-2018

01. Original issue reported with test case, checked with latest sources, fastdeug build - $ javac JvmTest.java $ java JvmTest Crash -- # assert((end_offset % BytesPerInt) == 0) failed: odd end offset 02. Getting new assert failure, with above proposed fix1 patch ---------- diff -r 74dde8b66b7f src/hotspot/share/opto/memnode.cpp --- a/src/hotspot/share/opto/memnode.cpp Tue Sep 11 09:42:27 2018 -0400 +++ b/src/hotspot/share/opto/memnode.cpp Tue Sep 11 09:11:25 2018 -0700 @@ -4095,10 +4095,11 @@ // See if this store needs a zero before it or under it. intptr_t zeroes_needed = st_off; - if (st_size < BytesPerInt) { + if (st_size < BytesPerInt || (zeroes_needed % BytesPerInt) != 0) { // Look for subword stores which only partially initialize words. // If we find some, we must lay down some word-level zeroes first, // underneath the subword stores. + // Do the same for unaligned stores. // // Examples: // byte[] a = { p,q,r,s } => a[0]=p,a[1]=q,a[2]=r,a[3]=s ---------------- $ javac JvmTest.java $ java JvmTest ... # # A fatal error has been detected by the Java Runtime Environment: # # Internal Error (/scratch/rvraghav/build/jdk-jdk/open/src/hotspot/share/opto/memnode.cpp:4156), pid=127678, tid=127693 # assert(!do_zeroing || zeroes_done >= next_init_off) failed: don't miss any # ...
12-09-2018

We don't need to do it in complete_stores() because zeroing stores are aligned. But the store itself should be marked. Note, the original failure is reported for JDK 8 where we did not have C2_UNALIGNED. JDK 8 also does not have putIntUnaligned() I think. An other fix could be to not collect unaligned stores in Initialize node - can_capture_store() should return false for it. It could be simpler.
11-09-2018

Vladimir, it looks like your fix only handles initialization. Wouldn't we also need to set C2_UNALIGNED like inline_unsafe_access() does?
11-09-2018

Or we can test my suggested fix which should handle unaligned stores: diff -r b9f6a4427da9 src/hotspot/share/opto/memnode.cpp --- a/src/hotspot/share/opto/memnode.cpp +++ b/src/hotspot/share/opto/memnode.cpp @@ -4095,10 +4095,11 @@ // See if this store needs a zero before it or under it. intptr_t zeroes_needed = st_off; - if (st_size < BytesPerInt) { + if (st_size < BytesPerInt || (zeroes_needed % BytesPerInt) != 0) { // Look for subword stores which only partially initialize words. // If we find some, we must lay down some word-level zeroes first, // underneath the subword stores. + // Do the same for unaligned stores. // // Examples: // byte[] a = { p,q,r,s } => a[0]=p,a[1]=q,a[2]=r,a[3]=s
11-09-2018

Shouldn't this code be using jdk.internal.misc.Unsafe.putIntUnaligned()? I think we can close this as "Not an issue".
11-09-2018

-- 8202414 test failure issue reproducible as reported using 8u141, 8u162 product builds. -- Failure issue not reproducible for me with jdk9, jdk10, latest source product builds. -- Got the assert failure - assert((end_offset % BytesPerInt) == 0) failed: odd end offset with latest source fastdebug build. e.g.: using 11-ea+11 fastdebug build - $ ..jdk-11/fastdebug/bin/javac JvmTest.java JvmTest.java:1: warning: Unsafe is internal proprietary API and may be removed in a future release import sun.misc.Unsafe; ^ JvmTest.java:39: warning: Unsafe is internal proprietary API and may be removed in a future release static final Unsafe THE_UNSAFE; ^ JvmTest.java:42: warning: Unsafe is internal proprietary API and may be removed in a future release THE_UNSAFE = (Unsafe) AccessController.doPrivileged( ^ JvmTest.java:47: warning: Unsafe is internal proprietary API and may be removed in a future release Field f = Unsafe.class.getDeclaredField("theUnsafe"); ^ 4 warnings $ ..jdk-11/fastdebug/bin/java JvmTest # To suppress the following error report, specify this argument # after -XX: or in .hotspotrc: SuppressErrorAt=/memnode.cpp:2902 # # A fatal error has been detected by the Java Runtime Environment: # # Internal Error (/scratch/opt/mach5/mesos/work_dir/slaves/f8c08d6c-24c9-4e26-9e53-04187d2df052-S530/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/f8ba0516-f8f7-4902-864b-3ad137f0a673/runs/15d3f796-1174-493a-bb84-f013e1546093/workspace/open/src/hotspot/share/opto/memnode.cpp:2902), pid=58248, tid=58259 # assert((end_offset % BytesPerInt) == 0) failed: odd end offset # # JRE version: Java(TM) SE Runtime Environment (11.0+11) (fastdebug build 11-ea+11) # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 11-ea+11, mixed mode, tiered, compressed oops, g1 gc, linux-amd64) # Core dump will be written. Default location: Core dumps may be processed with "/usr/libexec/abrt-hook-ccpp %s %c %p %u %g %t e" (or dumping to /scratch/rvraghav/temp/test/core.58248) # # An error report file with more information is saved as: # /scratch/rvraghav/temp/test/hs_err_pid58248.log # # Compiler replay data is saved as: # /scratch/rvraghav/temp/test/replay_pid58248.log # # If you would like to submit a bug report, please visit: # http://bugreport.java.com/bugreport/crash.jsp # Current thread is 58259 Dumping core ... Aborted (core dumped) ----------------------- Current thread (0x00007fea082da800): JavaThread "C2 CompilerThread0" daemon [_thread_in_native, id=58259, stack(0x00007fe9e036c000,0x00007fe9e046d000)] Current CompileTask: C2: 578 230 4 JvmTest::test (40 bytes) Stack: [0x00007fe9e036c000,0x00007fe9e046d000], sp=0x00007fe9e0467640, free space=1005k Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code) V [libjvm.so+0x184b56f] VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*, int, unsigned long)+0x25f V [libjvm.so+0x184c32f] VMError::report_and_die(Thread*, void*, char const*, int, char const*, char const*, __va_list_tag*)+0x2f V [libjvm.so+0xade400] report_vm_error(char const*, int, char const*, char const*, ...)+0x100 V [libjvm.so+0x1353b27] ClearArrayNode::clear_memory(Node*, Node*, Node*, long, long, PhaseGVN*) [clone .part.174]+0x1f7 V [libjvm.so+0x135a56c] InitializeNode::complete_stores(Node*, Node*, Node*, long, Node*, PhaseGVN*)+0x4cc V [libjvm.so+0x1288452] PhaseMacroExpand::initialize_object(AllocateNode*, Node*, Node*, Node*, Node*, Node*, Node*)+0x422 V [libjvm.so+0x128991f] PhaseMacroExpand::expand_allocate_common(AllocateNode*, Node*, TypeFunc const*, unsigned char*)+0x6af V [libjvm.so+0x128eea7] PhaseMacroExpand::expand_macro_nodes()+0xcb7 V [libjvm.so+0xa3c32d] Compile::Optimize()+0xc6d V [libjvm.so+0xa3d564] Compile::Compile(ciEnv*, C2Compiler*, ciMethod*, int, bool, bool, bool, DirectiveSet*)+0xf94 V [libjvm.so+0x82e471] C2Compiler::compile_method(ciEnv*, ciMethod*, int, DirectiveSet*)+0xd1 V [libjvm.so+0xa4ac43] CompileBroker::invoke_compiler_on_method(CompileTask*)+0x413 V [libjvm.so+0xa4bc17] CompileBroker::compiler_thread_loop()+0x317 V [libjvm.so+0x1796207] JavaThread::thread_main_inner()+0x2c7 V [libjvm.so+0x179655f] JavaThread::run()+0x24f V [libjvm.so+0x14a9e00] thread_native_entry(Thread*)+0x100 -------------- Will attach the error, replay log files. Assert failure reproducible with the replay file. $ ../jdk-11/fastdebug/bin/java -XX:+ReplayCompiles -XX:+ReplayIgnoreInitErrors -XX:ReplayDataFile=replay_pid58248.log ----------------- >> 1. "sun.misc.Unsafe;" are intended for internal API purpose, Yes, but should fix the crash. >>2. Not sure putInt is the right one to use here. (putByte could be one option) Though no failure with putByte usage in test, root cause is hidden. Error exists by keeping putInt in tests and changing all byte usages to int. ----------------- ILW = Assert failure in ClearArrayNode::clear_memory, corrupted array length; rare!, test case with Unsafe internal proprietary API usages, worng array length reproducible for now with JDK 8u only; none! / avoid sun.misc.Unsafe usages = HLM = P3
03-05-2018

I am not able to reproduce this issue, but there are couple of things to note 1. "sun.misc.Unsafe;" are intended for internal API purpose, 2. Not sure putInt is the right one to use here. (putByte could be one option)
30-04-2018