From a5f3ed80b3b058b006ee2b09915d1400cebd0442 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 26 Apr 2011 18:32:17 -0700 Subject: [PATCH] Add -Xjniopts:forcecopy-unmap to catch more errors than forcecopy. In particular, this spots the BreakIterator bug that forcecopy didn't. It's about 2x slower than regular forcecopy mode, so I've added a new option rather than just replace the fast-but-less-effective forcecopy. Bug: 3412449 Change-Id: I1f226ceeab2508dff607ba25b0afee51cf9c3f83 --- vm/CheckJni.cpp | 96 +++++++++++++++++++++--------- vm/Globals.h | 9 +++ vm/Init.cpp | 3 +- vm/Jni.cpp | 116 +++++++++++++++---------------------- vm/JniInternal.h | 10 ---- vm/native/dalvik_system_Zygote.cpp | 4 +- 6 files changed, 129 insertions(+), 109 deletions(-) diff --git a/vm/CheckJni.cpp b/vm/CheckJni.cpp index 77c9fa722..d9ec3553b 100644 --- a/vm/CheckJni.cpp +++ b/vm/CheckJni.cpp @@ -26,14 +26,14 @@ #include "Dalvik.h" #include "JniInternal.h" +#include #include /* * Abort if we are configured to bail out on JNI warnings. */ static void abortMaybe() { - JavaVMExt* vm = (JavaVMExt*) gDvm.vmList; - if (vm->warnError) { + if (!gDvmJni.warnOnly) { dvmDumpThread(dvmThreadSelf(), false); dvmAbort(); } @@ -838,9 +838,8 @@ struct GuardedCopy { const void* originalPtr; /* find the GuardedCopy given the pointer into the "live" data */ - static inline GuardedCopy* fromData(const void* dataBuf) { - u1* fullBuf = ((u1*) dataBuf) - kGuardLen / 2; - return reinterpret_cast(fullBuf); + static inline const GuardedCopy* fromData(const void* dataBuf) { + return reinterpret_cast(actualBuffer(dataBuf)); } /* @@ -850,12 +849,8 @@ struct GuardedCopy { * We use a 16-bit pattern to make a rogue memset less likely to elude us. */ static void* create(const void* buf, size_t len, bool modOkay) { - size_t newLen = (len + kGuardLen + 1) & ~0x01; - u1* newBuf = (u1*)malloc(newLen); - if (newBuf == NULL) { - LOGE("GuardedCopy::create failed on alloc of %d bytes", newLen); - dvmAbort(); - } + size_t newLen = actualLength(len); + u1* newBuf = debugAlloc(newLen); /* fill it in with a pattern */ u2* pat = (u2*) newBuf; @@ -887,12 +882,10 @@ struct GuardedCopy { * Free up the guard buffer, scrub it, and return the original pointer. */ static void* destroy(void* dataBuf) { - u1* fullBuf = ((u1*) dataBuf) - kGuardLen / 2; const GuardedCopy* pExtra = GuardedCopy::fromData(dataBuf); void* originalPtr = (void*) pExtra->originalPtr; size_t len = pExtra->originalLen; - memset(dataBuf, 0xdd, len); - free(fullBuf); + debugFree(dataBuf, len); return originalPtr; } @@ -904,7 +897,7 @@ struct GuardedCopy { */ static bool check(const void* dataBuf, bool modOkay) { static const u4 kMagicCmp = kGuardMagic; - const u1* fullBuf = ((const u1*) dataBuf) - kGuardLen / 2; + const u1* fullBuf = actualBuffer(dataBuf); const GuardedCopy* pExtra = GuardedCopy::fromData(dataBuf); /* @@ -970,6 +963,57 @@ struct GuardedCopy { return true; } + +private: + static u1* debugAlloc(size_t len) { + if (gDvmJni.forceDataUnmap) { + void* result = mmap(NULL, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0); + if (result == MAP_FAILED) { + LOGE("GuardedCopy::create mmap(%d) failed: %s", len, strerror(errno)); + dvmAbort(); + } + return reinterpret_cast(result); + } else { + u1* result = new u1[len]; + if (result == NULL) { + LOGE("GuardedCopy::create malloc(%d) failed: %s", len, strerror(errno)); + dvmAbort(); + } + return result; + } + } + + static void debugFree(void* dataBuf, size_t len) { + u1* fullBuf = actualBuffer(dataBuf); + size_t totalByteCount = actualLength(len); + if (gDvmJni.forceDataUnmap) { + // TODO: we could mprotect instead, and keep the allocation around for a while. + // This would be even more expensive, but it might catch more errors. + // if (mprotect(fullBuf, totalByteCount, PROT_NONE) != 0) { + // LOGW("mprotect(PROT_NONE) failed: %s\n", strerror(errno)); + // } + if (munmap(fullBuf, totalByteCount) != 0) { + LOGW("munmap failed: %s\n", strerror(errno)); + dvmAbort(); + } + } else { + memset(dataBuf, 0xdd, len); // TODO: wipe the underlying buffer, not just the user one? + delete[] fullBuf; + } + } + + static const u1* actualBuffer(const void* dataBuf) { + return reinterpret_cast(dataBuf) - kGuardLen / 2; + } + + static u1* actualBuffer(void* dataBuf) { + return reinterpret_cast(dataBuf) - kGuardLen / 2; + } + + // Underlying length of a user allocation of 'length' bytes. + static size_t actualLength(size_t length) { + return (length + kGuardLen + 1) & ~0x01; + } }; /* @@ -1532,7 +1576,7 @@ static const jchar* Check_GetStringChars(JNIEnv* env, jstring string, jboolean* ScopedCheck sc(env, kFlag_CritOkay, __FUNCTION__); sc.checkString(string); const jchar* result = baseEnv(env)->GetStringChars(env, string, isCopy); - if (((JNIEnvExt*)env)->forceDataCopy && result != NULL) { + if (gDvmJni.forceDataCopy && result != NULL) { ScopedJniThreadState ts(env); StringObject* strObj = (StringObject*) dvmDecodeIndirectRef(env, string); int byteCount = dvmStringLen(strObj) * 2; @@ -1548,7 +1592,7 @@ static void Check_ReleaseStringChars(JNIEnv* env, jstring string, const jchar* c ScopedCheck sc(env, kFlag_Default | kFlag_ExcepOkay, __FUNCTION__); sc.checkString(string); sc.checkNonNull(chars); - if (((JNIEnvExt*)env)->forceDataCopy) { + if (gDvmJni.forceDataCopy) { if (!GuardedCopy::check(chars, false)) { LOGE("JNI: failed guarded copy check in ReleaseStringChars"); abortMaybe(); @@ -1575,7 +1619,7 @@ static const char* Check_GetStringUTFChars(JNIEnv* env, jstring string, jboolean ScopedCheck sc(env, kFlag_CritOkay, __FUNCTION__); sc.checkString(string); const char* result = baseEnv(env)->GetStringUTFChars(env, string, isCopy); - if (((JNIEnvExt*)env)->forceDataCopy && result != NULL) { + if (gDvmJni.forceDataCopy && result != NULL) { result = (const char*) GuardedCopy::create(result, strlen(result) + 1, false); if (isCopy != NULL) { *isCopy = JNI_TRUE; @@ -1588,7 +1632,7 @@ static void Check_ReleaseStringUTFChars(JNIEnv* env, jstring string, const char* ScopedCheck sc(env, kFlag_ExcepOkay, __FUNCTION__); sc.checkString(string); sc.checkNonNull(utf); - if (((JNIEnvExt*)env)->forceDataCopy) { + if (gDvmJni.forceDataCopy) { if (!GuardedCopy::check(utf, false)) { LOGE("JNI: failed guarded copy check in ReleaseStringUTFChars"); abortMaybe(); @@ -1659,12 +1703,12 @@ NEW_PRIMITIVE_ARRAY(jdoubleArray, Double); ScopedCheck sc(env, kFlag_Default, __FUNCTION__); \ sc.checkArray(array); \ u4 noCopy = 0; \ - if (((JNIEnvExt*)env)->forceDataCopy && isCopy != NULL) { \ + if (gDvmJni.forceDataCopy && isCopy != NULL) { \ /* capture this before the base call tramples on it */ \ noCopy = *(u4*) isCopy; \ } \ _ctype* result = baseEnv(env)->Get##_jname##ArrayElements(env, array, isCopy); \ - if (((JNIEnvExt*)env)->forceDataCopy && result != NULL) { \ + if (gDvmJni.forceDataCopy && result != NULL) { \ if (noCopy == kNoCopyMagic) { \ LOGV("FC: not copying %p %x", array, noCopy); \ } else { \ @@ -1682,7 +1726,7 @@ NEW_PRIMITIVE_ARRAY(jdoubleArray, Double); sc.checkArray(array); \ sc.checkNonNull(elems); \ sc.checkReleaseMode(mode); \ - if (((JNIEnvExt*)env)->forceDataCopy) { \ + if (gDvmJni.forceDataCopy) { \ if ((uintptr_t)elems == kNoCopyMagic) { \ LOGV("FC: not freeing %p", array); \ elems = NULL; /* base JNI call doesn't currently need */ \ @@ -1773,7 +1817,7 @@ static void* Check_GetPrimitiveArrayCritical(JNIEnv* env, jarray array, jboolean ScopedCheck sc(env, kFlag_CritGet, __FUNCTION__); sc.checkArray(array); void* result = baseEnv(env)->GetPrimitiveArrayCritical(env, array, isCopy); - if (((JNIEnvExt*)env)->forceDataCopy && result != NULL) { + if (gDvmJni.forceDataCopy && result != NULL) { result = createGuardedPACopy(env, array, isCopy); } return result; @@ -1785,7 +1829,7 @@ static void Check_ReleasePrimitiveArrayCritical(JNIEnv* env, jarray array, void* sc.checkArray(array); sc.checkNonNull(carray); sc.checkReleaseMode(mode); - if (((JNIEnvExt*)env)->forceDataCopy) { + if (gDvmJni.forceDataCopy) { carray = releaseGuardedPACopy(env, array, carray, mode); } baseEnv(env)->ReleasePrimitiveArrayCritical(env, array, carray, mode); @@ -1795,7 +1839,7 @@ static const jchar* Check_GetStringCritical(JNIEnv* env, jstring string, jboolea ScopedCheck sc(env, kFlag_CritGet, __FUNCTION__); sc.checkString(string); const jchar* result = baseEnv(env)->GetStringCritical(env, string, isCopy); - if (((JNIEnvExt*)env)->forceDataCopy && result != NULL) { + if (gDvmJni.forceDataCopy && result != NULL) { ScopedJniThreadState ts(env); StringObject* strObj = (StringObject*) dvmDecodeIndirectRef(env, string); int byteCount = dvmStringLen(strObj) * 2; @@ -1811,7 +1855,7 @@ static void Check_ReleaseStringCritical(JNIEnv* env, jstring string, const jchar ScopedCheck sc(env, kFlag_CritRelease | kFlag_ExcepOkay, __FUNCTION__); sc.checkString(string); sc.checkNonNull(carray); - if (((JNIEnvExt*)env)->forceDataCopy) { + if (gDvmJni.forceDataCopy) { if (!GuardedCopy::check(carray, false)) { LOGE("JNI: failed guarded copy check in ReleaseStringCritical"); abortMaybe(); diff --git a/vm/Globals.h b/vm/Globals.h index 0e3207d32..58b9f49d5 100644 --- a/vm/Globals.h +++ b/vm/Globals.h @@ -963,6 +963,15 @@ extern int gDvmICHitCount; #endif +struct DvmJniGlobals { + bool useCheckJni; + bool warnOnly; + bool forceDataCopy; + bool forceDataUnmap; +}; + +extern struct DvmJniGlobals gDvmJni; + #ifdef __cplusplus } #endif diff --git a/vm/Init.cpp b/vm/Init.cpp index d3be16ff4..a33a86a6b 100644 --- a/vm/Init.cpp +++ b/vm/Init.cpp @@ -48,6 +48,7 @@ static bool initZygote(); /* global state */ struct DvmGlobals gDvm; +struct DvmJniGlobals gDvmJni; /* JIT-specific global state */ #if defined(WITH_JIT) @@ -111,7 +112,7 @@ static void usage(const char* progName) dvmFprintf(stderr, " -Xnoquithandler\n"); dvmFprintf(stderr, " -Xjnigreflimit:N (must be multiple of 100, >= 200)\n"); - dvmFprintf(stderr, " -Xjniopts:{warnonly,forcecopy}\n"); + dvmFprintf(stderr, " -Xjniopts:{warnonly,forcecopy,forcecopy-unmap}\n"); dvmFprintf(stderr, " -Xjnitrace:substring (eg NativeClass or nativeMethod)\n"); dvmFprintf(stderr, " -Xdeadlockpredict:{off,warn,err,abort}\n"); dvmFprintf(stderr, " -Xstacktracefile:\n"); diff --git a/vm/Jni.cpp b/vm/Jni.cpp index 13274c467..5a0599051 100644 --- a/vm/Jni.cpp +++ b/vm/Jni.cpp @@ -20,6 +20,7 @@ #include "Dalvik.h" #include "JniInternal.h" #include "ScopedPthreadMutexLock.h" +#include "UniquePtr.h" #include #include @@ -509,14 +510,12 @@ static jobject addGlobalReference(Object* obj) { /* watch for "excessive" use; not generally appropriate */ if (count >= gDvm.jniGrefLimit) { - JavaVMExt* vm = (JavaVMExt*) gDvm.vmList; - if (vm->warnError) { - dvmDumpIndirectRefTable(&gDvm.jniGlobalRefTable, - "JNI global"); + if (gDvmJni.warnOnly) { + LOGW("Excessive JNI global references (%d)", count); + } else { + dvmDumpIndirectRefTable(&gDvm.jniGlobalRefTable, "JNI global"); LOGE("Excessive JNI global references (%d)", count); dvmAbort(); - } else { - LOGW("Excessive JNI global references (%d)", count); } } } @@ -743,14 +742,6 @@ static bool dvmRegisterJNIMethod(ClassObject* clazz, const char* methodName, } /* - * Returns "true" if CheckJNI is enabled in the VM. - */ -static bool dvmIsCheckJNIEnabled() { - JavaVMExt* vm = (JavaVMExt*) gDvm.vmList; - return vm->useChecked; -} - -/* * Returns the appropriate JNI bridge for 'method', also taking into account * the -Xcheck:jni setting. */ @@ -806,7 +797,7 @@ static DalvikBridgeFunc dvmSelectJNIBridge(const Method* method) { } } - return dvmIsCheckJNIEnabled() ? checkFunc[kind] : stdFunc[kind]; + return gDvmJni.useCheckJni ? checkFunc[kind] : stdFunc[kind]; } /* @@ -3289,7 +3280,6 @@ JNIEnv* dvmCreateJNIEnv(Thread* self) { JNIEnvExt* newEnv = (JNIEnvExt*) calloc(1, sizeof(JNIEnvExt)); newEnv->funcTable = &gNativeInterface; newEnv->vm = vm; - newEnv->forceDataCopy = vm->forceDataCopy; if (self != NULL) { dvmSetJniEnvThreadId((JNIEnv*) newEnv, self); assert(newEnv->envThreadId != 0); @@ -3298,7 +3288,7 @@ JNIEnv* dvmCreateJNIEnv(Thread* self) { newEnv->envThreadId = 0x77777775; newEnv->self = (Thread*) 0x77777779; } - if (vm->useChecked) { + if (gDvmJni.useCheckJni) { dvmUseCheckedJniEnv(newEnv); } @@ -3367,13 +3357,10 @@ void dvmLateEnableCheckedJni() { JavaVMExt* extVm = extEnv->vm; assert(extVm != NULL); - if (!extVm->useChecked) { + if (!gDvmJni.useCheckJni) { LOGD("Late-enabling CheckJNI"); dvmUseCheckedJniVm(extVm); - extVm->useChecked = true; dvmUseCheckedJniEnv(extEnv); - - /* currently no way to pick up jniopts features */ } else { LOGD("Not late-enabling CheckJNI (already on)"); } @@ -3411,16 +3398,6 @@ jint JNI_GetCreatedJavaVMs(JavaVM** vmBuf, jsize bufLen, jsize* nVMs) { */ jint JNI_CreateJavaVM(JavaVM** p_vm, JNIEnv** p_env, void* vm_args) { const JavaVMInitArgs* args = (JavaVMInitArgs*) vm_args; - JNIEnvExt* pEnv = NULL; - JavaVMExt* pVM = NULL; - const char** argv; - int argc = 0; - int i, curOpt; - int result = JNI_ERR; - bool checkJni = false; - bool warnError = true; - bool forceDataCopy = false; - if (args->version < JNI_VERSION_1_2) { return JNI_EVERSION; } @@ -3433,21 +3410,14 @@ jint JNI_CreateJavaVM(JavaVM** p_vm, JNIEnv** p_env, void* vm_args) { /* * Set up structures for JNIEnv and VM. */ - //pEnv = (JNIEnvExt*) malloc(sizeof(JNIEnvExt)); - pVM = (JavaVMExt*) malloc(sizeof(JavaVMExt)); - - //memset(pEnv, 0, sizeof(JNIEnvExt)); - //pEnv->funcTable = &gNativeInterface; - //pEnv->vm = pVM; + JavaVMExt* pVM = (JavaVMExt*) malloc(sizeof(JavaVMExt)); memset(pVM, 0, sizeof(JavaVMExt)); pVM->funcTable = &gInvokeInterface; - pVM->envList = pEnv; + pVM->envList = NULL; dvmInitMutex(&pVM->envListLock); - argv = (const char**) malloc(sizeof(char*) * (args->nOptions)); - memset(argv, 0, sizeof(char*) * (args->nOptions)); - - curOpt = 0; + UniquePtr argv(new const char*[args->nOptions]); + memset(argv.get(), 0, sizeof(char*) * (args->nOptions)); /* * Convert JNI args to argv. @@ -3456,12 +3426,13 @@ jint JNI_CreateJavaVM(JavaVM** p_vm, JNIEnv** p_env, void* vm_args) { * "extraInfo" field to pass function pointer "hooks" in. We also * look for the -Xcheck:jni stuff here. */ - for (i = 0; i < args->nOptions; i++) { + int curOpt = 0; + bool sawJniOpts = false; + for (int i = 0; i < args->nOptions; i++) { const char* optStr = args->options[i].optionString; - if (optStr == NULL) { - fprintf(stderr, "ERROR: arg %d string was null", i); - goto bail; + dvmFprintf(stderr, "ERROR: CreateJavaVM failed: argument %d was NULL\n", i); + return JNI_ERR; } else if (strcmp(optStr, "vfprintf") == 0) { gDvm.vfprintfHook = (int (*)(FILE *, const char*, va_list))args->options[i].extraInfo; @@ -3472,15 +3443,18 @@ jint JNI_CreateJavaVM(JavaVM** p_vm, JNIEnv** p_env, void* vm_args) { } else if (strcmp(optStr, "sensitiveThread") == 0) { gDvm.isSensitiveThreadHook = (bool (*)(void))args->options[i].extraInfo; } else if (strcmp(optStr, "-Xcheck:jni") == 0) { - checkJni = true; + gDvmJni.useCheckJni = true; } else if (strncmp(optStr, "-Xjniopts:", 10) == 0) { + sawJniOpts = true; const char* jniOpts = optStr + 9; while (jniOpts != NULL) { jniOpts++; /* skip past ':' or ',' */ if (strncmp(jniOpts, "warnonly", 8) == 0) { - warnError = false; + gDvmJni.warnOnly = true; + } else if (strncmp(jniOpts, "forcecopy-unmap", 15) == 0) { + gDvmJni.forceDataUnmap = true; } else if (strncmp(jniOpts, "forcecopy", 9) == 0) { - forceDataCopy = true; + gDvmJni.forceDataCopy = true; } else { LOGW("unknown jni opt starting at '%s'", jniOpts); } @@ -3491,14 +3465,23 @@ jint JNI_CreateJavaVM(JavaVM** p_vm, JNIEnv** p_env, void* vm_args) { argv[curOpt++] = optStr; } } - argc = curOpt; + int argc = curOpt; - if (checkJni) { + if (sawJniOpts && !gDvmJni.useCheckJni) { + dvmFprintf(stderr, "ERROR: -Xjniopts only makes sense with -Xcheck:jni\n"); + return JNI_ERR; + } + if (gDvmJni.forceDataUnmap && gDvmJni.forceDataCopy) { + dvmFprintf(stderr, "ERROR: choose one of forcecopy or forcecopy-unmap\n"); + return JNI_ERR; + } + if (gDvmJni.forceDataUnmap) { + gDvmJni.forceDataCopy = true; // It simplifies CheckJNI if we only have to check one thing. + } + + if (gDvmJni.useCheckJni) { dvmUseCheckedJniVm(pVM); - pVM->useChecked = true; } - pVM->warnError = warnError; - pVM->forceDataCopy = forceDataCopy; /* set this up before initializing VM, so it can create some JNIEnvs */ gDvm.vmList = (JavaVM*) pVM; @@ -3508,14 +3491,18 @@ jint JNI_CreateJavaVM(JavaVM** p_vm, JNIEnv** p_env, void* vm_args) { * here because some of the class initialization we do when starting * up the VM will call into native code. */ - pEnv = (JNIEnvExt*) dvmCreateJNIEnv(NULL); + JNIEnvExt* pEnv = (JNIEnvExt*) dvmCreateJNIEnv(NULL); - /* initialize VM */ + /* Initialize VM. */ gDvm.initializing = true; - if (dvmStartup(argc, argv, args->ignoreUnrecognized, (JNIEnv*)pEnv) != 0) { + int rc = dvmStartup(argc, argv.get(), args->ignoreUnrecognized, (JNIEnv*)pEnv); + gDvm.initializing = false; + + if (rc != 0) { free(pEnv); free(pVM); - goto bail; + LOGW("CreateJavaVM failed"); + return JNI_ERR; } /* @@ -3524,15 +3511,6 @@ jint JNI_CreateJavaVM(JavaVM** p_vm, JNIEnv** p_env, void* vm_args) { dvmChangeStatus(NULL, THREAD_NATIVE); *p_env = (JNIEnv*) pEnv; *p_vm = (JavaVM*) pVM; - result = JNI_OK; - -bail: - gDvm.initializing = false; - if (result == JNI_OK) { - LOGV("JNI_CreateJavaVM succeeded"); - } else { - LOGW("JNI_CreateJavaVM failed"); - } - free(argv); - return result; + LOGV("CreateJavaVM succeeded"); + return JNI_OK; } diff --git a/vm/JniInternal.h b/vm/JniInternal.h index d81373fb0..f7d831a2e 100644 --- a/vm/JniInternal.h +++ b/vm/JniInternal.h @@ -50,9 +50,6 @@ typedef struct JNIEnvExt { /* if nonzero, we are in a "critical" JNI call */ int critical; - /* keep a copy of this here for speed */ - bool forceDataCopy; - struct JNIEnvExt* prev; struct JNIEnvExt* next; } JNIEnvExt; @@ -62,13 +59,6 @@ typedef struct JavaVMExt { const struct JNIInvokeInterface* baseFuncTable; - /* if multiple VMs are desired, add doubly-linked list stuff here */ - - /* per-VM feature flags */ - bool useChecked; - bool warnError; - bool forceDataCopy; - /* head of list of JNIEnvs associated with this VM */ JNIEnvExt* envList; pthread_mutex_t envListLock; diff --git a/vm/native/dalvik_system_Zygote.cpp b/vm/native/dalvik_system_Zygote.cpp index 62cd0c2dd..79d46ce47 100644 --- a/vm/native/dalvik_system_Zygote.cpp +++ b/vm/native/dalvik_system_Zygote.cpp @@ -262,9 +262,7 @@ static void Dalvik_dalvik_system_Zygote_fork(const u4* args, JValue* pResult) * easy to handle, because the JDWP thread isn't started until we call * dvmInitAfterZygote(). * checkjni - * If set, make sure "check JNI" is eabled. This is a little weird, - * because we already have the JNIEnv for the main thread set up. However, - * since we only have one thread at this point, it's easy to patch up. + * If set, make sure "check JNI" is enabled. * assert * If set, make sure assertions are enabled. This gets fairly weird, * because it affects the result of a method called by class initializers, -- 2.11.0