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
diff --git a/vm/CheckJni.cpp b/vm/CheckJni.cpp
index 77c9fa7..d9ec355 100644
--- a/vm/CheckJni.cpp
+++ b/vm/CheckJni.cpp
@@ -26,14 +26,14 @@
#include "Dalvik.h"
#include "JniInternal.h"
+#include <sys/mman.h>
#include <zlib.h>
/*
* 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 @@
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<GuardedCopy*>(fullBuf);
+ static inline const GuardedCopy* fromData(const void* dataBuf) {
+ return reinterpret_cast<const GuardedCopy*>(actualBuffer(dataBuf));
}
/*
@@ -850,12 +849,8 @@
* 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 @@
* 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 @@
*/
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 @@
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<u1*>(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<const u1*>(dataBuf) - kGuardLen / 2;
+ }
+
+ static u1* actualBuffer(void* dataBuf) {
+ return reinterpret_cast<u1*>(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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 0e3207d..58b9f49 100644
--- a/vm/Globals.h
+++ b/vm/Globals.h
@@ -963,6 +963,15 @@
#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 d3be16f..a33a86a 100644
--- a/vm/Init.cpp
+++ b/vm/Init.cpp
@@ -48,6 +48,7 @@
/* global state */
struct DvmGlobals gDvm;
+struct DvmJniGlobals gDvmJni;
/* JIT-specific global state */
#if defined(WITH_JIT)
@@ -111,7 +112,7 @@
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:<filename>\n");
diff --git a/vm/Jni.cpp b/vm/Jni.cpp
index 13274c4..5a05990 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 <stdlib.h>
#include <stdarg.h>
@@ -509,14 +510,12 @@
/* 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 @@
}
/*
- * 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 @@
}
}
- return dvmIsCheckJNIEnabled() ? checkFunc[kind] : stdFunc[kind];
+ return gDvmJni.useCheckJni ? checkFunc[kind] : stdFunc[kind];
}
/*
@@ -3289,7 +3280,6 @@
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 @@
newEnv->envThreadId = 0x77777775;
newEnv->self = (Thread*) 0x77777779;
}
- if (vm->useChecked) {
+ if (gDvmJni.useCheckJni) {
dvmUseCheckedJniEnv(newEnv);
}
@@ -3367,13 +3357,10 @@
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_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 @@
/*
* 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<const char*[]> argv(new const char*[args->nOptions]);
+ memset(argv.get(), 0, sizeof(char*) * (args->nOptions));
/*
* Convert JNI args to argv.
@@ -3456,12 +3426,13 @@
* "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 @@
} 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 @@
argv[curOpt++] = optStr;
}
}
- argc = curOpt;
+ int argc = curOpt;
- if (checkJni) {
- dvmUseCheckedJniVm(pVM);
- pVM->useChecked = true;
+ if (sawJniOpts && !gDvmJni.useCheckJni) {
+ dvmFprintf(stderr, "ERROR: -Xjniopts only makes sense with -Xcheck:jni\n");
+ return JNI_ERR;
}
- pVM->warnError = warnError;
- pVM->forceDataCopy = forceDataCopy;
+ 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);
+ }
/* set this up before initializing VM, so it can create some JNIEnvs */
gDvm.vmList = (JavaVM*) pVM;
@@ -3508,14 +3491,18 @@
* 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 @@
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 d81373f..f7d831a 100644
--- a/vm/JniInternal.h
+++ b/vm/JniInternal.h
@@ -50,9 +50,6 @@
/* 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 @@
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 62cd0c2..79d46ce 100644
--- a/vm/native/dalvik_system_Zygote.cpp
+++ b/vm/native/dalvik_system_Zygote.cpp
@@ -262,9 +262,7 @@
* 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,
