Bug ID: JDK-8220496 Race in java_lang_String::length() when deduplicating

JDK-8220496 : Race in java_lang_String::length() when deduplicating

Type: Bug
Component: hotspot
Sub-Component: runtime
Affected Version: 13

Priority: P4
Status: Resolved
Resolution: Fixed
OS: generic
CPU: generic

Submitted: 2019-03-12
Updated: 2019-08-15
Resolved: 2019-03-13

Versions (Unresolved/Resolved/Fixed)

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

Related Reports

Relates :

JDK-8217442 - Optimize native accesses to String.value

Description

Shenandoah CI reports an assertion failure:

#  Internal Error (/home/jenkins/workspace/nightly/shenandoah-jdk/src/hotspot/share/classfile/javaClasses.inline.hpp:78), pid=22355, tid=22358
#  assert(oopDesc::equals(value, java_lang_String::value(java_string))) failed: value must be same as java_lang_String::value(java_string)

This seems to be caused by String.value changing concurrently caused by String deduplication.

The call stack is:

Stack: [0x00002b41db723000,0x00002b41db824000],  sp=0x00002b41db821120,  free space=1016k
Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0xd68c5c]  java_lang_String::length(oop, typeArrayOop)+0x32c
V  [libjvm.so+0xe83801]  jni_GetStringLength+0x2b1
C  [libjava.so+0xd6d8]  getUTF+0x38
C  [libjava.so+0xd9b0]  Java_java_lang_ClassLoader_defineClass1+0x100
j  java.lang.ClassLoader.defineClass1(Ljava/lang/ClassLoader;Ljava/lang/String;[BIILjava/security/ProtectionDomain;Ljava/lang/String;)Ljava/lang/Class;+0 java.base@13-internal
j  java.lang.ClassLoader.defineClass(Ljava/lang/String;[BIILjava/security/ProtectionDomain;)Ljava/lang/Class;+27 java.base@13-internal
j  java.security.SecureClassLoader.defineClass(Ljava/lang/String;[BIILjava/security/CodeSource;)Ljava/lang/Class;+12 java.base@13-internal
j  jdk.internal.loader.BuiltinClassLoader.defineClass(Ljava/lang/String;Ljdk/internal/loader/Resource;)Ljava/lang/Class;+117 java.base@13-internal
j  jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(Ljava/lang/String;)Ljava/lang/Class;+37 java.base@13-internal
j  jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(Ljava/lang/String;Z)Ljava/lang/Class;+111 java.base@13-internal
j  jdk.internal.loader.BuiltinClassLoader.loadClass(Ljava/lang/String;Z)Ljava/lang/Class;+3 java.base@13-internal
j  jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(Ljava/lang/String;Z)Ljava/lang/Class;+36 java.base@13-internal
j  java.lang.ClassLoader.loadClass(Ljava/lang/String;)Ljava/lang/Class;+3 java.base@13-internal
v  ~StubRoutines::call_stub
V  [libjvm.so+0xda13bd]  JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*, Thread*)+0x74d
V  [libjvm.so+0xd9ddfd]  JavaCalls::call_virtual(JavaValue*, Klass*, Symbol*, Symbol*, JavaCallArguments*, Thread*)+0x3bd
V  [libjvm.so+0xd9e115]  JavaCalls::call_virtual(JavaValue*, Handle, Klass*, Symbol*, Symbol*, Handle, Thread*)+0x115
V  [libjvm.so+0x16e5197]  SystemDictionary::load_instance_class(Symbol*, Handle, Thread*)+0x267
V  [libjvm.so+0x16e3ca4]  SystemDictionary::resolve_instance_class_or_null(Symbol*, Handle, Handle, Thread*)+0xc34
V  [libjvm.so+0x16e3ee2]  SystemDictionary::resolve_instance_class_or_null_helper(Symbol*, Handle, Handle, Thread*)+0xa2
V  [libjvm.so+0x16e5b0d]  SystemDictionary::resolve_or_fail(Symbol*, Handle, Handle, bool, Thread*)+0x4d
V  [libjvm.so+0xf015a2]  find_class_from_class_loader(JNIEnv_*, Symbol*, unsigned char, Handle, Handle, unsigned char, Thread*)+0x32
V  [libjvm.so+0xf0594f]  JVM_FindClassFromCaller+0x3af
C  [libjava.so+0xd560]  Java_java_lang_Class_forName0+0x140
j  java.lang.Class.forName0(Ljava/lang/String;ZLjava/lang/ClassLoader;Ljava/lang/Class;)Ljava/lang/Class;+0 java.base@13-internal
j  java.lang.Class.forName(Ljava/lang/String;ZLjava/lang/ClassLoader;)Ljava/lang/Class;+43 java.base@13-internal
j  sun.launcher.LauncherHelper.loadMainClass(ILjava/lang/String;)Ljava/lang/Class;+93 java.base@13-internal
j  sun.launcher.LauncherHelper.checkAndLoadMain(ZILjava/lang/String;)Ljava/lang/Class;+42 java.base@13-internal
v  ~StubRoutines::call_stub
V  [libjvm.so+0xda13bd]  JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*, Thread*)+0x74d
V  [libjvm.so+0xe62042]  jni_invoke_static(JNIEnv_*, JavaValue*, _jobject*, JNICallType, _jmethodID*, JNI_ArgumentPusher*, Thread*) [clone .isra.178]+0x3c2
V  [libjvm.so+0xe888c9]  jni_CallStaticObjectMethod+0x1f9
C  [libjli.so+0x494c]  JavaMain+0x7dc

Comments

Cleaned up and updated in-place. Running local tests and will submit a test job before RFR. Not sure a regression test is needed.
12-03-2019
Yes. Note there are also similar asserts in: java_lang_String::as_utf8_string(oop java_string, typeArrayOop value, char* buf, int buflen) java_lang_String::as_utf8_string(oop java_string, typeArrayOop value, int start, int len, char* buf, int buflen) ...and nit: newline here: 98 } 99 bool java_lang_String::values_equal(typeArrayOop str_value1, typeArrayOop str_value2) {
12-03-2019
Right, with risk of scope creep, we could consolidate and clean this up, and potentially improve performance a bit in java_lang_String::equals: http://cr.openjdk.java.net/~redestad/8220496/open.00/
12-03-2019
memcmp comparison is probably tad more efficient and does not involve Access barriers that would come with byte_at(i) calls. Maybe java_lang_String::equals should use memcmp to begin with :)
12-03-2019
Are we looking at the same code? This is what I see: bool java_lang_String::equals(oop str1, oop str2) { assert(str1->klass() == SystemDictionary::String_klass(), "must be java String"); assert(str2->klass() == SystemDictionary::String_klass(), "must be java String"); typeArrayOop value1 = java_lang_String::value_no_keepalive(str1); int length1 = java_lang_String::length(str1, value1); bool is_latin1 = java_lang_String::is_latin1(str1); typeArrayOop value2 = java_lang_String::value_no_keepalive(str2); int length2 = java_lang_String::length(str2, value2); bool is_latin2 = java_lang_String::is_latin1(str2); if ((length1 != length2) \|\| (is_latin1 != is_latin2)) { // Strings of different size or with different // coders are never equal. return false; } int blength1 = value1->length(); for (int i = 0; i < blength1; i++) { if (value1->byte_at(i) != value2->byte_at(i)) { return false; } } return true; }
12-03-2019
equals(oop, oop) does pointer comparison, and I think the assertion needs be loosened to replicate the logic in StringDedupTable::equals(typeArrayOop, typeArrayOop): bool StringDedupTable::equals(typeArrayOop value1, typeArrayOop value2) { return (oopDesc::equals(value1, value2) \|\| (value1->length() == value2->length() && (!memcmp(value1->base(T_BYTE), value2->base(T_BYTE), value1->length() * sizeof(jbyte))))); }
12-03-2019
Instead of comparing the pointers, we can probably compare the string values with java_lang_String::equals. It would be slower, but we don't care all too much for asserts?
12-03-2019
Seems to be related to JDK-8217442, assigning Claes.
12-03-2019