Race Condition in initializeEncoding() function in jni_util.c

[Ronald Servant]

Hi,

While building the J9 JVM for the IBM SDK for Java(*), we've run across
what we believe is a race condition in initializeEncoding(JNIEnv*) in
jni_util.c.

      (*)  The IBM SDK for Java is comprised of the J9 JVM and OpenJDK
class library.
We've encountered a situation where (*env)->CallObjectMethod() in
JNU_GetStringPlatformChars() is invoked with an uninitialized method ID
(the static variable "String_getBytes_ID").

The function initializeEnconding() initializes 4 static variables, but only
one of them ("fastEncoding") is checked to see if initializeEnconding()
should be called in JNU_NewStringPlatform() and JNU_GetStringPlatformChars
().  Further, "fastEncoding" is the first of the 4 static variables set by
initializeEncoding() leaving a race condition where another thread sees
"fastEncoding" set but "String_getBytes_ID" is not yet initialized.

I believe that moving the initialization of "String_getBytes_ID" and
"String_init_ID" to the top of the function would eliminate the impact of
the race condition.

I'd also recommend moving the initialization of "jnuEncoding" up one line,
but this may lead to leaking a global ref.

We are able to reproduce the failure in JNU_GetStringPlatformChars() due to
using an null String_getBytes_ID variable for a method id race condition in
our environment 1 in 20.  With the following patch applied, the race
condition can no longer be reproduced.

Do you agree there is a race condition?  If so, would the patch be an
appropriate fix?  We could introduce locking, but that feels like
overkill.

Ron.

diff -r efa71dc820eb src/java.base/share/native/libjava/jni_util.c
--- a/src/java.base/share/native/libjava/jni_util.c    Mon Nov 07 13:10:42
2016 -0400
+++ b/src/java.base/share/native/libjava/jni_util.c    Wed Nov 09 17:19:16
2016 -0500
@@ -688,6 +688,15 @@

     strClazz = JNU_ClassString(env);
     CHECK_NULL(strClazz);
+
+    /* Initialize method-id cache */
+    String_getBytes_ID = (*env)->GetMethodID(env, strClazz,
+                                             "getBytes",
"(Ljava/lang/String;)[B");
+    CHECK_NULL(String_getBytes_ID);
+    String_init_ID = (*env)->GetMethodID(env, strClazz,
+                                         "<init>",
"([BLjava/lang/String;)V");
+    CHECK_NULL(String_init_ID);
+

     propname = (*env)->NewStringUTF(env, "sun.jnu.encoding");
     if (propname) {
@@ -727,8 +736,8 @@
                              strcmp(encname, "utf-16le") == 0)
                         fastEncoding = FAST_CP1252;
                     else {
+                        jnuEncoding = (jstring)(*env)->NewGlobalRef(env,
enc);
                         fastEncoding = NO_FAST_ENCODING;
-                        jnuEncoding = (jstring)(*env)->NewGlobalRef(env,
enc);
                     }
                     (*env)->ReleaseStringUTFChars(env, enc, encname);
                 }
@@ -741,13 +750,6 @@
     }
     (*env)->DeleteLocalRef(env, propname);
     (*env)->DeleteLocalRef(env, enc);
-
-    /* Initialize method-id cache */
-    String_getBytes_ID = (*env)->GetMethodID(env, strClazz,
-                                             "getBytes",
"(Ljava/lang/String;)[B");
-    CHECK_NULL(String_getBytes_ID);
-    String_init_ID = (*env)->GetMethodID(env, strClazz,
-                                         "<init>",
"([BLjava/lang/String;)V");
 }