Implement JNI UnregisterNatives.
This function is of limited use, but one of our partners found a need
for it, so it's finally getting implemented. It clears all JNI native
method bindings in a given class.
It turns out there are some interesting potential races which can be
avoided by treading lightly. The utility function that updates the
Method struct has been modified appropriately.
(There's a race on ARM SMP in the original implementation, in which
we could still observe "insns" as NULL after nativeFunc is pointing at
the JNI bridge. Handled in this change with an explicit full barrier,
which is overkill. We can make it better as part of the general
barrier-removal pass in bug 2781972 -- really wants an acquiring load
on nativeFunc, though that's a waste for internal native calls.)
Also, stifled an unnecessary warning about redefining a native method
implementation (which is explicitly allowed). Limited the scope of
an inappropriate "const" declaration.
Bug 2697885.
Change-Id: Ie406e25de3f77d00fe3e9130b406abcb45c3d60e
diff --git a/vm/Jni.c b/vm/Jni.c
index 9973508..b983ce2 100644
--- a/vm/Jni.c
+++ b/vm/Jni.c
@@ -1296,26 +1296,26 @@
if (method == NULL)
method = dvmFindVirtualMethodByDescriptor(clazz, methodName, signature);
if (method == NULL) {
- LOGW("ERROR: Unable to find decl for native %s.%s %s\n",
+ LOGW("ERROR: Unable to find decl for native %s.%s:%s\n",
clazz->descriptor, methodName, signature);
goto bail;
}
if (!dvmIsNativeMethod(method)) {
- LOGW("Unable to register: not native: %s.%s %s\n",
+ LOGW("Unable to register: not native: %s.%s:%s\n",
clazz->descriptor, methodName, signature);
goto bail;
}
if (method->nativeFunc != dvmResolveNativeMethod) {
- LOGW("Warning: %s.%s %s was already registered/resolved?\n",
+ /* this is allowed, but unusual */
+ LOGV("Note: %s.%s:%s was already registered\n",
clazz->descriptor, methodName, signature);
- /* keep going, I guess */
}
dvmUseJNIBridge(method, fnPtr);
- LOGV("JNI-registered %s.%s %s\n", clazz->descriptor, methodName,
+ LOGV("JNI-registered %s.%s:%s\n", clazz->descriptor, methodName,
signature);
result = true;
@@ -1607,8 +1607,6 @@
jclass staticMethodClass;
JNIEnv* env = self->jniEnv;
- assert(method->insns != NULL);
-
//LOGI("JNI calling %p (%s.%s:%s):\n", method->insns,
// method->clazz->descriptor, method->name, method->shorty);
@@ -1671,6 +1669,9 @@
oldStatus = dvmChangeStatus(self, THREAD_NATIVE);
+ ANDROID_MEMBAR_FULL(); /* guarantee ordering on method->insns */
+ assert(method->insns != NULL);
+
COMPUTE_STACK_SUM(self);
dvmPlatformInvoke(env, staticMethodClass,
method->jniArgInfo, method->insSize, modArgs, method->shorty,
@@ -1730,6 +1731,8 @@
oldStatus = dvmChangeStatus(self, THREAD_NATIVE);
+ ANDROID_MEMBAR_FULL(); /* guarantee ordering on method->insns */
+
COMPUTE_STACK_SUM(self);
dvmPlatformInvoke(self->jniEnv, NULL,
method->jniArgInfo, method->insSize, modArgs, method->shorty,
@@ -1764,6 +1767,8 @@
oldStatus = dvmChangeStatus(self, THREAD_NATIVE);
+ ANDROID_MEMBAR_FULL(); /* guarantee ordering on method->insns */
+
COMPUTE_STACK_SUM(self);
dvmPlatformInvoke(self->jniEnv, staticMethodClass,
method->jniArgInfo, method->insSize, args, method->shorty,
@@ -3236,6 +3241,9 @@
/*
* Register one or more native functions in one class.
+ *
+ * This can be called multiple times on the same method, allowing the
+ * caller to redefine the method implementation at will.
*/
static jint RegisterNatives(JNIEnv* env, jclass jclazz,
const JNINativeMethod* methods, jint nMethods)
@@ -3264,19 +3272,44 @@
}
/*
- * Un-register a native function.
+ * Un-register all native methods associated with the class.
+ *
+ * The JNI docs refer to this as a way to reload/relink native libraries,
+ * and say it "should not be used in normal native code". In particular,
+ * there is no need to do this during shutdown, and you do not need to do
+ * this before redefining a method implementation with RegisterNatives.
+ *
+ * It's chiefly useful for a native "plugin"-style library that wasn't
+ * loaded with System.loadLibrary() (since there's no way to unload those).
+ * For example, the library could upgrade itself by:
+ *
+ * 1. call UnregisterNatives to unbind the old methods
+ * 2. ensure that no code is still executing inside it (somehow)
+ * 3. dlclose() the library
+ * 4. dlopen() the new library
+ * 5. use RegisterNatives to bind the methods from the new library
+ *
+ * The above can work correctly without the UnregisterNatives call, but
+ * creates a window of opportunity in which somebody might try to call a
+ * method that is pointing at unmapped memory, crashing the VM. In theory
+ * the same guards that prevent dlclose() from unmapping executing code could
+ * prevent that anyway, but with this we can be more thorough and also deal
+ * with methods that only exist in the old or new form of the library (maybe
+ * the lib wants to try the call and catch the UnsatisfiedLinkError).
*/
static jint UnregisterNatives(JNIEnv* env, jclass jclazz)
{
JNI_ENTER();
- /*
- * The JNI docs refer to this as a way to reload/relink native libraries,
- * and say it "should not be used in normal native code".
- *
- * We can implement it if we decide we need it.
- */
+
+ ClassObject* clazz = (ClassObject*) dvmDecodeIndirectRef(env, jclazz);
+ if (gDvm.verboseJni) {
+ LOGI("[Unregistering JNI native methods for class %s]\n",
+ clazz->descriptor);
+ }
+ dvmUnregisterJNINativeMethods(clazz);
+
JNI_EXIT();
- return JNI_ERR;
+ return JNI_OK;
}
/*
diff --git a/vm/Native.c b/vm/Native.c
index 479fd52..a641c25 100644
--- a/vm/Native.c
+++ b/vm/Native.c
@@ -57,7 +57,7 @@
*
* This is executed as if it were a native bridge or function. If the
* resolution succeeds, method->insns is replaced, and we don't go through
- * here again.
+ * here again unless the method is unregistered.
*
* Initializes method's class if necessary.
*
@@ -103,8 +103,7 @@
dvmAbort(); // harsh, but this is VM-internal problem
}
DalvikBridgeFunc dfunc = (DalvikBridgeFunc) func;
- dvmSetNativeFunc(method, dfunc, NULL);
- assert(method->insns == NULL);
+ dvmSetNativeFunc((Method*) method, dfunc, NULL);
dfunc(args, pResult, method, self);
return;
}
@@ -594,6 +593,71 @@
/*
+ * Un-register JNI native methods.
+ *
+ * There are two relevant fields in struct Method, "nativeFunc" and
+ * "insns". The former holds a function pointer to a "bridge" function
+ * (or, for internal native, the actual implementation). The latter holds
+ * a pointer to the actual JNI method.
+ *
+ * The obvious approach is to reset both fields to their initial state
+ * (nativeFunc points at dvmResolveNativeMethod, insns holds NULL), but
+ * that creates some unpleasant race conditions. In particular, if another
+ * thread is executing inside the call bridge for the method in question,
+ * and we reset insns to NULL, the VM will crash. (See the comments above
+ * dvmSetNativeFunc() for additional commentary.)
+ *
+ * We can't rely on being able to update two 32-bit fields in one atomic
+ * operation (e.g. no 64-bit atomic ops on ARMv5TE), so we want to change
+ * only one field. It turns out we can simply reset nativeFunc to its
+ * initial state, leaving insns alone, because dvmResolveNativeMethod
+ * ignores "insns" entirely.
+ *
+ * When the method is re-registered, both fields will be updated, but
+ * dvmSetNativeFunc guarantees that "insns" is updated first. This means
+ * we shouldn't be in a situation where we have a "live" call bridge and
+ * a stale implementation pointer.
+ */
+static void unregisterJNINativeMethods(Method* methods, size_t count)
+{
+ while (count != 0) {
+ count--;
+
+ Method* meth = &methods[count];
+ if (!dvmIsNativeMethod(meth))
+ continue;
+ if (dvmIsAbstractMethod(meth)) /* avoid abstract method stubs */
+ continue;
+
+ /*
+ * Strictly speaking this ought to test the function pointer against
+ * the various JNI bridge functions to ensure that we only undo
+ * methods that were registered through JNI. In practice, any
+ * native method with a non-NULL "insns" is a registered JNI method.
+ *
+ * If we inadvertently unregister an internal-native, it'll get
+ * re-resolved on the next call; unregistering an unregistered
+ * JNI method is a no-op. So we don't really need to test for
+ * anything.
+ */
+
+ LOGD("Unregistering JNI method %s.%s:%s\n",
+ meth->clazz->descriptor, meth->name, meth->shorty);
+ dvmSetNativeFunc(meth, dvmResolveNativeMethod, NULL);
+ }
+}
+
+/*
+ * Un-register all JNI native methods from a class.
+ */
+void dvmUnregisterJNINativeMethods(ClassObject* clazz)
+{
+ unregisterJNINativeMethods(clazz->directMethods, clazz->directMethodCount);
+ unregisterJNINativeMethods(clazz->virtualMethods, clazz->virtualMethodCount);
+}
+
+
+/*
* ===========================================================================
* Signature-based method lookup
* ===========================================================================
diff --git a/vm/Native.h b/vm/Native.h
index 2887fbd..5739647 100644
--- a/vm/Native.h
+++ b/vm/Native.h
@@ -78,6 +78,11 @@
void dvmResolveNativeMethod(const u4* args, JValue* pResult,
const Method* method, struct Thread* self);
+/*
+ * Unregister all JNI native methods associated with a class.
+ */
+void dvmUnregisterJNINativeMethods(ClassObject* clazz);
+
//#define GET_ARG_LONG(_args, _elem) (*(s8*)(&(_args)[_elem]))
#define GET_ARG_LONG(_args, _elem) dvmGetArgLong(_args, _elem)
diff --git a/vm/oo/Class.c b/vm/oo/Class.c
index 2816e50..b9dfb7d 100644
--- a/vm/oo/Class.c
+++ b/vm/oo/Class.c
@@ -4527,19 +4527,47 @@
/*
* Replace method->nativeFunc and method->insns with new values. This is
- * performed on resolution of a native method.
+ * commonly performed after successful resolution of a native method.
+ *
+ * There are three basic states:
+ * (1) (initial) nativeFunc = dvmResolveNativeMethod, insns = NULL
+ * (2) (internal native) nativeFunc = <impl>, insns = NULL
+ * (3) (JNI) nativeFunc = JNI call bridge, insns = <impl>
+ *
+ * nativeFunc must never be NULL for a native method.
+ *
+ * The most common transitions are (1)->(2) and (1)->(3). The former is
+ * atomic, since only one field is updated; the latter is not, but since
+ * dvmResolveNativeMethod ignores the "insns" field we just need to make
+ * sure the update happens in the correct order.
+ *
+ * A transition from (2)->(1) would work fine, but (3)->(1) will not,
+ * because both fields change. If we did this while a thread was executing
+ * in the call bridge, we could null out the "insns" field right before
+ * the bridge tried to call through it. So, once "insns" is set, we do
+ * not allow it to be cleared. A NULL value for the "insns" argument is
+ * treated as "do not change existing value".
*/
-void dvmSetNativeFunc(const Method* method, DalvikBridgeFunc func,
+void dvmSetNativeFunc(Method* method, DalvikBridgeFunc func,
const u2* insns)
{
ClassObject* clazz = method->clazz;
+ assert(func != NULL);
+
/* just open up both; easier that way */
dvmLinearReadWrite(clazz->classLoader, clazz->virtualMethods);
dvmLinearReadWrite(clazz->classLoader, clazz->directMethods);
- ((Method*)method)->nativeFunc = func;
- ((Method*)method)->insns = insns;
+ if (insns != NULL) {
+ /* update both, ensuring that "insns" is observed first */
+ method->insns = insns;
+ android_atomic_release_store((int32_t) func,
+ (void*) &method->nativeFunc);
+ } else {
+ /* only update nativeFunc */
+ method->nativeFunc = func;
+ }
dvmLinearReadOnly(clazz->classLoader, clazz->virtualMethods);
dvmLinearReadOnly(clazz->classLoader, clazz->directMethods);
diff --git a/vm/oo/Class.h b/vm/oo/Class.h
index 7457a60..e27ef79 100644
--- a/vm/oo/Class.h
+++ b/vm/oo/Class.h
@@ -139,10 +139,10 @@
bool dvmLoaderInInitiatingList(const ClassObject* clazz, const Object* loader);
/*
- * Update method's "nativeFunc" and "insns" after native method resolution.
+ * Update method's "nativeFunc" and "insns". If "insns" is NULL, the
+ * current method->insns value is not changed.
*/
-void dvmSetNativeFunc(const Method* method, DalvikBridgeFunc func,
- const u2* insns);
+void dvmSetNativeFunc(Method* method, DalvikBridgeFunc func, const u2* insns);
/*
* Set the method's "registerMap" field.
