Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

J9 Verification Issue: did not check uninitialized register? #9271

Closed
fuzzy000 opened this issue Apr 17, 2020 · 11 comments
Closed

J9 Verification Issue: did not check uninitialized register? #9271

fuzzy000 opened this issue Apr 17, 2020 · 11 comments

Comments

@fuzzy000
Copy link

Java -version output

The issue happened in following versions:

openjdk version "1.8.0_232"
OpenJDK Runtime Environment (build 1.8.0_232-b09)
Eclipse OpenJ9 VM (build openj9-0.17.0, JRE 1.8.0 Linux amd64-64-Bit Compressed References 20191017_442 (JIT enabled, AOT enabled)
OpenJ9   - 77c1cf708
OMR      - 20db4fbc
JCL      - 97b5ec8f383 based on jdk8u232-b09)
openjdk version "9.0.4-adoptopenjdk"
OpenJDK Runtime Environment (build 9.0.4-adoptopenjdk+12)
Eclipse OpenJ9 VM (build openj9-0.9.0, JRE 9 Linux amd64-64-Bit Compressed References 20180814_248 (JIT enabled, AOT enabled)
OpenJ9   - 24e53631
OMR      - fad6bf6e
JCL      - feec4d2ae based on jdk-9.0.4+12)
openjdk version "11.0.5" 2019-10-15
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.5+10)
Eclipse OpenJ9 VM AdoptOpenJDK (build openj9-0.17.0, JRE 11 Linux amd64-64-Bit Compressed References 20191016_358 (JIT enabled, AOT enabled)
OpenJ9   - 77c1cf708
OMR      - 20db4fbc
JCL      - 2a7af5674b based on jdk-11.0.5+10)

Summary of problem

We made some changes to a class file in the Apache Ant project (ant-launcher/org/apache/tools/ant/launch/Launcher.class), and let several JVMs run it. We found that compared to the corresponding versions of OpenJDKs, J9 may have Verify issues on the test case.

Take OpenJDK (8u232-b09) and J9 (8u232-b09) as an example:
OpenJDK (8u232-b09):

Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.VerifyError: (class: org/apache/tools/ant/launch/Launcher, method: getJarArray signature: ([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL;) Accessing value from uninitialized register 49
	at java.lang.Class.getDeclaredMethods0(Native Method)
	at java.lang.Class.privateGetDeclaredMethods(Class.java:2701)
	at java.lang.Class.privateGetMethodRecursive(Class.java:3048)
	at java.lang.Class.getMethod0(Class.java:3018)
	at java.lang.Class.getMethod(Class.java:1784)
	at sun.launcher.LauncherHelper.validateMainClass(LauncherHelper.java:544)
	at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:526)

J9 (8u232-b09):

Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.VerifyError: JVMVRFY012 stack shape inconsistent; class=org/apache/tools/ant/launch/Launcher, method=getJarArray([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL;, pc=849
Exception Details:
  Location:
    org/apache/tools/ant/launch/Launcher.getJarArray([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL; @849: JBaload
  Reason:
    Type top (current frame, locals[49]) is not assignable to 'reference' type
  Current Frame:
    bci: @849
    flags: { }
    locals: { integer, '[Ljava/net/URL;', '[Ljava/net/URL;', '[Ljava/net/URL;', 'java/io/File', integer, integer, top, top, top, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer, integer }
    stack: { }
	at java.lang.J9VMInternals.prepareClassImpl(Native Method)
	at java.lang.J9VMInternals.prepare(J9VMInternals.java:304)
	at java.lang.Class.getMethodHelper(Class.java:1243)
	at java.lang.Class.getMethod(Class.java:1187)
	at sun.launcher.LauncherHelper.validateMainClass(LauncherHelper.java:544)
	at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:526)

The point is, J9 reports Type top (current frame, locals[49]) is not assignable to 'reference' type, while OpenJDK reports Accessing value from uninitialized register 49. However, in the Current Frame printed by J9, there is only 48 element in locals. Since J9 did not check if the register is initialized, maybe it is a bug in J9?

Diagnostic files

antVerify1.zip
Steps to reproduce the behavior:

  1. extract the antVerify1.zip
  2. In ant directory, run command java -cp ant-launcher/ org.apache.tools.ant.launch.Launcher compile jar run

Execution environment

  • OS and version: Ubuntu 16.04.6 LTS
  • CPU model: Intel(R) Xeon(R) CPU E5-4610 v4 @ 1.80GHz
  • Number of CPU cores: 4CPUs, each has 10 cores
  • Size of physical memory: 16384 MB * 20
@DanHeidinga
Copy link
Member

@fuzzy000 Both OpenJ9 and Hotspot correctly identify that the code is invalid to run and throw a VerifyError. The error messages are different and the order of checks may be different as well (would need to look closer). There aren't any security implications here as neither JVM runs the invalid bytecode.

@ChengJin01 Can you reproduce this and dump the relevant method using an -Xdump:system:events=systhrow+throw,filter=java/lang/VerifyError and jdmpview?

Then we can take a look at the verifier code and validate the order of the checks.

@fuzzy000
Copy link
Author

Thanks for your kind attention!!

The reason why I think it is a defect is that in most other cases, these three versions of J9 and corresponding OpenJDK have consistent behaviors.

For example, if you try this case antVerify1-consistent.zip (the reproduce steps is the same), you will find that OpenJDK series report Register 17 contains wrong type, while J9 series report Type top (current frame, locals[17]) is not assignable to 'reference' type, which are consistent.

@ChengJin01
Copy link

ChengJin01 commented Apr 17, 2020

@DanHeidinga , The problem can be reproduced locally but jdmpview/DDR has got problem in decoding the specified class.


> !bytecodes 0x00000000026C4F98
  Name: getJarArray
  Signature: ([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL;
  Access Flags (1260002): private 
  Max Stack: 5
  Thrown Exceptions (1):
java/net/MalformedURLException  Argument Count: 5
  Temp Count: 47

    0 iconst5 
    1 istore 10
    3 iconst5 
...
  298 lookupswitch pairs 1
        default        637
              4Problem running command:
com.ibm.j9ddr.CorruptDataException: com.ibm.j9ddr.InvalidDataTypeException: UDATA contains value larger than Integer.MAX_VALUE

So I directly decoded the class file with javap as follows:
Launcher_class.txt
Launcher_getJarArray_bytecode.TXT

.../bin/javap  -v -p  ./ant-launcher/org/apache/tools/ant/launch/Launcher.class
...
  private java.net.URL[] getJarArray(java.net.URL[], java.net.URL[], java.net.URL[], java.io.File) throws java.net.MalformedURLException;
    descriptor: ([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL;
    flags: (0x0002) ACC_PRIVATE
    Code:
      stack=5, locals=52, args_size=5
   0: iconst_5
   1: istore        10
   3: iconst_5
   4: istore        11
   6: iconst_5
   7: istore        12
  ...
 111: istore        46
 113: iconst_1
       114: istore        47  <-------- locals[47] = 1
       116: iconst_1
       117: istore        6   <--------- locals[6] = 1
       119: aload_1
       120: arraylength  <------ length of the 1st argument
       121: istore        5   <--------- locals[6] =  length of the 1st argument
       123: ldc_w         #648                // String <org.apache.tools.ant.launch.Launcher: java.net.URL[] getJarArray(java.net.URL[],java.net.URL[],java.net.URL[],java.io.File)> **** Executed Line: **** 5 **** $i1 = lengthof r0
       126: invokestatic  #253                // Method Print.logPrint:(Ljava/lang/String;)V
       129: iinc          6, -1  <----- locals[6] - 1 = 1 -1 = 0
       132: iload         6   <------ locals[6] = 0;
       134: ifle   848  <------ jump to 848 if locals[6] <= 0
       137: aload_2
       138: arraylength  <------ length of the 2nd argument
 ...
 170: iload         7
 172: iload         8
 174: iadd
 175: istore        48  
<---- the length of the 1st,2nd and 3rd arguments is stored to locals[48])
...
 233: iload         48
 235: anewarray     #618  // class java/net/URL <----- new a java/net/URL array with length = locals[48]
 238: astore        49  <------ store the uninitialized java.net.URL[] to locals[49]

 240: ldc_w         #289                // String <org.apache.tools.ant.launch.Launcher: java.net.URL[] getJarArray(java.net.URL[],java.net.URL[],java.net.URL[],java.io.File)> **** Executed Line: **** 12 **** r4 = newarray (java.net.URL)[i13]
 243: invokestatic  #253                // Method Print.logPrint:(Ljava/lang/String;)V
 ...
 326: aload         49   <--- (1) OK: load java/net/URL for System.arraycopy
 328: iconst_0
 329: iload         9
 331: invokestatic  #967                // Method java/lang/System.arraycopy:(Ljava/lang/Object;ILjava/lang/Object;II)V
 334: ldc           #88                 // String <org.apache.tools.ant.launch.Launcher: java.net.URL[] getJarArray(java.net.URL[],java.net.URL[],java.net.URL[],java.io.File)> **** Executed Line: **** 14 **** staticinvoke <java.lang.System: void arraycopy(java.lang.Object,int,java.lang.Object,int,int)>(r0, 0, r4, 0, $i4)
 336: invokestatic  #253                // Method Print.logPrint:(Ljava/lang/String;)V
 ...
 470: aload         49   <--- (2) OK: load java/net/URL for System.arraycopy
 472: iload         50
 474: iload         51
 476: invokestatic  #967                // Method java/lang/System.arraycopy:(Ljava/lang/Object;ILjava/lang/Object;II)V
 479: ldc_w         #785                // String <org.apache.tools.ant.launch.Launcher: java.net.URL[] getJarArray(java.net.URL[],java.net.URL[],java.net.URL[],java.io.File)> **** Executed Line: **** 17 **** staticinvoke <java.lang.System: void arraycopy(java.lang.Object,int,java.lang.Object,int,int)>(r1, 0, r4, $i6, $i5)
 482: invokestatic  #253                // Method Print.logPrint:(Ljava/lang/String;)V
 ...
 615: aload         49  <--- (3) OK: load java/net/URL for System.arraycopy
 617: iload         50
 619: iload         51
 621: invokestatic  #967                // Method java/lang/System.arraycopy:(Ljava/lang/Object;ILjava/lang/Object;II)V
 624: ldc           #246                // String <org.apache.tools.ant.launch.Launcher: java.net.URL[] getJarArray(java.net.URL[],java.net.URL[],java.net.URL[],java.io.File)> **** Executed Line: **** 22 **** staticinvoke <java.lang.System: void arraycopy(java.lang.Object,int,java.lang.Object,int,int)>(r2, 0, r4, $i10, $i9)
 626: invokestatic  #253                // Method Print.logPrint:(Ljava/lang/String;)V
 ...
 648: aload         49
 650: arraylength   <-------- (4) OK: check the length of the newed array java.net.URL[]
 651: istore        50
 653: ldc_w         #368                // String <org.apache.tools.ant.launch.Launcher: java.net.URL[] getJarArray(java.net.URL[],java.net.URL[],java.net.URL[],java.io.File)> **** Executed Line: **** 24 **** $i11 = lengthof r4
 656: invokestatic  #253                // Method Print.logPrint:(Ljava/lang/String;)V
 ...
 828: aload         49    <--- (5) OK: load java/net/URL to store an element  java/net/URL
 830: iload         50  <--- length - 1 of the newed URL[]
 832: aload         51
 834: aastore   <----- store java/net/URL to URL[length - 1]
 835: ldc           #1                  // String <org.apache.tools.ant.launch.Launcher: java.net.URL[] getJarArray(java.net.URL[],java.net.URL[],java.net.URL[],java.io.File)> **** Executed Line: **** 27 **** r4[$i12] = $r5
 837: invokestatic  #253                // Method Print.logPrint:(Ljava/lang/String;)V
 ...
  848: nop   <------ jump here from 134 when locals[6]<= 0  (locals[6] = 0)
 849: aload    49  <----- NOK: (6) locals[49] is garbage data (never initialized) if jump from 134
 851: areturn    <----- return the garbage arra if jump from 134

according to the bytecode of the class file, locals[49] is never initialized at 848 if jump from 134 when locals[6] <= 0, in which case locals[49] is garbage data when loading at 849. Thus, the verifier correctly identified the wrong behaviour in this method.

The snaptrace also confirmed this at:

 j9bcverify(j9vm).75 Event       nextStack - org/apache/tools/ant/launch/Launcher 
    getJarArray([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL; 
    stackMapsCount = 22, nextMapIndex = 22, nextStackPC = 852 of 852
j9bcverify(j9vm).29 Exception  * verifyBytecodes - verifyError 0xc,
    method getJarArray([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL;, pc = 0x351
j9bcverify(j9vm).68 Exception  * verifyBytecodes - org/apache/tools/ant/launch/Launcher 
    getJarArray([Ljava/net/URL;[Ljava/net/URL;[Ljava/net/URL;Ljava/io/File;)[Ljava/net/URL; 
    error 0xc, at offset 849(0x351), bytecode = 0x19 <-----
j9bcverify(j9vm).28 Exception  * j9rtv_verifyBytecodes - verifyError 0xc, method 0x351
j9bcverify(j9vm).31 Exit       <j9rtv_verifyBytecodes
j9bcverify(j9vm).111 Exit       <j9bcv_verifyBytecodes - returning -1

From the error message framework, we already identified the failing location where the bytecode went wrong, which is more useful in trouleshooting verifier-related issues, as compared to the simplified output of Hotspot,

As for the output of reason message, this is a generic message of how we deal with any mismatch with the expected type via an unified message template, not specific to uninitialized object in this particular case. In addition, the number of variables in locals[] is part of our existing design which aimed not to print out any garbage data at the current index to avoid messing up the framework buffer.

If users prefer more specific/clear message in this case, we can as well adjust the framework there to meet the requirements.

@ChengJin01
Copy link

ChengJin01 commented Apr 17, 2020

Thanks for your kind attention!!

The reason why I think it is a defect is that in most other cases, these three versions of J9 and corresponding OpenJDK have consistent behaviors.

For example, if you try this case antVerify1-consistent.zip (the reproduce steps is the same), you will find that OpenJDK series report Register 17 contains wrong type, while J9 series report Type top (current frame, locals[17]) is not assignable to 'reference' type, which are consistent.

@fuzzy000, this is the way of how we deal with any mismatch with the expected type (via an unified message template), not specific to a particular verification error. The rule here is to avoid exactly match the output message from RI/Hotspot in each verification error.

@pshipton
Copy link
Member

@keithc-ca fyi the InvalidDataTypeException: UDATA contains value larger than Integer.MAX_VALUE which needs to be fixed in the DDR code.

@fuzzy000
Copy link
Author

Thanks for confirming that Hotspot's output is more useful in troubleshooting such verifier-related issues, I hope J9 can be refined in the future soon.

@ChengJin01
Copy link

ChengJin01 commented Apr 18, 2020

Thanks for confirming that Hotspot's output is more useful in troubleshooting such verifier-related issues, I hope J9 can be refined in the future soon.

@fuzzy000, sorry for any confusion(I updated my comment above). What I meant is our message is more straightforward to identify the failing location at the bytecode as compared to Hotspot's concise output without any location of bytecode.

@DanHeidinga, please confirm whether we need to add more info the Reason section to deal with these specific cases above.

@ChengJin01
Copy link

Already talked to @DanHeidinga, we won't move forward with this given that the verifier works good in identifying these kind of issues & generating the correct error message specific to the wrong bytecode at right place in these methods.

@DanHeidinga
Copy link
Member

Based on @ChengJin01's analysis, I agree. The OpenJ9 verifier is correctly detecting the issue and throwing the required VerifyError.

I'm going to close this issue now but @fuzzy000 if you (or anyone else) is interested in opening PRs to make OpenJ9's verifier messages clearer, we'd be open to helping and merging them.

@fuzzy000
Copy link
Author

Thank you all!
BTW, I opened another issue which may be a possible defect of J9. May you also check this issue J9 failed to throw the IllegalMonitorStateException? @DanHeidinga

@DanHeidinga
Copy link
Member

Thanks @fuzzy000. I'll try and get that looked at this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants