[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