JDK-6761791 : Crash in the FontManager code due to use of JNIEnv saved by another thread
  • Type: Bug
  • Component: client-libs
  • Sub-Component: 2d
  • Affected Version: 7
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2008-10-21
  • Updated: 2011-05-18
  • Resolved: 2011-05-18
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 7 Other
7 b54Fixed OpenJDK6Fixed
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.
17-12-2008
SUGGESTED FIX Actual fix is http://sa.sfbay.sun.com/projects/java2d_data/7/6761791.0
17-12-2008
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);
21-10-2008