Improve JniAbort (and JNI tests).
This has been on my to-do list for a while, but it actually bit people
in the ass yesterday. This change enables us to write a lot more (and
better) tests, but for now I've just improved the tests that already
existed.
Change-Id: I04a18656de60b47e5a6b5777204c144209d1448e
diff --git a/src/check_jni.cc b/src/check_jni.cc
index fb7fa02..4da4b37 100644
--- a/src/check_jni.cc
+++ b/src/check_jni.cc
@@ -33,19 +33,19 @@
namespace art {
-void JniAbort(const char* jni_function_name) {
+static void JniAbort(const char* jni_function_name, const char* msg) {
Thread* self = Thread::Current();
Method* current_method = self->GetCurrentMethod();
std::ostringstream os;
- os << "Aborting because JNI app bug detected (see above for details)";
+ os << "JNI DETECTED ERROR IN APPLICATION: " << msg;
if (jni_function_name != NULL) {
- os << "\n in call to " << jni_function_name;
+ os << "\n in call to " << jni_function_name;
}
// TODO: is this useful given that we're about to dump the calling thread's stack?
if (current_method != NULL) {
- os << "\n from " << PrettyMethod(current_method);
+ os << "\n from " << PrettyMethod(current_method);
}
os << "\n";
self->Dump(os);
@@ -59,6 +59,19 @@
}
}
+static void JniAbortV(const char* jni_function_name, const char* fmt, va_list ap) {
+ std::string msg;
+ StringAppendV(&msg, fmt, ap);
+ JniAbort(jni_function_name, msg.c_str());
+}
+
+void JniAbortF(const char* jni_function_name, const char* fmt, ...) {
+ va_list args;
+ va_start(args, fmt);
+ JniAbortV(jni_function_name, fmt, args);
+ va_end(args);
+}
+
/*
* ===========================================================================
* JNI function helpers
@@ -75,32 +88,28 @@
return reinterpret_cast<T>(ts.Self()->DecodeJObject(obj));
}
-/*
- * Hack to allow forcecopy to work with jniGetNonMovableArrayElements.
- * The code deliberately uses an invalid sequence of operations, so we
- * need to pass it through unmodified. Review that code before making
- * any changes here.
- */
+// Hack to allow forcecopy to work with jniGetNonMovableArrayElements.
+// The code deliberately uses an invalid sequence of operations, so we
+// need to pass it through unmodified. Review that code before making
+// any changes here.
#define kNoCopyMagic 0xd5aab57f
-/*
- * Flags passed into ScopedCheck.
- */
+// Flags passed into ScopedCheck.
#define kFlag_Default 0x0000
-#define kFlag_CritBad 0x0000 /* calling while in critical is bad */
-#define kFlag_CritOkay 0x0001 /* ...okay */
-#define kFlag_CritGet 0x0002 /* this is a critical "get" */
-#define kFlag_CritRelease 0x0003 /* this is a critical "release" */
-#define kFlag_CritMask 0x0003 /* bit mask to get "crit" value */
+#define kFlag_CritBad 0x0000 // Calling while in critical is not allowed.
+#define kFlag_CritOkay 0x0001 // Calling while in critical is allowed.
+#define kFlag_CritGet 0x0002 // This is a critical "get".
+#define kFlag_CritRelease 0x0003 // This is a critical "release".
+#define kFlag_CritMask 0x0003 // Bit mask to get "crit" value.
-#define kFlag_ExcepBad 0x0000 /* raised exceptions are bad */
-#define kFlag_ExcepOkay 0x0004 /* ...okay */
+#define kFlag_ExcepBad 0x0000 // Raised exceptions are not allowed.
+#define kFlag_ExcepOkay 0x0004 // Raised exceptions are allowed.
-#define kFlag_Release 0x0010 /* are we in a non-critical release function? */
-#define kFlag_NullableUtf 0x0020 /* are our UTF parameters nullable? */
+#define kFlag_Release 0x0010 // Are we in a non-critical release function?
+#define kFlag_NullableUtf 0x0020 // Are our UTF parameters nullable?
-#define kFlag_Invocation 0x8000 /* Part of the invocation interface (JavaVM*) */
+#define kFlag_Invocation 0x8000 // Part of the invocation interface (JavaVM*).
#define kFlag_ForceTrace 0x80000000 // Add this to a JNI function's flags if you want to trace every call.
@@ -116,7 +125,7 @@
NULL
};
-bool ShouldTrace(JavaVMExt* vm, const Method* method) {
+static bool ShouldTrace(JavaVMExt* vm, const Method* method) {
// If both "-Xcheck:jni" and "-Xjnitrace:" are enabled, we print trace messages
// when a native method that matches the -Xjnitrace argument calls a JNI function
// such as NewByteArray.
@@ -162,9 +171,10 @@
// circumstances, but this is incorrect.
void CheckClassName(const char* class_name) {
if (!IsValidJniClassName(class_name)) {
- LOG(ERROR) << "JNI ERROR: illegal class name '" << class_name << "' (" << function_name_ << ")\n"
- << " (should be of the form 'java/lang/String', [Ljava/lang/String;' or '[[B')\n";
- JniAbort();
+ JniAbortF(function_name_,
+ "illegal class name '%s'\n"
+ " (should be of the form 'package/Class', [Lpackage/Class;' or '[[B')",
+ class_name);
}
}
@@ -184,36 +194,33 @@
if (!field_type->IsPrimitive()) {
if (java_object != NULL) {
Object* obj = Decode<Object*>(ts, java_object);
- /*
- * If java_object is a weak global ref whose referent has been cleared,
- * obj will be NULL. Otherwise, obj should always be non-NULL
- * and valid.
- */
+ // If java_object is a weak global ref whose referent has been cleared,
+ // obj will be NULL. Otherwise, obj should always be non-NULL
+ // and valid.
if (!Runtime::Current()->GetHeap()->IsHeapAddress(obj)) {
- LOG(ERROR) << "JNI ERROR: field operation on invalid " << GetIndirectRefKind(java_object) << ": " << java_object;
- JniAbort();
+ JniAbortF(function_name_, "field operation on invalid %s: %p",
+ ToStr<IndirectRefKind>(GetIndirectRefKind(java_object)).c_str(), java_object);
return;
} else {
if (!obj->InstanceOf(field_type)) {
- LOG(ERROR) << "JNI ERROR: attempt to set field " << PrettyField(f) << " with value of wrong type: " << PrettyTypeOf(obj);
- JniAbort();
+ JniAbortF(function_name_, "attempt to set field %s with value of wrong type: %s",
+ PrettyField(f).c_str(), PrettyTypeOf(obj).c_str());
return;
}
}
}
} else if (field_type != Runtime::Current()->GetClassLinker()->FindPrimitiveClass(prim)) {
- LOG(ERROR) << "JNI ERROR: attempt to set field " << PrettyField(f) << " with value of wrong type: " << prim;
- JniAbort();
+ JniAbortF(function_name_, "attempt to set field %s with value of wrong type: %c",
+ PrettyField(f).c_str(), prim);
return;
}
- if (isStatic && !f->IsStatic()) {
+ if (isStatic != f->IsStatic()) {
if (isStatic) {
- LOG(ERROR) << "JNI ERROR: accessing non-static field " << PrettyField(f) << " as static";
+ JniAbortF(function_name_, "accessing non-static field %s as static", PrettyField(f).c_str());
} else {
- LOG(ERROR) << "JNI ERROR: accessing static field " << PrettyField(f) << " as non-static";
+ JniAbortF(function_name_, "accessing static field %s as non-static", PrettyField(f).c_str());
}
- JniAbort();
return;
}
}
@@ -228,8 +235,8 @@
Object* o = Decode<Object*>(ts, java_object);
if (o == NULL || !Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
- LOG(ERROR) << "JNI ERROR: field operation on invalid " << GetIndirectRefKind(java_object) << ": " << java_object;
- JniAbort();
+ JniAbortF(function_name_, "field operation on invalid %s: %p",
+ ToStr<IndirectRefKind>(GetIndirectRefKind(java_object)).c_str(), java_object);
return;
}
@@ -240,9 +247,8 @@
Class* c = o->GetClass();
FieldHelper fh(f);
if (c->FindInstanceField(fh.GetName(), fh.GetTypeDescriptor()) == NULL) {
- LOG(ERROR) << "JNI ERROR: jfieldID " << PrettyField(f)
- << " not valid for an object of class " << PrettyTypeOf(o);
- JniAbort();
+ JniAbortF(function_name_, "jfieldID %s not valid for an object of class %s",
+ PrettyField(f).c_str(), PrettyTypeOf(o).c_str());
}
}
@@ -251,8 +257,7 @@
*/
void CheckNonNull(const void* ptr) {
if (ptr == NULL) {
- LOG(ERROR) << "JNI ERROR: invalid null pointer";
- JniAbort();
+ JniAbortF(function_name_, "non-nullable argument was NULL");
}
}
@@ -267,18 +272,17 @@
return;
}
if (*expectedType != MethodHelper(m).GetShorty()[0]) {
- LOG(ERROR) << "JNI ERROR: the return type of " << function_name_ << " does not match "
- << PrettyMethod(m);
- JniAbort();
- } else if (isStatic && !m->IsStatic()) {
+ JniAbortF(function_name_, "the return type of %s does not match %s",
+ function_name_, PrettyMethod(m).c_str());
+ }
+ if (isStatic != m->IsStatic()) {
if (isStatic) {
- LOG(ERROR) << "JNI ERROR: calling non-static method "
- << PrettyMethod(m) << " with " << function_name_;
+ JniAbortF(function_name_, "calling non-static method %s with %s",
+ PrettyMethod(m).c_str(), function_name_);
} else {
- LOG(ERROR) << "JNI ERROR: calling static method "
- << PrettyMethod(m) << " with non-static " << function_name_;
+ JniAbortF(function_name_, "calling static method %s with %s",
+ PrettyMethod(m).c_str(), function_name_);
}
- JniAbort();
}
}
@@ -295,8 +299,8 @@
return;
}
if (f->GetDeclaringClass() != c) {
- LOG(ERROR) << "JNI ERROR: static jfieldID " << fid << " not valid for class " << PrettyClass(c);
- JniAbort();
+ JniAbortF(function_name_, "static jfieldID %p not valid for class %s",
+ fid, PrettyClass(c).c_str());
}
}
@@ -317,8 +321,8 @@
}
Class* c = Decode<Class*>(ts, java_class);
if (!c->IsAssignableFrom(m->GetDeclaringClass())) {
- LOG(ERROR) << "JNI ERROR: can't call static " << PrettyMethod(m) << " on class " << PrettyClass(c);
- JniAbort();
+ JniAbortF(function_name_, "can't call static %s on class %s",
+ PrettyMethod(m).c_str(), PrettyClass(c).c_str());
}
}
@@ -337,8 +341,8 @@
}
Object* o = Decode<Object*>(ts, java_object);
if (!o->InstanceOf(m->GetDeclaringClass())) {
- LOG(ERROR) << "JNI ERROR: can't call " << PrettyMethod(m) << " on instance of " << PrettyTypeOf(o);
- JniAbort();
+ JniAbortF(function_name_, "can't call %s on instance of %s",
+ PrettyMethod(m).c_str(), PrettyTypeOf(o).c_str());
}
}
@@ -500,8 +504,7 @@
} else if (ch == '.') {
msg += "...";
} else {
- LOG(ERROR) << "unknown trace format specifier: " << ch;
- JniAbort();
+ JniAbortF(function_name_, "unknown trace format specifier: %c", ch);
return;
}
if (*fmt) {
@@ -603,17 +606,15 @@
}
if (java_object == NULL) {
- LOG(ERROR) << "JNI ERROR: " << function_name_ << " received null " << what;
- JniAbort();
+ JniAbortF(function_name_, "%s received null %s", function_name_, what);
return false;
}
ScopedJniThreadState ts(env_);
Object* obj = Decode<Object*>(ts, java_object);
if (!Runtime::Current()->GetHeap()->IsHeapAddress(obj)) {
- LOG(ERROR) << "JNI ERROR: " << what << " is an invalid " << GetIndirectRefKind(java_object) << ": "
- << java_object << " (" << obj << ")";
- JniAbort();
+ JniAbortF(function_name_, "%s is an invalid %s: %p (%p)",
+ what, ToStr<IndirectRefKind>(GetIndirectRefKind(java_object)).c_str(), java_object, obj);
return false;
}
@@ -635,8 +636,7 @@
break;
}
if (!okay) {
- LOG(ERROR) << "JNI ERROR: " << what << " has wrong type: " << PrettyTypeOf(obj);
- JniAbort();
+ JniAbortF(function_name_, "%s has wrong type: %s", what, PrettyTypeOf(obj).c_str());
return false;
}
@@ -662,39 +662,34 @@
*/
void CheckArray(jarray java_array) {
if (java_array == NULL) {
- LOG(ERROR) << "JNI ERROR: received null array";
- JniAbort();
+ JniAbortF(function_name_, "jarray was NULL");
return;
}
ScopedJniThreadState ts(env_);
Array* a = Decode<Array*>(ts, java_array);
if (!Runtime::Current()->GetHeap()->IsHeapAddress(a)) {
- LOG(ERROR) << "JNI ERROR: jarray is an invalid " << GetIndirectRefKind(java_array) << ": " << reinterpret_cast<void*>(java_array) << " (" << a << ")";
- JniAbort();
+ JniAbortF(function_name_, "jarray is an invalid %s: %p (%p)",
+ ToStr<IndirectRefKind>(GetIndirectRefKind(java_array)).c_str(), java_array, a);
} else if (!a->IsArrayInstance()) {
- LOG(ERROR) << "JNI ERROR: jarray argument has non-array type: " << PrettyTypeOf(a);
- JniAbort();
+ JniAbortF(function_name_, "jarray argument has non-array type: %s", PrettyTypeOf(a).c_str());
}
}
void CheckLengthPositive(jsize length) {
if (length < 0) {
- LOG(ERROR) << "JNI ERROR: negative jsize: " << length;
- JniAbort();
+ JniAbortF(function_name_, "negative jsize: %d", length);
}
}
Field* CheckFieldID(jfieldID fid) {
if (fid == NULL) {
- LOG(ERROR) << "JNI ERROR: null jfieldID";
- JniAbort();
+ JniAbortF(function_name_, "jfieldID was NULL");
return NULL;
}
Field* f = DecodeField(fid);
if (!Runtime::Current()->GetHeap()->IsHeapAddress(f)) {
- LOG(ERROR) << "JNI ERROR: invalid jfieldID: " << fid;
- JniAbort();
+ JniAbortF(function_name_, "invalid jfieldID: %p", fid);
return NULL;
}
return f;
@@ -702,14 +697,12 @@
Method* CheckMethodID(jmethodID mid) {
if (mid == NULL) {
- LOG(ERROR) << "JNI ERROR: null jmethodID";
- JniAbort();
+ JniAbortF(function_name_, "jmethodID was NULL");
return NULL;
}
Method* m = DecodeMethod(mid);
if (!Runtime::Current()->GetHeap()->IsHeapAddress(m)) {
- LOG(ERROR) << "JNI ERROR: invalid jmethodID: " << mid;
- JniAbort();
+ JniAbortF(function_name_, "invalid jmethodID: %p", mid);
return NULL;
}
return m;
@@ -731,8 +724,8 @@
Object* o = Decode<Object*>(ts, java_object);
if (!Runtime::Current()->GetHeap()->IsHeapAddress(o)) {
// TODO: when we remove work_around_app_jni_bugs, this should be impossible.
- LOG(ERROR) << "JNI ERROR: native code passing in reference to invalid " << GetIndirectRefKind(java_object) << ": " << java_object;
- JniAbort();
+ JniAbortF(function_name_, "native code passing in reference to invalid %s: %p",
+ ToStr<IndirectRefKind>(GetIndirectRefKind(java_object)).c_str(), java_object);
}
}
@@ -742,58 +735,52 @@
*/
void CheckReleaseMode(jint mode) {
if (mode != 0 && mode != JNI_COMMIT && mode != JNI_ABORT) {
- LOG(ERROR) << "JNI ERROR: bad value for release mode: " << mode;
- JniAbort();
+ JniAbortF(function_name_, "bad value for release mode: %d", mode);
}
}
void CheckThread(int flags) {
Thread* self = Thread::Current();
if (self == NULL) {
- LOG(ERROR) << "JNI ERROR: a thread is making JNI calls without being attached";
- JniAbort();
+ JniAbortF(function_name_, "a thread (tid %d) is making JNI calls without being attached", GetTid());
return;
}
// Get the *correct* JNIEnv by going through our TLS pointer.
JNIEnvExt* threadEnv = self->GetJniEnv();
- /*
- * Verify that the current thread is (a) attached and (b) associated with
- * this particular instance of JNIEnv.
- */
+ // Verify that the current thread is (a) attached and (b) associated with
+ // this particular instance of JNIEnv.
if (env_ != threadEnv) {
- LOG(ERROR) << "JNI ERROR: thread " << *self << " using JNIEnv* from thread " << *env_->self;
- // If we're keeping broken code limping along, we need to suppress the abort...
- if (!vm_->work_around_app_jni_bugs) {
- JniAbort();
+ if (vm_->work_around_app_jni_bugs) {
+ // If we're keeping broken code limping along, we need to suppress the abort...
+ LOG(ERROR) << "APP BUG DETECTED: thread " << *self << " using JNIEnv* from thread " << *env_->self;
+ } else {
+ JniAbortF(function_name_, "thread %s using JNIEnv* from thread %s",
+ ToStr<Thread>(*self).c_str(), ToStr<Thread>(*env_->self).c_str());
return;
}
}
- /*
- * Verify that, if this thread previously made a critical "get" call, we
- * do the corresponding "release" call before we try anything else.
- */
+ // Verify that, if this thread previously made a critical "get" call, we
+ // do the corresponding "release" call before we try anything else.
switch (flags & kFlag_CritMask) {
case kFlag_CritOkay: // okay to call this method
break;
case kFlag_CritBad: // not okay to call
if (threadEnv->critical) {
- LOG(ERROR) << "JNI ERROR: thread " << *self << " using JNI after critical get";
- JniAbort();
+ JniAbortF(function_name_, "thread %s using JNI after critical get", ToStr<Thread>(*self).c_str());
return;
}
break;
case kFlag_CritGet: // this is a "get" call
- /* don't check here; we allow nested gets */
+ // Don't check here; we allow nested gets.
threadEnv->critical++;
break;
case kFlag_CritRelease: // this is a "release" call
threadEnv->critical--;
if (threadEnv->critical < 0) {
- LOG(ERROR) << "JNI ERROR: thread " << *self << " called too many critical releases";
- JniAbort();
+ JniAbortF(function_name_, "thread %s called too many critical releases", ToStr<Thread>(*self).c_str());
return;
}
break;
@@ -801,31 +788,27 @@
LOG(FATAL) << "Bad flags (internal error): " << flags;
}
- /*
- * Verify that, if an exception has been raised, the native code doesn't
- * make any JNI calls other than the Exception* methods.
- */
+ // Verify that, if an exception has been raised, the native code doesn't
+ // make any JNI calls other than the Exception* methods.
if ((flags & kFlag_ExcepOkay) == 0 && self->IsExceptionPending()) {
std::string type(PrettyTypeOf(self->GetException()));
- LOG(ERROR) << "JNI ERROR: JNI " << function_name_ << " called with " << type << " pending";
// TODO: write native code that doesn't require allocation for dumping an exception.
+ // TODO: do we care any more? art always dumps pending exceptions on aborting threads.
if (type != "java.lang.OutOfMemoryError") {
- LOG(ERROR) << "Pending exception is: ";
- LOG(ERROR) << jniGetStackTrace(env_);
+ JniAbortF(function_name_, "JNI %s called with pending exception: %s",
+ function_name_, type.c_str(), jniGetStackTrace(env_).c_str());
+ } else {
+ JniAbortF(function_name_, "JNI %s called with %s pending", function_name_, type.c_str());
}
- JniAbort();
return;
}
}
- /*
- * Verify that "bytes" points to valid Modified UTF-8 data.
- */
+ // Verifies that "bytes" points to valid Modified UTF-8 data.
void CheckUtfString(const char* bytes, bool nullable) {
if (bytes == NULL) {
if (!nullable) {
- LOG(ERROR) << "JNI ERROR: non-nullable const char* was NULL";
- JniAbort();
+ JniAbortF(function_name_, "non-nullable const char* was NULL");
return;
}
return;
@@ -834,10 +817,9 @@
const char* errorKind = NULL;
uint8_t utf8 = CheckUtfBytes(bytes, &errorKind);
if (errorKind != NULL) {
- LOG(ERROR) << "JNI ERROR: input is not valid Modified UTF-8: "
- << "illegal " << errorKind << " byte " << StringPrintf("%#x", utf8) << "\n"
- << " string: '" << bytes << "'";
- JniAbort();
+ JniAbortF(function_name_,
+ "input is not valid Modified UTF-8: illegal %s byte %#x\n"
+ " string: '%s'", errorKind, utf8, bytes);
return;
}
}
@@ -891,10 +873,6 @@
return 0;
}
- void JniAbort() {
- ::art::JniAbort(function_name_);
- }
-
JNIEnvExt* env_;
JavaVMExt* vm_;
const char* function_name_;
@@ -948,16 +926,16 @@
size_t newLen = ActualLength(len);
uint8_t* newBuf = DebugAlloc(newLen);
- /* fill it in with a pattern */
+ // Fill it in with a pattern.
uint16_t* pat = reinterpret_cast<uint16_t*>(newBuf);
for (size_t i = 0; i < newLen / 2; i++) {
*pat++ = kGuardPattern;
}
- /* copy the data in; note "len" could be zero */
+ // Copy the data in; note "len" could be zero.
memcpy(newBuf + kGuardLen / 2, buf, len);
- /* if modification is not expected, grab a checksum */
+ // If modification is not expected, grab a checksum.
uLong adler = 0;
if (!modOkay) {
adler = adler32(0L, Z_NULL, 0);
@@ -996,65 +974,57 @@
const uint8_t* fullBuf = ActualBuffer(dataBuf);
const GuardedCopy* pExtra = GuardedCopy::FromData(dataBuf);
- /*
- * Before we do anything with "pExtra", check the magic number. We
- * do the check with memcmp rather than "==" in case the pointer is
- * unaligned. If it points to completely bogus memory we're going
- * to crash, but there's no easy way around that.
- */
+ // Before we do anything with "pExtra", check the magic number. We
+ // do the check with memcmp rather than "==" in case the pointer is
+ // unaligned. If it points to completely bogus memory we're going
+ // to crash, but there's no easy way around that.
if (memcmp(&pExtra->magic, &kMagicCmp, 4) != 0) {
uint8_t buf[4];
memcpy(buf, &pExtra->magic, 4);
- LOG(ERROR) << StringPrintf("JNI: guard magic does not match "
- "(found 0x%02x%02x%02x%02x) -- incorrect data pointer %p?",
- buf[3], buf[2], buf[1], buf[0], dataBuf); /* assume little endian */
- JniAbort(functionName);
+ JniAbortF(functionName,
+ "guard magic does not match (found 0x%02x%02x%02x%02x) -- incorrect data pointer %p?",
+ buf[3], buf[2], buf[1], buf[0], dataBuf); // Assumes little-endian.
}
size_t len = pExtra->original_length;
- /* check bottom half of guard; skip over optional checksum storage */
+ // Check bottom half of guard; skip over optional checksum storage.
const uint16_t* pat = reinterpret_cast<const uint16_t*>(fullBuf);
for (size_t i = sizeof(GuardedCopy) / 2; i < (kGuardLen / 2 - sizeof(GuardedCopy)) / 2; i++) {
if (pat[i] != kGuardPattern) {
- LOG(ERROR) << "JNI: guard pattern(1) disturbed at " << reinterpret_cast<const void*>(fullBuf) << " + " << (i*2);
- JniAbort(functionName);
+ JniAbortF(functionName, "guard pattern(1) disturbed at %p +%d", fullBuf, i*2);
}
}
int offset = kGuardLen / 2 + len;
if (offset & 0x01) {
- /* odd byte; expected value depends on endian-ness of host */
+ // Odd byte; expected value depends on endian.
const uint16_t patSample = kGuardPattern;
- if (fullBuf[offset] != reinterpret_cast<const uint8_t*>(&patSample)[1]) {
- LOG(ERROR) << "JNI: guard pattern disturbed in odd byte after "
- << reinterpret_cast<const void*>(fullBuf) << " (+" << offset << ") "
- << StringPrintf("0x%02x 0x%02x", fullBuf[offset], ((const uint8_t*) &patSample)[1]);
- JniAbort(functionName);
+ uint8_t expected_byte = reinterpret_cast<const uint8_t*>(&patSample)[1];
+ if (fullBuf[offset] != expected_byte) {
+ JniAbortF(functionName, "guard pattern disturbed in odd byte after %p +%d 0x%02x 0x%02x",
+ fullBuf, offset, fullBuf[offset], expected_byte);
}
offset++;
}
- /* check top half of guard */
+ // Check top half of guard.
pat = reinterpret_cast<const uint16_t*>(fullBuf + offset);
for (size_t i = 0; i < kGuardLen / 4; i++) {
if (pat[i] != kGuardPattern) {
- LOG(ERROR) << "JNI: guard pattern(2) disturbed at " << reinterpret_cast<const void*>(fullBuf) << " + " << (offset + i*2);
- JniAbort(functionName);
+ JniAbortF(functionName, "guard pattern(2) disturbed at %p +%d", fullBuf, offset + i*2);
}
}
- /*
- * If modification is not expected, verify checksum. Strictly speaking
- * this is wrong: if we told the client that we made a copy, there's no
- * reason they can't alter the buffer.
- */
+ // If modification is not expected, verify checksum. Strictly speaking
+ // this is wrong: if we told the client that we made a copy, there's no
+ // reason they can't alter the buffer.
if (!modOkay) {
uLong adler = adler32(0L, Z_NULL, 0);
adler = adler32(adler, (const Bytef*)dataBuf, len);
if (pExtra->adler != adler) {
- LOG(ERROR) << StringPrintf("JNI: buffer modified (0x%08lx vs 0x%08lx) at addr %p", pExtra->adler, adler, dataBuf);
- JniAbort(functionName);
+ JniAbortF(functionName, "buffer modified (0x%08lx vs 0x%08lx) at address %p",
+ pExtra->adler, adler, dataBuf);
}
}
}
@@ -1099,7 +1069,7 @@
* Create a guarded copy of a primitive array. Modifications to the copied
* data are allowed. Returns a pointer to the copied data.
*/
-void* CreateGuardedPACopy(JNIEnv* env, const jarray java_array, jboolean* isCopy) {
+static void* CreateGuardedPACopy(JNIEnv* env, const jarray java_array, jboolean* isCopy) {
ScopedJniThreadState ts(env);
Array* a = Decode<Array*>(ts, java_array);
@@ -1116,7 +1086,7 @@
* Perform the array "release" operation, which may or may not copy data
* back into the managed heap, and may or may not release the underlying storage.
*/
-void ReleaseGuardedPACopy(JNIEnv* env, jarray java_array, void* dataBuf, int mode) {
+static void ReleaseGuardedPACopy(JNIEnv* env, jarray java_array, void* dataBuf, int mode) {
if (reinterpret_cast<uintptr_t>(dataBuf) == kNoCopyMagic) {
return;
}
@@ -1249,8 +1219,8 @@
static void DeleteGlobalRef(JNIEnv* env, jobject globalRef) {
CHECK_JNI_ENTRY(kFlag_Default | kFlag_ExcepOkay, "EL", env, globalRef);
if (globalRef != NULL && GetIndirectRefKind(globalRef) != kGlobal) {
- LOG(ERROR) << "JNI ERROR: DeleteGlobalRef on " << GetIndirectRefKind(globalRef) << ": " << globalRef;
- JniAbort(__FUNCTION__);
+ JniAbortF(__FUNCTION__, "DeleteGlobalRef on %s: %p",
+ ToStr<IndirectRefKind>(GetIndirectRefKind(globalRef)).c_str(), globalRef);
} else {
baseEnv(env)->DeleteGlobalRef(env, globalRef);
CHECK_JNI_EXIT_VOID();
@@ -1260,8 +1230,8 @@
static void DeleteWeakGlobalRef(JNIEnv* env, jweak weakGlobalRef) {
CHECK_JNI_ENTRY(kFlag_Default | kFlag_ExcepOkay, "EL", env, weakGlobalRef);
if (weakGlobalRef != NULL && GetIndirectRefKind(weakGlobalRef) != kWeakGlobal) {
- LOG(ERROR) << "JNI ERROR: DeleteWeakGlobalRef on " << GetIndirectRefKind(weakGlobalRef) << ": " << weakGlobalRef;
- JniAbort(__FUNCTION__);
+ JniAbortF(__FUNCTION__, "DeleteWeakGlobalRef on %s: %p",
+ ToStr<IndirectRefKind>(GetIndirectRefKind(weakGlobalRef)).c_str(), weakGlobalRef);
} else {
baseEnv(env)->DeleteWeakGlobalRef(env, weakGlobalRef);
CHECK_JNI_EXIT_VOID();
@@ -1271,8 +1241,8 @@
static void DeleteLocalRef(JNIEnv* env, jobject localRef) {
CHECK_JNI_ENTRY(kFlag_Default | kFlag_ExcepOkay, "EL", env, localRef);
if (localRef != NULL && GetIndirectRefKind(localRef) != kLocal && !IsSirtLocalRef(env, localRef)) {
- LOG(ERROR) << "JNI ERROR: DeleteLocalRef on " << GetIndirectRefKind(localRef) << ": " << localRef;
- JniAbort(__FUNCTION__);
+ JniAbortF(__FUNCTION__, "DeleteLocalRef on %s: %p",
+ ToStr<IndirectRefKind>(GetIndirectRefKind(localRef)).c_str(), localRef);
} else {
baseEnv(env)->DeleteLocalRef(env, localRef);
CHECK_JNI_EXIT_VOID();
@@ -1605,7 +1575,7 @@
force_copy = sc.ForceCopy();
no_copy = 0;
if (force_copy && isCopy != NULL) {
- /* capture this before the base call tramples on it */
+ // Capture this before the base call tramples on it.
no_copy = *reinterpret_cast<uint32_t*>(isCopy);
}
}
@@ -1662,7 +1632,7 @@
GET_PRIMITIVE_ARRAY_REGION(_ctype, _jname); \
SET_PRIMITIVE_ARRAY_REGION(_ctype, _jname);
-/* TODO: verify primitive array type matches call type */
+// TODO: verify primitive array type matches call type.
PRIMITIVE_ARRAY_FUNCTIONS(jboolean, Boolean, 'Z');
PRIMITIVE_ARRAY_FUNCTIONS(jbyte, Byte, 'B');
PRIMITIVE_ARRAY_FUNCTIONS(jchar, Char, 'C');
@@ -1781,12 +1751,10 @@
static jobject NewDirectByteBuffer(JNIEnv* env, void* address, jlong capacity) {
CHECK_JNI_ENTRY(kFlag_Default, "EpJ", env, address, capacity);
if (address == NULL) {
- LOG(ERROR) << "JNI ERROR: non-nullable address is NULL";
- JniAbort(__FUNCTION__);
+ JniAbortF(__FUNCTION__, "non-nullable address is NULL");
}
if (capacity <= 0) {
- LOG(ERROR) << "JNI ERROR: capacity must be greater than 0: " << capacity;
- JniAbort(__FUNCTION__);
+ JniAbortF(__FUNCTION__, "capacity must be greater than 0: %d", capacity);
}
return CHECK_JNI_EXIT("L", baseEnv(env)->NewDirectByteBuffer(env, address, capacity));
}
diff --git a/src/common_test.h b/src/common_test.h
index 6b284fd..54c9bb7 100644
--- a/src/common_test.h
+++ b/src/common_test.h
@@ -548,11 +548,13 @@
EXPECT_TRUE(actual_.find(expected_text) != std::string::npos) << "\n"
<< "Expected to find: " << expected_text << "\n"
<< "In the output : " << actual_;
+ actual_.clear();
}
private:
static void Hook(void* data, const std::string& reason) {
- *reinterpret_cast<std::string*>(data) = reason;
+ // We use += because when we're hooking the aborts like this, multiple problems can be found.
+ *reinterpret_cast<std::string*>(data) += reason;
}
JavaVMExt* vm_;
diff --git a/src/compiler_llvm/runtime_support_llvm.cc b/src/compiler_llvm/runtime_support_llvm.cc
index 2236525..27541c3 100644
--- a/src/compiler_llvm/runtime_support_llvm.cc
+++ b/src/compiler_llvm/runtime_support_llvm.cc
@@ -507,9 +507,8 @@
}
if (o == kInvalidIndirectRefObject) {
- LOG(ERROR) << "JNI ERROR (app bug): invalid reference returned from "
- << PrettyMethod(self->GetCurrentMethod());
- JniAbort(NULL);
+ JniAbortF(NULL, "invalid reference returned from %s",
+ PrettyMethod(self->GetCurrentMethod()).c_str());
}
// Make sure that the result is an instance of the type this
@@ -519,9 +518,8 @@
Class* return_type = mh.GetReturnType();
if (!o->InstanceOf(return_type)) {
- LOG(ERROR) << "JNI ERROR (app bug): attempt to return an instance of " << PrettyTypeOf(o)
- << " from " << PrettyMethod(m);
- JniAbort(NULL);
+ JniAbortF(NULL, "attempt to return an instance of %s from %s",
+ PrettyTypeOf(o).c_str(), PrettyMethod(m).c_str());
}
return o;
diff --git a/src/jni_compiler_test.cc b/src/jni_compiler_test.cc
index 61e1867..c5710f8 100644
--- a/src/jni_compiler_test.cc
+++ b/src/jni_compiler_test.cc
@@ -673,18 +673,17 @@
SetUpForTest(class_loader.get(), false, "instanceMethodThatShouldReturnClass", "()Ljava/lang/Class;",
reinterpret_cast<void*>(&Java_MyClassNatives_instanceMethodThatShouldReturnClass));
- {
- CheckJniAbortCatcher check_jni_abort_catcher;
- // TODO: check type of returns with portable JNI compiler.
- // This native method is bad, and tries to return a jstring as a jclass.
- env_->CallObjectMethod(jobj_, jmethod_);
- check_jni_abort_catcher.Check("java.lang.Class MyClassNatives.instanceMethodThatShouldReturnClass");
- }
-
CheckJniAbortCatcher check_jni_abort_catcher;
- // Here, we just call the method wrong; we should catch that too.
+ // TODO: check type of returns with portable JNI compiler.
+ // This native method is bad, and tries to return a jstring as a jclass.
+ env_->CallObjectMethod(jobj_, jmethod_);
+ check_jni_abort_catcher.Check("attempt to return an instance of java.lang.String from java.lang.Class MyClassNatives.instanceMethodThatShouldReturnClass()");
+
+ // Here, we just call the method incorrectly; we should catch that too.
env_->CallVoidMethod(jobj_, jmethod_);
- check_jni_abort_catcher.Check("Aborting because JNI app bug detected");
+ check_jni_abort_catcher.Check("attempt to return an instance of java.lang.String from java.lang.Class MyClassNatives.instanceMethodThatShouldReturnClass()");
+ env_->CallStaticVoidMethod(jklass_, jmethod_);
+ check_jni_abort_catcher.Check("calling non-static method java.lang.Class MyClassNatives.instanceMethodThatShouldReturnClass() with CallStaticVoidMethodV");
}
TEST_F(JniCompilerTest, UpcallReturnTypeChecking_Static) {
@@ -692,18 +691,17 @@
SetUpForTest(class_loader.get(), true, "staticMethodThatShouldReturnClass", "()Ljava/lang/Class;",
reinterpret_cast<void*>(&Java_MyClassNatives_staticMethodThatShouldReturnClass));
- {
- CheckJniAbortCatcher check_jni_abort_catcher;
- // TODO: check type of returns with portable JNI compiler.
- // This native method is bad, and tries to return a jstring as a jclass.
- env_->CallStaticObjectMethod(jklass_, jmethod_);
- check_jni_abort_catcher.Check("java.lang.Class MyClassNatives.staticMethodThatShouldReturnClass");
- }
-
CheckJniAbortCatcher check_jni_abort_catcher;
- // Here, we just call the method wrong; we should catch that too.
+ // TODO: check type of returns with portable JNI compiler.
+ // This native method is bad, and tries to return a jstring as a jclass.
+ env_->CallStaticObjectMethod(jklass_, jmethod_);
+ check_jni_abort_catcher.Check("attempt to return an instance of java.lang.String from java.lang.Class MyClassNatives.staticMethodThatShouldReturnClass()");
+
+ // Here, we just call the method incorrectly; we should catch that too.
+ env_->CallStaticVoidMethod(jklass_, jmethod_);
+ check_jni_abort_catcher.Check("attempt to return an instance of java.lang.String from java.lang.Class MyClassNatives.staticMethodThatShouldReturnClass()");
env_->CallVoidMethod(jobj_, jmethod_);
- check_jni_abort_catcher.Check("Aborting because JNI app bug detected");
+ check_jni_abort_catcher.Check("calling static method java.lang.Class MyClassNatives.staticMethodThatShouldReturnClass() with CallVoidMethodV");
}
// This should take jclass, but we're imitating a bug pattern.
@@ -722,7 +720,7 @@
CheckJniAbortCatcher check_jni_abort_catcher;
// We deliberately pass a bad second argument here.
env_->CallVoidMethod(jobj_, jmethod_, 123, env_->NewStringUTF("not a class!"));
- check_jni_abort_catcher.Check("Aborting because JNI app bug detected");
+ check_jni_abort_catcher.Check("bad arguments passed to void MyClassNatives.instanceMethodThatShouldTakeClass(int, java.lang.Class)");
}
TEST_F(JniCompilerTest, UpcallArgumentTypeChecking_Static) {
@@ -733,7 +731,7 @@
CheckJniAbortCatcher check_jni_abort_catcher;
// We deliberately pass a bad second argument here.
env_->CallStaticVoidMethod(jklass_, jmethod_, 123, env_->NewStringUTF("not a class!"));
- check_jni_abort_catcher.Check("Aborting because JNI app bug detected");
+ check_jni_abort_catcher.Check("bad arguments passed to void MyClassNatives.staticMethodThatShouldTakeClass(int, java.lang.Class)");
}
} // namespace art
diff --git a/src/jni_internal.cc b/src/jni_internal.cc
index 82fba54..ed46837 100644
--- a/src/jni_internal.cc
+++ b/src/jni_internal.cc
@@ -302,7 +302,7 @@
if (error_count > 0) {
// TODO: pass the JNI function name (such as "CallVoidMethodV") through so we can call JniAbort
// with an argument.
- JniAbort(NULL);
+ JniAbortF(NULL, "bad arguments passed to %s (see above for details)", PrettyMethod(m).c_str());
}
}
diff --git a/src/jni_internal.h b/src/jni_internal.h
index e62d62a..be5bca0 100644
--- a/src/jni_internal.h
+++ b/src/jni_internal.h
@@ -46,7 +46,7 @@
class Thread;
void SetJniGlobalsMax(size_t max);
-void JniAbort(const char* jni_function_name);
+void JniAbortF(const char* jni_function_name, const char* fmt, ...);
void* FindNativeMethod(Thread* thread);
void RegisterNativeMethods(JNIEnv* env, const char* jni_class_name, const JNINativeMethod* methods, size_t method_count);
diff --git a/src/jni_internal_test.cc b/src/jni_internal_test.cc
index 2ca2b3c..9383db3 100644
--- a/src/jni_internal_test.cc
+++ b/src/jni_internal_test.cc
@@ -547,28 +547,33 @@
EXPECT_CLASS_FOUND("java/lang/String");
// ...for arrays too, where you must include "L;".
EXPECT_CLASS_FOUND("[Ljava/lang/String;");
+ // Primitive arrays are okay too, if the primitive type is valid.
+ EXPECT_CLASS_FOUND("[C");
{
- CheckJniAbortCatcher check_jni_abort_catcher;
-
// We support . as well as / for compatibility, if -Xcheck:jni is off.
+ CheckJniAbortCatcher check_jni_abort_catcher;
EXPECT_CLASS_FOUND("java.lang.String");
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: illegal class name 'java.lang.String'");
EXPECT_CLASS_NOT_FOUND("Ljava.lang.String;");
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: illegal class name 'Ljava.lang.String;'");
EXPECT_CLASS_FOUND("[Ljava.lang.String;");
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: illegal class name '[Ljava.lang.String;'");
EXPECT_CLASS_NOT_FOUND("[java.lang.String");
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: illegal class name '[java.lang.String'");
// You can't include the "L;" in a JNI class descriptor.
EXPECT_CLASS_NOT_FOUND("Ljava/lang/String;");
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: illegal class name 'Ljava/lang/String;'");
+
// But you must include it for an array of any reference type.
EXPECT_CLASS_NOT_FOUND("[java/lang/String");
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: illegal class name '[java/lang/String'");
+
+ EXPECT_CLASS_NOT_FOUND("[K");
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: illegal class name '[K'");
}
- // Primitive arrays are okay (if the primitive type is valid)...
- EXPECT_CLASS_FOUND("[C");
- {
- CheckJniAbortCatcher check_jni_abort_catcher;
- EXPECT_CLASS_NOT_FOUND("[K");
- }
// But primitive types aren't allowed...
EXPECT_CLASS_NOT_FOUND("C");
EXPECT_CLASS_NOT_FOUND("K");
@@ -1022,10 +1027,11 @@
}
TEST_F(JniInternalTest, GetStringUTFChars_ReleaseStringUTFChars) {
+ // Passing in a NULL jstring is ignored normally, but caught by -Xcheck:jni.
{
- // Passing in a NULL jstring is ignored normally, but caught by -Xcheck:jni.
CheckJniAbortCatcher check_jni_abort_catcher;
EXPECT_TRUE(env_->GetStringUTFChars(NULL, NULL) == NULL);
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: GetStringUTFChars received null jstring");
}
jstring s = env_->NewStringUTF("hello");
@@ -1217,10 +1223,11 @@
ASSERT_TRUE(s != NULL);
env_->DeleteLocalRef(s);
+ // Currently, deleting an already-deleted reference is just a CheckJNI warning.
{
- // Currently, deleting an already-deleted reference is just a CheckJNI warning.
CheckJniAbortCatcher check_jni_abort_catcher;
env_->DeleteLocalRef(s);
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: native code passing in reference to invalid local reference: 0x200001");
}
s = env_->NewStringUTF("");
@@ -1296,10 +1303,11 @@
ASSERT_TRUE(o != NULL);
env_->DeleteGlobalRef(o);
+ // Currently, deleting an already-deleted reference is just a CheckJNI warning.
{
- // Currently, deleting an already-deleted reference is just a CheckJNI warning.
CheckJniAbortCatcher check_jni_abort_catcher;
env_->DeleteGlobalRef(o);
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: native code passing in reference to invalid global reference: 0x10000e");
}
jobject o1 = env_->NewGlobalRef(s);
@@ -1337,10 +1345,11 @@
ASSERT_TRUE(o != NULL);
env_->DeleteWeakGlobalRef(o);
+ // Currently, deleting an already-deleted reference is just a CheckJNI warning.
{
- // Currently, deleting an already-deleted reference is just a CheckJNI warning.
CheckJniAbortCatcher check_jni_abort_catcher;
env_->DeleteWeakGlobalRef(o);
+ check_jni_abort_catcher.Check("JNI ERROR: app bug found: native code passing in reference to invalid weak global reference: 0x100003");
}
jobject o1 = env_->NewWeakGlobalRef(s);
@@ -1566,10 +1575,7 @@
CheckJniAbortCatcher check_jni_abort_catcher;
env_->MonitorEnter(NULL);
check_jni_abort_catcher.Check("in call to MonitorEnter");
- }
- {
- CheckJniAbortCatcher check_jni_abort_catcher;
env_->MonitorExit(NULL);
check_jni_abort_catcher.Check("in call to MonitorExit");
}
diff --git a/src/logging.h b/src/logging.h
index f7b687b..585cc6d 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -249,6 +249,30 @@
return os;
}
+// Helps you use operator<< in a const char*-like context such as our various 'F' methods with
+// format strings.
+template<typename T>
+class ToStr {
+ public:
+ ToStr(const T& value) {
+ std::ostringstream os;
+ os << value;
+ s_ = os.str();
+ }
+
+ const char* c_str() const {
+ return s_.c_str();
+ }
+
+ const std::string& str() const {
+ return s_;
+ }
+
+ private:
+ std::string s_;
+ DISALLOW_COPY_AND_ASSIGN(ToStr);
+};
+
// The members of this struct are the valid arguments to VLOG and VLOG_IS_ON in code,
// and the "-verbose:" command line argument.
struct LogVerbosity {
diff --git a/src/oat/runtime/support_jni.cc b/src/oat/runtime/support_jni.cc
index e6e6478..fb8b2e0 100644
--- a/src/oat/runtime/support_jni.cc
+++ b/src/oat/runtime/support_jni.cc
@@ -51,9 +51,8 @@
}
if (o == kInvalidIndirectRefObject) {
- LOG(ERROR) << "JNI ERROR (app bug): invalid reference returned from "
- << PrettyMethod(self->GetCurrentMethod());
- JniAbort(NULL);
+ JniAbortF(NULL, "invalid reference returned from %s",
+ PrettyMethod(self->GetCurrentMethod()).c_str());
}
// Make sure that the result is an instance of the type this
@@ -63,9 +62,8 @@
Class* return_type = mh.GetReturnType();
if (!o->InstanceOf(return_type)) {
- LOG(ERROR) << "JNI ERROR (app bug): attempt to return an instance of " << PrettyTypeOf(o)
- << " from " << PrettyMethod(m);
- JniAbort(NULL);
+ JniAbortF(NULL, "attempt to return an instance of %s from %s",
+ PrettyTypeOf(o).c_str(), PrettyMethod(m).c_str());
}
return o;
diff --git a/src/scoped_jni_thread_state.h b/src/scoped_jni_thread_state.h
index fdf6581..7ef92d4 100644
--- a/src/scoped_jni_thread_state.h
+++ b/src/scoped_jni_thread_state.h
@@ -55,9 +55,9 @@
Thread* env_self = full_env->self;
Thread* self = work_around_app_jni_bugs ? Thread::Current() : env_self;
if (!work_around_app_jni_bugs && self != env_self) {
- LOG(FATAL) << "JNI ERROR (app bug): JNIEnv for " << *env_self
- << " used on " << *self;
- // TODO: pass JNI function through so we can call JniAbort(function_name) instead.
+ // TODO: pass through function name so we can use it here instead of NULL...
+ JniAbortF(NULL, "JNIEnv for %s used on %s",
+ ToStr<Thread>(*env_self).c_str(), ToStr<Thread>(*self).c_str());
}
return self;
}
diff --git a/src/thread.cc b/src/thread.cc
index a87e550..60f0199 100644
--- a/src/thread.cc
+++ b/src/thread.cc
@@ -1062,8 +1062,7 @@
}
if (result == NULL) {
- LOG(ERROR) << "JNI ERROR (app bug): use of deleted " << kind << ": " << obj;
- JniAbort(NULL);
+ JniAbortF(NULL, "use of deleted %s %p", ToStr<IndirectRefKind>(kind).c_str(), obj);
} else {
if (result != kInvalidIndirectRefObject) {
Runtime::Current()->GetHeap()->VerifyObject(result);
