United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
JDK-6761791 : Crash in the FontManager code due to use of JNIEnv saved by another thread

Details
Type:
Bug
Submit Date:
2008-10-21
Status:
Closed
Updated Date:
2011-05-18
Project Name:
JDK
Resolved Date:
2011-05-18
Component:
client-libs
OS:
generic
Sub-Component:
2d
CPU:
generic
Priority:
P3
Resolution:
Fixed
Affected Versions:
7
Fixed Versions:

Related Reports
Backport:

Sub Tasks

Description
JNI code in the font manager caches JNIEnv and it may be saved by one thread and reused 
by another. This may cause crash.
	
The 'font2D' jobject needs to be converted into a global reference because its lifetime exceeds the lifetime of a native method call.

This is applicable at least to Freetype glue code.

Here is sample stacktrace from the crash:

Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x298366]
C  [libfontmanager.so+0x59e4]
C  [libfreetype.so.6+0x73c9]  FT_Stream_Close+0x19
C  [libfreetype.so.6+0xa065]  FT_Stream_Free+0x25
C  [libfreetype.so.6+0xa6e2]
C  [libfreetype.so.6+0xaf78]  FT_Done_Face+0x78
C  [libfontmanager.so+0x6964]
Java_sun_font_FreetypeFontScaler_disposeNativeScaler+0x34

                                    

Comments
EVALUATION

I do not see how multithread usage may cause problems per se as all relevant (non private, and not methods dealing with scalercontext lifecycle) FreetypeScaler methods are synchronized. 

However, freetype scaler does not always update cached JNIEnv/font2d values but calling freetype functions may cause callbacks and actual attempt to use cached values. I've added requests to update cached values in 2 identified places (and lokking through the code it seems all over cases are covered).

Proposed fix is in line with what we do for T2K.

Note that fix proposed on openjdk mailing list suggests to make reference to font2d object global. However, it does not seem necessary as it is not possible to save it in one thread and use on another thread for the same reasons - all methods are synchronized and we update cached reference every time native scaler is called.
                                     
2008-12-17
SUGGESTED FIX

Actual fix is 
  http://sa.sfbay.sun.com/projects/java2d_data/7/6761791.0
                                     
2008-12-17
SUGGESTED FIX

Contributed suggested fix:

Evaluation:

Appropriately register/free everything with the garbage collector.

Fix:

# HG changeset patch
# User martin
# Date 1224202830 25200
# Node ID 3c9d6001d8a90698a3540a2a483717f26a98db78
# Parent  68730f05449cd4f39ce1cb82adc6c4e57f87554f
Crash in freetypeScaler.c due to insufficient GC protection
Summary: NewGlobalRef/DeleteGlobalRef as needed.
Reviewed-by:
Contributed-by: ###@###.###

diff --git a/make/sun/font/mapfile-vers.openjdk
b/make/sun/font/mapfile-vers.openjdk
--- a/make/sun/font/mapfile-vers.openjdk
+++ b/make/sun/font/mapfile-vers.openjdk
@@ -29,6 +29,7 @@

 SUNWprivate_1.1 {
 	global:
+                JNI_OnLoad;
                 getSunFontIDs;
                 newLayoutTableCache;
                 freeLayoutTableCache;
diff --git a/src/share/native/sun/font/freetypeScaler.c
b/src/share/native/sun/font/freetypeScaler.c
--- a/src/share/native/sun/font/freetypeScaler.c
+++ b/src/share/native/sun/font/freetypeScaler.c
@@ -48,16 +48,6 @@
 #define  ROUND(x) ((int) (x+0.5))

 typedef struct {
-    /* Important note:
-         JNI forbids sharing same env between different threads.
-         We are safe, because pointer is overwritten every time we get into
-         JNI call (see setupFTContext).
-
-         Pointer is used by font data reading callbacks
-         such as ReadTTFontFileFunc.
-
-         NB: We may consider switching to JNI_GetEnv. */
-    JNIEnv* env;
     FT_Library library;
     FT_Face face;
     jobject font2D;
@@ -90,6 +80,13 @@
 void z_error(char *s) {}
 #endif

+static JavaVM* jvm = NULL;
+
+JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved) {
+    jvm = vm;
+    return JNI_VERSION_1_2;
+}
+
 /**************** Error handling utilities *****************/

 static jmethodID invalidateScalerMID;
@@ -107,6 +104,10 @@

     FT_Done_Face(scalerInfo->face);
     FT_Done_FreeType(scalerInfo->library);
+
+    if (scalerInfo->font2D != NULL) {
+        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
+    }

     if (scalerInfo->directBuffer != NULL) {
         (*env)->DeleteGlobalRef(env, scalerInfo->directBuffer);
@@ -131,10 +132,9 @@

 #define FILEDATACACHESIZE 1024

-/* NB: is it ever called? */
 static void CloseTTFontFileFunc(FT_Stream stream) {
+    JNIEnv* env = (JNIEnv*) JNU_GetEnv(jvm, JNI_VERSION_1_2);
     FTScalerInfo *scalerInfo = (FTScalerInfo *) stream->pathname.pointer;
-    JNIEnv* env = scalerInfo->env;
     jclass tmpClass = (*env)->FindClass(env, "sun/font/TrueTypeFont");
     jfieldID platNameField =
          (*env)->GetFieldID(env, tmpClass, "platName", "Ljava/lang/String;");
@@ -150,8 +150,8 @@
                                         unsigned char* destBuffer,
                                         unsigned long numBytes)
 {
+    JNIEnv* env = (JNIEnv*) JNU_GetEnv(jvm, JNI_VERSION_1_2);
     FTScalerInfo *scalerInfo = (FTScalerInfo *) stream->pathname.pointer;
-    JNIEnv* env = scalerInfo->env;
     jobject bBuffer;
     int bread = 0;

@@ -245,8 +245,7 @@
     if (scalerInfo == NULL)
         return 0;

-    scalerInfo->env = env;
-    scalerInfo->font2D = font2D;
+    scalerInfo->font2D = (*env)->NewGlobalRef(env, font2D);
     scalerInfo->fontDataOffset = 0;
     scalerInfo->fontDataLength = 0;
     scalerInfo->fileSize = filesize;
@@ -263,6 +262,7 @@
     */
     error = FT_Init_FreeType(&scalerInfo->library);
     if (error) {
+        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
         free(scalerInfo);
         return 0;
     }
@@ -331,6 +331,7 @@
         }
         if (scalerInfo->fontData != NULL)
             free(scalerInfo->fontData);
+        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
         free(scalerInfo);
         return 0;
     }
@@ -391,8 +392,10 @@
                           FTScalerContext *context) {
     int errCode = 0;

-    scalerInfo->env = env;
-    scalerInfo->font2D = font2D;
+    if (scalerInfo->font2D != NULL) {
+        (*env)->DeleteGlobalRef(env, scalerInfo->font2D);
+    }
+    scalerInfo->font2D = (*env)->NewGlobalRef(env, font2D);

     FT_Set_Transform(scalerInfo->face, &context->transform, NULL);
                                     
2008-10-21



Hardware and Software, Engineered to Work Together