[OpenJDK 2D-Dev] JNI crashes in FontManager code

Martin Buchholz martinrb at google.com
Sun Oct 19 20:46:13 UTC 2008


Hi Igor,

Caveat: I am not a 2d-developer, and am unfamiliar with the API,
and did not write the fix (but I did review it) or the test case.

I was hoping it would be easy for you font wranglers to easily
see how a crash could be triggered.  Or at least much easier
than for myself.   Given the P2 JDK crashing priority of the
bug, 2d maintainers should be highly motivated to create a
proper test case.

It seems to me initNativeScaler will store a FTScalerInfo,
containing a JNIEnv, and also stores a callback to CloseTTFontFileFunc,
which will be invoked later from another thread, which will then access
the JNIEnv, causing a possible crash.

Clearer seems the missing NewGlobalRef.  The reference obviously
outlives the lifetime of the native method.

Martin

On Sat, Oct 18, 2008 at 01:07, Igor Nekrestyanov
<Igor.Nekrestyanov at sun.com> wrote:
> Hello Martin,
>
> Suggested changes seems reasonable.
> However, i failed to invent testcase to reproduce this issue.
> Could you please describe what google test is doing in regard to the font
> code?
>
> It first glance it seems that the only way to get into freetypeScaler.c is
> through synchronized methods and
> env variable gets overridden every time. So, it seems that it should match
> current thread always.
> I am interested to understand what i am missing here.
>
> -igor
>
> Martin Buchholz wrote:
>>
>> Hi font maintainers,
>>
>> This is a bug report, with fix,
>> provided by Hiroshi Yamauchi.
>> This is a crash detected by a google test
>> (which is unfortunately impractical to share)
>> which makes this at least a P2 bug.
>>
>> Description:
>>
>>        Fixing the bad JNI code in the font manager code. Two issues:
>>
>>          o The JNIEnv is unique to the thread. It cannot be saved by one
>> thread and
>>            reused by another. Use GetEnv instead.
>>
>>          o The 'font2D' jobject needs to be converted into a global
>> reference because
>>            its lifetime exceeds the lifetime of a native method call.
>>
>> 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: yamauchi at google.com
>>
>> 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);
>>
>
>



More information about the 2d-dev mailing list