Race Condition in initializeEncoding() function in jni_util.c

[David Holmes]

Hi Ron,

On 11/11/2016 1:01 AM, Ronald Servant wrote:
>
>
> 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.

I suspect there is an assumption of single-threaded initialization of 
the system classes, which would hit this code and so safely initialize 
the statics. There may be many such assumptions.

Reordering the statements may address your failure but does not make the 
code thread-safe, and you may encounter additional, more obscure race 
conditions.

David
-----

>
>       (*)  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");
>  }
>
>
>