OSDN Git Service

Improve JNI indirect ref diagnostics
authorAndy McFadden <fadden@android.com>
Thu, 7 Apr 2011 17:36:56 +0000 (10:36 -0700)
committerAndy McFadden <fadden@android.com>
Thu, 7 Apr 2011 21:24:42 +0000 (14:24 -0700)
Improved CheckJNI problem detection and failure reporting.  Also,
the indirect reference table data structure now responds to invalid
requests with a magic bogus object instead of NULL, so that failures
are immediate and obvious.

The extended checks in the indirect ref implementation are still on,
mostly so that bad code fails in an obvious way even when CheckJNI
is not enabled.

This change also includes a hack to allow weak globals to work.
Returning non-NULL for invalid refs turned a rarely-fails-weirdly
issue into an always-fails-loudly, which is good, but we need to
paper over it until the WGR fix is in place.  This will need to be
removed later.

Also, reduced some log levels.

Bug 4184721

Change-Id: Iaec75c71dbc85934366be2e717329b635d0fa49e

vm/CheckJni.c
vm/IndirectRefTable.c
vm/IndirectRefTable.h
vm/Jni.c
vm/test/TestIndirectRefTable.c

index 5478eb1..17c76c8 100644 (file)
 
 #include <zlib.h>
 
-static void abortMaybe(void);       // fwd
+#define kUnknownFuncName "      -"      /* can pass to showLocation() */
+
+static void showLocation(const char* func);
+static void abortMaybe(void);
 
 
 /*
@@ -60,11 +63,19 @@ static void abortMaybe(void);       // fwd
  *
  * At this point, pResult->l has already been converted to an object pointer.
  */
-static void checkCallResultCommon(const u4* args, JValue* pResult,
+static void checkCallResultCommon(const u4* args, const JValue* pResult,
     const Method* method, Thread* self)
 {
     assert(pResult->l != NULL);
-    Object* resultObj = (Object*) pResult->l;
+    const Object* resultObj = (const Object*) pResult->l;
+
+    if (resultObj == kInvalidIndirectRefObject) {
+        LOGW("JNI WARNING: invalid reference returned from native code\n");
+        showLocation(kUnknownFuncName);
+        abortMaybe();
+        return;
+    }
+
     ClassObject* objClazz = resultObj->clazz;
 
     /*
@@ -117,8 +128,8 @@ static void checkCallResultCommon(const u4* args, JValue* pResult,
 /*
  * Determine if we need to check the return type coming out of the call.
  *
- * (We don't do this at the top of checkCallResultCommon() because this is on
- * the critical path for native method calls.)
+ * (We don't simply do this at the top of checkCallResultCommon() because
+ * this is on the critical path for native method calls.)
  */
 static inline bool callNeedsCheck(const u4* args, JValue* pResult,
     const Method* method, Thread* self)
@@ -227,12 +238,14 @@ void dvmCheckCallJNIMethod_staticNoRef(const u4* args, JValue* pResult,
 
 #define CHECK_FIELD_TYPE(_env, _obj, _fieldid, _prim, _isstatic)            \
     checkFieldType(_env, _obj, _fieldid, _prim, _isstatic, __FUNCTION__)
+#define CHECK_STATIC_FIELD_ID(_env, _clazz, _fieldid)                       \
+    checkStaticFieldID(_env, _clazz, _fieldid, __FUNCTION__)
 #define CHECK_INST_FIELD_ID(_env, _obj, _fieldid)                           \
     checkInstanceFieldID(_env, _obj, _fieldid, __FUNCTION__)
 #define CHECK_CLASS(_env, _clazz)                                           \
-    checkClass(_env, _clazz, __FUNCTION__)
+    checkInstance(_env, _clazz, gDvm.classJavaLangClass, "jclass", __FUNCTION__)
 #define CHECK_STRING(_env, _str)                                            \
-    checkString(_env, _str, __FUNCTION__)
+    checkInstance(_env, _str, gDvm.classJavaLangString, "jstring", __FUNCTION__)
 #define CHECK_UTF_STRING(_env, _str)                                        \
     checkUtfString(_env, _str, #_str, __FUNCTION__)
 #define CHECK_NULLABLE_UTF_STRING(_env, _str)                               \
@@ -281,10 +294,11 @@ void dvmCheckCallJNIMethod_staticNoRef(const u4* args, JValue* pResult,
  *
  * "func" looks like "Check_DeleteLocalRef"; we drop the "Check_".
  */
-static void showLocation(const Method* meth, const char* func)
+static void showLocation(const char* func)
 {
+    const Method* meth = dvmGetCurrentJNIMethod();
     char* desc = dexProtoCopyMethodDescriptor(&meth->prototype);
-    LOGW("             in %s.%s %s (%s)",
+    LOGW("             in %s.%s:%s (%s)",
         meth->clazz->descriptor, meth->name, desc, func + 6);
     free(desc);
 }
@@ -393,7 +407,7 @@ static void checkThread(JNIEnv* env, int flags, const char* func)
     }
 
     if (printWarn)
-        showLocation(dvmGetCurrentJNIMethod(), func);
+        showLocation(func);
     if (printException) {
         LOGW("Pending exception is:");
         dvmLogExceptionStackTrace();
@@ -423,7 +437,7 @@ static const char* primitiveTypeToName(PrimitiveType primType) {
 
 /*
  * Verify that the field is of the appropriate type.  If the field has an
- * object type, "obj" is the object we're trying to assign into it.
+ * object type, "jobj" is the object we're trying to assign into it.
  *
  * Works for both static and instance fields.
  */
@@ -434,14 +448,26 @@ static void checkFieldType(JNIEnv* env, jobject jobj, jfieldID fieldID,
     bool printWarn = false;
 
     if (fieldID == NULL) {
-        LOGE("JNI ERROR: null field ID");
+        LOGW("JNI WARNING: null field ID");
+        showLocation(func);
         abortMaybe();
     }
 
-    if (field->signature[0] == 'L' || field->signature[0] == '[') {
+    if ((field->signature[0] == 'L' || field->signature[0] == '[') &&
+        jobj != NULL)
+    {
         JNI_ENTER();
         Object* obj = dvmDecodeIndirectRef(env, jobj);
-        if (obj != NULL) {
+        /*
+         * If jobj is a weak global ref whose referent has been cleared,
+         * obj will be NULL.  Otherwise, obj should always be non-NULL
+         * and valid.
+         */
+        if (obj != NULL && !dvmIsValidObject(obj)) {
+            LOGW("JNI WARNING: field operation on invalid %s ref (%p)\n",
+                dvmIndirectRefTypeName(jobj), jobj);
+            printWarn = true;
+        } else {
             ClassObject* fieldClass =
                 dvmFindLoadedClass(field->signature);
             ClassObject* objClass = obj->clazz;
@@ -450,14 +476,14 @@ static void checkFieldType(JNIEnv* env, jobject jobj, jfieldID fieldID,
             assert(objClass != NULL);
 
             if (!dvmInstanceof(objClass, fieldClass)) {
-                LOGW("JNI WARNING: field '%s' with type '%s' set with wrong type (%s)",
+                LOGW("JNI WARNING: set field '%s' expected type %s, got %s",
                     field->name, field->signature, objClass->descriptor);
                 printWarn = true;
             }
         }
         JNI_EXIT();
     } else if (dexGetPrimitiveTypeFromDescriptorChar(field->signature[0]) != prim) {
-        LOGW("JNI WARNING: field '%s' with type '%s' set with wrong type (%s)",
+        LOGW("JNI WARNING: set field '%s' expected type %s, got %s",
             field->name, field->signature, primitiveTypeToName(prim));
         printWarn = true;
     } else if (isStatic && !dvmIsStaticField(field)) {
@@ -471,7 +497,7 @@ static void checkFieldType(JNIEnv* env, jobject jobj, jfieldID fieldID,
     }
 
     if (printWarn) {
-        showLocation(dvmGetCurrentJNIMethod(), func);
+        showLocation(func);
         abortMaybe();
     }
 }
@@ -480,122 +506,81 @@ static void checkFieldType(JNIEnv* env, jobject jobj, jfieldID fieldID,
  * Verify that "jobj" is a valid object, and that it's an object that JNI
  * is allowed to know about.  We allow NULL references.
  *
- * Must be in "running" mode before calling here.
+ * Switches to "running" mode before performing checks.
  */
-static void checkObject0(JNIEnv* env, jobject jobj, const char* func)
+static void checkObject(JNIEnv* env, jobject jobj, const char* func)
 {
-    UNUSED_PARAMETER(env);
     bool printWarn = false;
 
     if (jobj == NULL)
         return;
 
+    JNI_ENTER();
+
     if (dvmGetJNIRefType(env, jobj) == JNIInvalidRefType) {
-        LOGW("JNI WARNING: %p is not a valid JNI reference", jobj);
+        LOGW("JNI WARNING: %p is not a valid JNI reference (type=%s)",
+            jobj, dvmIndirectRefTypeName(jobj));
         printWarn = true;
     } else {
         Object* obj = dvmDecodeIndirectRef(env, jobj);
 
-        if (obj == NULL || !dvmIsValidObject(obj)) {
-            LOGW("JNI WARNING: native code passing in bad object %p %p (%s)",
-                jobj, obj, func);
+        /*
+         * The decoded object will be NULL if this is a weak global ref
+         * with a cleared referent.
+         */
+        if (obj == kInvalidIndirectRefObject ||
+            (obj != NULL && !dvmIsValidObject(obj)))
+        {
+            LOGW("JNI WARNING: native code passing in bad object %p %p",
+                jobj, obj);
             printWarn = true;
         }
     }
 
     if (printWarn) {
-        showLocation(dvmGetCurrentJNIMethod(), func);
+        showLocation(func);
         abortMaybe();
     }
-}
-
-/*
- * Verify that "jobj" is a valid object, and that it's an object that JNI
- * is allowed to know about.  We allow NULL references.
- *
- * Switches to "running" mode before performing checks.
- */
-static void checkObject(JNIEnv* env, jobject jobj, const char* func)
-{
-    JNI_ENTER();
-    checkObject0(env, jobj, func);
     JNI_EXIT();
 }
 
 /*
- * Verify that "clazz" actually points to a class object.  (Also performs
- * checkObject.)
- *
- * We probably don't need to identify where we're being called from,
- * because the VM is most likely about to crash and leave a core dump
- * if something is wrong.
+ * Verify that "jobj" is a valid non-NULL object reference, and points to
+ * an instance of expectedClass.
  *
  * Because we're looking at an object on the GC heap, we have to switch
  * to "running" mode before doing the checks.
  */
-static void checkClass(JNIEnv* env, jclass jclazz, const char* func)
+static void checkInstance(JNIEnv* env, jobject jobj,
+    ClassObject* expectedClass, const char* argName, const char* func)
 {
-    JNI_ENTER();
-    bool printWarn = false;
-
-    Object* obj = dvmDecodeIndirectRef(env, jclazz);
-
-    if (obj == NULL) {
-        LOGW("JNI WARNING: received null jclass");
-        printWarn = true;
-    } else if (!dvmIsValidObject(obj)) {
-        LOGW("JNI WARNING: jclass points to invalid object %p", obj);
-        printWarn = true;
-    } else if (obj->clazz != gDvm.classJavaLangClass) {
-        LOGW("JNI WARNING: jclass arg is not a Class reference "
-             "(%p is instance of %s)",
-            jclazz, obj->clazz->descriptor);
-        printWarn = true;
-    }
-    JNI_EXIT();
-
-    if (printWarn)
+    if (jobj == NULL) {
+        LOGW("JNI WARNING: received null %s", argName);
+        showLocation(func);
         abortMaybe();
-    else
-        checkObject(env, jclazz, func);
-}
+        return;
+    }
 
-/*
- * Verify that "str" is non-NULL and points to a String object.
- *
- * Since we're dealing with objects, switch to "running" mode.
- */
-static void checkString(JNIEnv* env, jstring jstr, const char* func)
-{
     JNI_ENTER();
     bool printWarn = false;
 
-    Object* obj = dvmDecodeIndirectRef(env, jstr);
+    Object* obj = dvmDecodeIndirectRef(env, jobj);
 
-    if (obj == NULL) {
-        LOGW("JNI WARNING: received null jstring (%s)", func);
+    if (!dvmIsValidObject(obj)) {
+        LOGW("JNI WARNING: %s is invalid %s ref (%p)", argName,
+            dvmIndirectRefTypeName(jobj), jobj);
         printWarn = true;
-    } else if (obj->clazz != gDvm.classJavaLangString) {
-        /*
-         * TODO: we probably should test dvmIsValidObject first, because
-         * this will crash if "obj" is non-null but pointing to an invalid
-         * memory region.  However, the "is valid" test is a little slow,
-         * we're doing it again over in checkObject().
-         */
-        if (dvmIsValidObject(obj))
-            LOGW("JNI WARNING: jstring %p points to object of type %s (%s)",
-                jstr, obj->clazz->descriptor, func);
-        else
-            LOGW("JNI WARNING: jstring %p is not a valid object (%s)", jstr,
-                func);
+    } else if (obj->clazz != expectedClass) {
+        LOGW("JNI WARNING: %s arg has wrong type (expected %s, got %s)",
+            argName, expectedClass->descriptor, obj->clazz->descriptor);
         printWarn = true;
     }
     JNI_EXIT();
 
-    if (printWarn)
+    if (printWarn) {
+        showLocation(func);
         abortMaybe();
-    else
-        checkObject(env, jstr, func);
+    }
 }
 
 /*
@@ -676,7 +661,7 @@ fail_with_string:
         errorKind, utf8);
     LOGW("             string: '%s'", origBytes);
 fail:
-    showLocation(dvmGetCurrentJNIMethod(), func);
+    showLocation(func);
     abortMaybe();
 }
 
@@ -710,29 +695,34 @@ static void checkClassName(JNIEnv* env, const char* className, const char* func)
  */
 static void checkArray(JNIEnv* env, jarray jarr, const char* func)
 {
+    if (jarr == NULL) {
+        LOGW("JNI WARNING: received null array");
+        showLocation(func);
+        abortMaybe();
+        return;
+    }
+
     JNI_ENTER();
     bool printWarn = false;
 
     Object* obj = dvmDecodeIndirectRef(env, jarr);
 
-    if (obj == NULL) {
-        LOGW("JNI WARNING: received null array (%s)", func);
+    if (!dvmIsValidObject(obj)) {
+        LOGW("JNI WARNING: jarray is invalid %s ref (%p)",
+            dvmIndirectRefTypeName(jarr), jarr);
         printWarn = true;
     } else if (obj->clazz->descriptor[0] != '[') {
-        if (dvmIsValidObject(obj))
-            LOGW("JNI WARNING: jarray %p points to non-array object (%s)",
-                jarr, obj->clazz->descriptor);
-        else
-            LOGW("JNI WARNING: jarray %p is not a valid object", jarr);
+        LOGW("JNI WARNING: jarray arg has wrong type (expected array, got %s)",
+            obj->clazz->descriptor);
         printWarn = true;
     }
 
     JNI_EXIT();
 
-    if (printWarn)
+    if (printWarn) {
+        showLocation(func);
         abortMaybe();
-    else
-        checkObject(env, jarr, func);
+    }
 }
 
 /*
@@ -796,15 +786,18 @@ static void checkSig(JNIEnv* env, jmethodID methodID, char expectedSigByte,
         LOGW("             calling %s.%s %s",
             meth->clazz->descriptor, meth->name, desc);
         free(desc);
-        showLocation(dvmGetCurrentJNIMethod(), func);
+        showLocation(func);
         abortMaybe();
     }
 }
 
 /*
  * Verify that this static field ID is valid for this class.
+ *
+ * Assumes "jclazz" has already been validated.
  */
-static void checkStaticFieldID(JNIEnv* env, jclass jclazz, jfieldID fieldID)
+static void checkStaticFieldID(JNIEnv* env, jclass jclazz, jfieldID fieldID,
+    const char* func)
 {
     JNI_ENTER();
     ClassObject* clazz = (ClassObject*) dvmDecodeIndirectRef(env, jclazz);
@@ -817,6 +810,7 @@ static void checkStaticFieldID(JNIEnv* env, jclass jclazz, jfieldID fieldID)
         LOGW("JNI WARNING: static fieldID %p not valid for class %s",
             fieldID, clazz->descriptor);
         LOGW("             base=%p count=%d", base, fieldCount);
+        showLocation(func);
         abortMaybe();
     }
     JNI_EXIT();
@@ -824,18 +818,14 @@ static void checkStaticFieldID(JNIEnv* env, jclass jclazz, jfieldID fieldID)
 
 /*
  * Verify that this instance field ID is valid for this object.
+ *
+ * Assumes "jobj" has already been validated.
  */
 static void checkInstanceFieldID(JNIEnv* env, jobject jobj, jfieldID fieldID,
     const char* func)
 {
     JNI_ENTER();
 
-    if (jobj == NULL) {
-        LOGW("JNI WARNING: invalid null object (%s)", func);
-        abortMaybe();
-        goto bail;
-    }
-
     Object* obj = dvmDecodeIndirectRef(env, jobj);
     ClassObject* clazz = obj->clazz;
 
@@ -855,6 +845,7 @@ static void checkInstanceFieldID(JNIEnv* env, jobject jobj, jfieldID fieldID,
 
     LOGW("JNI WARNING: inst fieldID %p not valid for class %s",
         fieldID, obj->clazz->descriptor);
+    showLocation(func);
     abortMaybe();
 
 bail:
@@ -879,6 +870,7 @@ static void checkVirtualMethod(JNIEnv* env, jobject jobj, jmethodID methodID,
     if (!dvmInstanceof(obj->clazz, meth->clazz)) {
         LOGW("JNI WARNING: can't call %s.%s on instance of %s",
             meth->clazz->descriptor, meth->name, obj->clazz->descriptor);
+        showLocation(func);
         abortMaybe();
     }
 
@@ -888,7 +880,7 @@ static void checkVirtualMethod(JNIEnv* env, jobject jobj, jmethodID methodID,
 /*
  * Verify that "methodID" is appropriate for "clazz".
  *
- * A mismatch isn't dangerous, because the method defines the class.  In
+ * A mismatch isn't dangerous, because the jmethodID defines the class.  In
  * fact, jclazz is unused in the implementation.  It's best if we don't
  * allow bad code in the system though.
  *
@@ -905,7 +897,8 @@ static void checkStaticMethod(JNIEnv* env, jclass jclazz, jmethodID methodID,
     if (!dvmInstanceof(clazz, meth->clazz)) {
         LOGW("JNI WARNING: can't call static %s.%s on class %s",
             meth->clazz->descriptor, meth->name, clazz->descriptor);
-        // no abort
+        showLocation(func);
+        // no abort?
     }
 
     JNI_EXIT();
@@ -1307,6 +1300,7 @@ static jint Check_Throw(JNIEnv* env, jthrowable obj)
 {
     CHECK_ENTER(env, kFlag_Default);
     CHECK_OBJECT(env, obj);
+    /* TODO: verify that "obj" is an instance of Throwable */
     jint result;
     result = BASE_ENV(env)->Throw(env, obj);
     CHECK_EXIT(env);
@@ -1576,7 +1570,7 @@ static jfieldID Check_GetStaticFieldID(JNIEnv* env, jclass clazz,
         CHECK_ENTER(env, kFlag_Default);                                    \
         CHECK_CLASS(env, clazz);                                            \
         _ctype result;                                                      \
-        checkStaticFieldID(env, clazz, fieldID);                            \
+        CHECK_STATIC_FIELD_ID(env, clazz, fieldID);                         \
         result = BASE_ENV(env)->GetStatic##_jname##Field(env, clazz,        \
             fieldID);                                                       \
         CHECK_EXIT(env);                                                    \
@@ -1598,7 +1592,7 @@ GET_STATIC_TYPE_FIELD(jdouble, Double);
     {                                                                       \
         CHECK_ENTER(env, kFlag_Default);                                    \
         CHECK_CLASS(env, clazz);                                            \
-        checkStaticFieldID(env, clazz, fieldID);                            \
+        CHECK_STATIC_FIELD_ID(env, clazz, fieldID);                         \
         /* "value" arg only used when type == ref */                        \
         CHECK_FIELD_TYPE(env, (jobject)(u4)value, fieldID, _ftype, true);   \
         BASE_ENV(env)->SetStatic##_jname##Field(env, clazz, fieldID,        \
index 81cfeed..819b15d 100644 (file)
@@ -173,7 +173,7 @@ IndirectRef dvmAddToIndirectRefTable(IndirectRefTable* pRef, u4 cookie,
                 pRef->allocEntries, newSize, pRef->maxEntries);
             return false;
         }
-        LOGI("Growing ireftab %p from %d to %d (max=%d)\n",
+        LOGV("Growing ireftab %p from %d to %d (max=%d)\n",
             pRef, pRef->allocEntries, newSize, pRef->maxEntries);
 
         /* update entries; adjust "nextEntry" in case memory moved */
@@ -230,19 +230,19 @@ bool dvmGetFromIndirectRefTableCheck(IndirectRefTable* pRef, IndirectRef iref)
     int idx = dvmIndirectRefToIndex(iref);
 
     if (iref == NULL) {
-        LOGI("--- lookup on NULL iref\n");
+        LOGD("Attempt to look up NULL iref\n");
         return false;
     }
     if (idx >= topIndex) {
         /* bad -- stale reference? */
-        LOGI("Attempt to access invalid index %d (top=%d)\n",
+        LOGD("Attempt to access invalid index %d (top=%d)\n",
             idx, topIndex);
         return false;
     }
 
     Object* obj = pRef->table[idx];
     if (obj == NULL) {
-        LOGI("Attempt to read from hole, iref=%p\n", iref);
+        LOGD("Attempt to read from hole, iref=%p\n", iref);
         return false;
     }
     if (!checkEntry(pRef, iref, idx))
@@ -285,7 +285,7 @@ bool dvmRemoveFromIndirectRefTable(IndirectRefTable* pRef, u4 cookie,
     }
     if (idx >= topIndex) {
         /* bad -- stale reference? */
-        LOGI("Attempt to remove invalid index %d (bottom=%d top=%d)\n",
+        LOGD("Attempt to remove invalid index %d (bottom=%d top=%d)\n",
             idx, bottomIndex, topIndex);
         return false;
     }
@@ -345,6 +345,20 @@ bool dvmRemoveFromIndirectRefTable(IndirectRefTable* pRef, u4 cookie,
 }
 
 /*
+ * Return a type name, useful for debugging.
+ */
+const char* dvmIndirectRefTypeName(IndirectRef iref)
+{
+    switch (dvmGetIndirectRefType(iref)) {
+    case kIndirectKindInvalid:      return "invalid";
+    case kIndirectKindLocal:        return "local";
+    case kIndirectKindGlobal:       return "global";
+    case kIndirectKindWeakGlobal:   return "weak global";
+    default:                        return "UNKNOWN";
+    }
+}
+
+/*
  * Dump the contents of a IndirectRefTable to the log.
  */
 void dvmDumpIndirectRefTable(const IndirectRefTable* pRef, const char* descr)
index c33a509..7248a7b 100644 (file)
@@ -240,6 +240,11 @@ INLINE IndirectRefKind dvmGetIndirectRefType(IndirectRef iref)
 }
 
 /*
+ * Return a string constant describing the indirect ref type.
+ */
+const char* dvmIndirectRefTypeName(IndirectRef iref);
+
+/*
  * Initialize an IndirectRefTable.
  *
  * If "initialCount" != "maxCount", the table will expand as required.
@@ -347,16 +352,19 @@ INLINE IndirectRef dvmAppendToIndirectRefTable(IndirectRefTable* pRef,
 /* extra debugging checks */
 bool dvmGetFromIndirectRefTableCheck(IndirectRefTable* pRef, IndirectRef iref);
 
+/* magic failure value; must not pass dvmIsValidObject() */
+#define kInvalidIndirectRefObject ((Object*)0xdead4321)
+
 /*
  * Given an IndirectRef in the table, return the Object it refers to.
  *
- * Returns NULL if iref is invalid.
+ * Returns kInvalidIndirectRefObject if iref is invalid.
  */
 INLINE Object* dvmGetFromIndirectRefTable(IndirectRefTable* pRef,
     IndirectRef iref)
 {
     if (!dvmGetFromIndirectRefTableCheck(pRef, iref))
-        return NULL;
+        return kInvalidIndirectRefObject;
 
     int idx = dvmIndirectRefToIndex(iref);
     return pRef->table[idx];
index dab94bb..6f96132 100644 (file)
--- a/vm/Jni.c
+++ b/vm/Jni.c
@@ -448,7 +448,9 @@ static inline IndirectRefTable* getLocalRefTable(JNIEnv* env)
  * Convert an indirect reference to an Object reference.  The indirect
  * reference may be local, global, or weak-global.
  *
- * If "jobj" is NULL or an invalid indirect reference, this returns NULL.
+ * If "jobj" is NULL, or is a weak global reference whose reference has
+ * been cleared, this returns NULL.  If jobj is an invalid indirect
+ * reference, kInvalidIndirectRefObject is returned.
  */
 Object* dvmDecodeIndirectRef(JNIEnv* env, jobject jobj)
 {
@@ -480,13 +482,25 @@ Object* dvmDecodeIndirectRef(JNIEnv* env, jobject jobj)
             dvmLockMutex(&gDvm.jniWeakGlobalRefLock);
             result = dvmGetFromIndirectRefTable(pRefTable, jobj);
             dvmUnlockMutex(&gDvm.jniWeakGlobalRefLock);
+            /*
+             * TODO: this is a temporary workaround for broken weak global
+             * refs (bug 4260055).  We treat any invalid reference as if it
+             * were a weak global with a cleared referent.  This means that
+             * actual invalid references won't be detected, and if an empty
+             * slot gets re-used we will return the new reference instead.
+             * This must be removed when weak global refs get fixed.
+             */
+            if (result == kInvalidIndirectRefObject) {
+                LOGW("Warning: used weak global ref hack\n");
+                result = NULL;
+            }
         }
         break;
     case kIndirectKindInvalid:
     default:
         LOGW("Invalid indirect reference %p in decodeIndirectRef\n", jobj);
         dvmAbort();
-        result = NULL;
+        result = kInvalidIndirectRefObject;
         break;
     }
 
@@ -815,10 +829,6 @@ void dvmDumpJniReferenceTables(void)
  *  - is present in the JNI global refs table
  *
  * Used by -Xcheck:jni and GetObjectRefType.
- *
- * NOTE: in the current VM, global and local references are identical.  If
- * something is both global and local, we can't tell them apart, and always
- * return "local".
  */
 jobjectRefType dvmGetJNIRefType(JNIEnv* env, jobject jobj)
 {
@@ -827,10 +837,11 @@ jobjectRefType dvmGetJNIRefType(JNIEnv* env, jobject jobj)
      * jobjectRefType, so this is easy.  We have to decode it to determine
      * if it's a valid reference and not merely valid-looking.
      */
+    assert(jobj != NULL);
+
     Object* obj = dvmDecodeIndirectRef(env, jobj);
 
-    if (obj == NULL) {
-        /* invalid ref, or jobj was NULL */
+    if (obj == kInvalidIndirectRefObject) {
         return JNIInvalidRefType;
     } else {
         return (jobjectRefType) dvmGetIndirectRefType(jobj);
@@ -1205,6 +1216,9 @@ static void checkStackSum(Thread* self)
 /*
  * If necessary, convert the value in pResult from a local/global reference
  * to an object pointer.
+ *
+ * If the returned reference is invalid, kInvalidIndirectRefObject will
+ * be returned in pResult.
  */
 static inline void convertReferenceResult(JNIEnv* env, JValue* pResult,
     const Method* method, Thread* self)
@@ -1505,11 +1519,10 @@ bail:
 static jclass GetSuperclass(JNIEnv* env, jclass jclazz)
 {
     JNI_ENTER();
-    jclass jsuper = NULL;
 
     ClassObject* clazz = (ClassObject*) dvmDecodeIndirectRef(env, jclazz);
-    if (clazz != NULL)
-        jsuper = addLocalReference(env, (Object*)clazz->super);
+    jclass jsuper = addLocalReference(env, (Object*)clazz->super);
+
     JNI_EXIT();
     return jsuper;
 }
@@ -2688,14 +2701,14 @@ static jobjectArray NewObjectArray(JNIEnv* env, jsize length,
     JNI_ENTER();
 
     jobjectArray newArray = NULL;
-    ClassObject* elemClassObj =
-        (ClassObject*) dvmDecodeIndirectRef(env, jelementClass);
 
-    if (elemClassObj == NULL) {
-        dvmThrowNullPointerException("JNI NewObjectArray");
+    if (jelementClass == NULL) {
+        dvmThrowNullPointerException("JNI NewObjectArray element class");
         goto bail;
     }
 
+    ClassObject* elemClassObj =
+        (ClassObject*) dvmDecodeIndirectRef(env, jelementClass);
     ArrayObject* newObj =
         dvmAllocObjectArray(elemClassObj, length, ALLOC_DEFAULT);
     if (newObj == NULL) {
index 25f1dd1..155ba32 100644 (file)
@@ -95,7 +95,7 @@ static bool basicTest(void)
     }
 
     /* get invalid entry (off the end of the list) */
-    if (dvmGetFromIndirectRefTable(&irt, iref0) != NULL) {
+    if (dvmGetFromIndirectRefTable(&irt, iref0) != kInvalidIndirectRefObject) {
         LOGE("stale entry get succeeded unexpectedly\n");
         goto bail;
     }
@@ -153,7 +153,7 @@ static bool basicTest(void)
     }
 
     /* get invalid entry (from hole) */
-    if (dvmGetFromIndirectRefTable(&irt, iref1) != NULL) {
+    if (dvmGetFromIndirectRefTable(&irt, iref1) != kInvalidIndirectRefObject) {
         LOGE("hole get succeeded unexpectedly\n");
         goto bail;
     }