Add check JNI tests for byte and 16-bit primitives.
Make boolean checking check 32-bit values in case of truncation.
Add unit test for out-of-bound primitive call values.
Make reflection's ArgArray anonymous as per more recent cppstyle and
address other minor style issues.
Test: mma -j32 test-art-host; boot (eng) emulator and check logcat is clean
Bug: 73656264
Change-Id: If7947e24ec3fe3303c1d3d0545605e82f651de25
diff --git a/runtime/check_jni.cc b/runtime/check_jni.cc
index 05f099f..02580cc 100644
--- a/runtime/check_jni.cc
+++ b/runtime/check_jni.cc
@@ -61,6 +61,10 @@
static constexpr uint64_t kCriticalWarnTimeUs = MsToUs(16);
static_assert(kCriticalWarnTimeUs > 0, "No JNI critical warn time set");
+// True if primitives within specific ranges cause a fatal error,
+// otherwise just warn.
+static constexpr bool kBrokenPrimitivesAreFatal = kIsDebugBuild;
+
// Flags passed into ScopedCheck.
static constexpr uint16_t kFlag_Default = 0x0000;
@@ -206,10 +210,12 @@
JniValueType o;
if (type_ == kTypeVaList) {
switch (fmt) {
- case 'Z': o.Z = static_cast<jboolean>(va_arg(vargs_, jint)); break;
- case 'B': o.B = static_cast<jbyte>(va_arg(vargs_, jint)); break;
- case 'C': o.C = static_cast<jchar>(va_arg(vargs_, jint)); break;
- case 'S': o.S = static_cast<jshort>(va_arg(vargs_, jint)); break;
+ // Assign a full int for va_list values as this is what is done in reflection.cc.
+ // TODO(b/73656264): avoid undefined behavior.
+ case 'Z': FALLTHROUGH_INTENDED;
+ case 'B': FALLTHROUGH_INTENDED;
+ case 'C': FALLTHROUGH_INTENDED;
+ case 'S': FALLTHROUGH_INTENDED;
case 'I': o.I = va_arg(vargs_, jint); break;
case 'J': o.J = va_arg(vargs_, jlong); break;
case 'F': o.F = static_cast<jfloat>(va_arg(vargs_, jdouble)); break;
@@ -224,10 +230,14 @@
jvalue v = ptr_[cnt_];
cnt_++;
switch (fmt) {
- case 'Z': o.Z = v.z; break;
- case 'B': o.B = v.b; break;
- case 'C': o.C = v.c; break;
- case 'S': o.S = v.s; break;
+ // Copy just the amount of the jvalue necessary, as done in
+ // reflection.cc, but extend to an int to be consistent with
+ // var args in CheckNonHeapValue.
+ // TODO(b/73656264): avoid undefined behavior.
+ case 'Z': o.I = v.z; break;
+ case 'B': o.I = v.b; break;
+ case 'C': o.I = v.c; break;
+ case 'S': o.I = v.s; break;
case 'I': o.I = v.i; break;
case 'J': o.J = v.j; break;
case 'F': o.F = v.f; break;
@@ -911,17 +921,20 @@
switch (fmt) {
case 'p': // TODO: pointer - null or readable?
case 'v': // JavaVM*
- case 'B': // jbyte
- case 'C': // jchar
case 'D': // jdouble
case 'F': // jfloat
- case 'I': // jint
case 'J': // jlong
- case 'S': // jshort
+ case 'I': // jint
break; // Ignored.
case 'b': // jboolean, why two? Fall-through.
case 'Z':
- return CheckBoolean(arg.Z);
+ return CheckBoolean(arg.I);
+ case 'B': // jbyte
+ return CheckByte(arg.I);
+ case 'C': // jchar
+ return CheckChar(arg.I);
+ case 'S': // jshort
+ return CheckShort(arg.I);
case 'u': // utf8
if ((flags_ & kFlag_Release) != 0) {
return CheckNonNull(arg.u);
@@ -1152,14 +1165,54 @@
return true;
}
- bool CheckBoolean(jboolean z) {
+ bool CheckBoolean(jint z) {
if (z != JNI_TRUE && z != JNI_FALSE) {
+ // Note, broken booleans are always fatal.
AbortF("unexpected jboolean value: %d", z);
return false;
}
return true;
}
+ bool CheckByte(jint b) {
+ if (b < std::numeric_limits<jbyte>::min() ||
+ b > std::numeric_limits<jbyte>::max()) {
+ if (kBrokenPrimitivesAreFatal) {
+ AbortF("unexpected jbyte value: %d", b);
+ return false;
+ } else {
+ LOG(WARNING) << "Unexpected jbyte value: " << b;
+ }
+ }
+ return true;
+ }
+
+ bool CheckShort(jint s) {
+ if (s < std::numeric_limits<jshort>::min() ||
+ s > std::numeric_limits<jshort>::max()) {
+ if (kBrokenPrimitivesAreFatal) {
+ AbortF("unexpected jshort value: %d", s);
+ return false;
+ } else {
+ LOG(WARNING) << "Unexpected jshort value: " << s;
+ }
+ }
+ return true;
+ }
+
+ bool CheckChar(jint c) {
+ if (c < std::numeric_limits<jchar>::min() ||
+ c > std::numeric_limits<jchar>::max()) {
+ if (kBrokenPrimitivesAreFatal) {
+ AbortF("unexpected jchar value: %d", c);
+ return false;
+ } else {
+ LOG(WARNING) << "Unexpected jchar value: " << c;
+ }
+ }
+ return true;
+ }
+
bool CheckLengthPositive(jsize length) {
if (length < 0) {
AbortF("negative jsize: %d", length);
@@ -1813,7 +1866,7 @@
static jobject ToReflectedMethod(JNIEnv* env, jclass cls, jmethodID mid, jboolean isStatic) {
ScopedObjectAccess soa(env);
ScopedCheck sc(kFlag_Default, __FUNCTION__);
- JniValueType args[4] = {{.E = env}, {.c = cls}, {.m = mid}, {.b = isStatic}};
+ JniValueType args[4] = {{.E = env}, {.c = cls}, {.m = mid}, {.I = isStatic}};
if (sc.Check(soa, true, "Ecmb", args)) {
JniValueType result;
result.L = baseEnv(env)->ToReflectedMethod(env, cls, mid, isStatic);
@@ -1828,7 +1881,7 @@
static jobject ToReflectedField(JNIEnv* env, jclass cls, jfieldID fid, jboolean isStatic) {
ScopedObjectAccess soa(env);
ScopedCheck sc(kFlag_Default, __FUNCTION__);
- JniValueType args[4] = {{.E = env}, {.c = cls}, {.f = fid}, {.b = isStatic}};
+ JniValueType args[4] = {{.E = env}, {.c = cls}, {.f = fid}, {.I = isStatic}};
if (sc.Check(soa, true, "Ecfb", args)) {
JniValueType result;
result.L = baseEnv(env)->ToReflectedField(env, cls, fid, isStatic);
@@ -2113,7 +2166,7 @@
return GetFieldIDInternal(__FUNCTION__, env, c, name, sig, true);
}
-#define FIELD_ACCESSORS(jtype, name, ptype, shorty) \
+#define FIELD_ACCESSORS(jtype, name, ptype, shorty, slot_sized_shorty) \
static jtype GetStatic##name##Field(JNIEnv* env, jclass c, jfieldID fid) { \
return GetField(__FUNCTION__, env, c, fid, true, ptype).shorty; \
} \
@@ -2124,25 +2177,25 @@
\
static void SetStatic##name##Field(JNIEnv* env, jclass c, jfieldID fid, jtype v) { \
JniValueType value; \
- value.shorty = v; \
+ value.slot_sized_shorty = v; \
SetField(__FUNCTION__, env, c, fid, true, ptype, value); \
} \
\
static void Set##name##Field(JNIEnv* env, jobject obj, jfieldID fid, jtype v) { \
JniValueType value; \
- value.shorty = v; \
+ value.slot_sized_shorty = v; \
SetField(__FUNCTION__, env, obj, fid, false, ptype, value); \
}
- FIELD_ACCESSORS(jobject, Object, Primitive::kPrimNot, L)
- FIELD_ACCESSORS(jboolean, Boolean, Primitive::kPrimBoolean, Z)
- FIELD_ACCESSORS(jbyte, Byte, Primitive::kPrimByte, B)
- FIELD_ACCESSORS(jchar, Char, Primitive::kPrimChar, C)
- FIELD_ACCESSORS(jshort, Short, Primitive::kPrimShort, S)
- FIELD_ACCESSORS(jint, Int, Primitive::kPrimInt, I)
- FIELD_ACCESSORS(jlong, Long, Primitive::kPrimLong, J)
- FIELD_ACCESSORS(jfloat, Float, Primitive::kPrimFloat, F)
- FIELD_ACCESSORS(jdouble, Double, Primitive::kPrimDouble, D)
+ FIELD_ACCESSORS(jobject, Object, Primitive::kPrimNot, L, L)
+ FIELD_ACCESSORS(jboolean, Boolean, Primitive::kPrimBoolean, Z, I)
+ FIELD_ACCESSORS(jbyte, Byte, Primitive::kPrimByte, B, I)
+ FIELD_ACCESSORS(jchar, Char, Primitive::kPrimChar, C, I)
+ FIELD_ACCESSORS(jshort, Short, Primitive::kPrimShort, S, I)
+ FIELD_ACCESSORS(jint, Int, Primitive::kPrimInt, I, I)
+ FIELD_ACCESSORS(jlong, Long, Primitive::kPrimLong, J, J)
+ FIELD_ACCESSORS(jfloat, Float, Primitive::kPrimFloat, F, F)
+ FIELD_ACCESSORS(jdouble, Double, Primitive::kPrimDouble, D, D)
#undef FIELD_ACCESSORS
static void CallVoidMethodA(JNIEnv* env, jobject obj, jmethodID mid, jvalue* vargs) {
diff --git a/runtime/jni_internal_test.cc b/runtime/jni_internal_test.cc
index ad24c94..6300038 100644
--- a/runtime/jni_internal_test.cc
+++ b/runtime/jni_internal_test.cc
@@ -835,6 +835,73 @@
check_jni_abort_catcher.Check("null");
}
+TEST_F(JniInternalTest, CallVarArgMethodBadPrimitive) {
+ // Check that bad primitive values cause check JNI to abort when
+ // passed out-of-range primitive value var args. As var args can't
+ // differentiate type sizes less than an int, and this isn't
+ // corrected by JNI, this helps ensure JNI code is valid.
+#define DoCall(boxed_type, shorty, c_type, bad_value) \
+ { \
+ jclass prim_class = env_->FindClass("java/lang/" #boxed_type); \
+ jmethodID method = env_->GetStaticMethodID(prim_class, "valueOf", \
+ "(" #shorty ")Ljava/lang/" #boxed_type ";"); \
+ EXPECT_NE(nullptr, method); \
+ EXPECT_FALSE(env_->ExceptionCheck()); \
+ CheckJniAbortCatcher check_jni_abort_catcher; \
+ env_->CallStaticObjectMethod(prim_class, method, bad_value); \
+ check_jni_abort_catcher.Check("unexpected " #c_type " value: " #bad_value); \
+ }
+
+ DoCall(Boolean, Z, jboolean, 2);
+ DoCall(Byte, B, jbyte, 128);
+ DoCall(Byte, B, jbyte, -129);
+ DoCall(Short, S, jshort, 32768);
+ DoCall(Short, S, jshort, -32769);
+ DoCall(Character, C, jchar, 65536);
+ DoCall(Character, C, jchar, -1);
+#undef DoCall
+}
+
+TEST_F(JniInternalTest, CallJValueMethodBadPrimitive) {
+ // Check that bad primitive values, passed as jvalues, cause check
+ // JNI to abort. Unlike with var args, sizes less than an int should
+ // be truncated or sign extended and not cause an abort except for
+ // jbooleans that are passed as bytes.
+#define DoFailCall(boxed_type, shorty, c_type, bad_value) \
+ { \
+ jclass prim_class = env_->FindClass("java/lang/" #boxed_type); \
+ jmethodID method = env_->GetStaticMethodID(prim_class, "valueOf", \
+ "(" #shorty ")Ljava/lang/" #boxed_type ";"); \
+ EXPECT_NE(nullptr, method); \
+ EXPECT_FALSE(env_->ExceptionCheck()); \
+ CheckJniAbortCatcher check_jni_abort_catcher; \
+ jvalue jval; \
+ jval.i = bad_value; \
+ env_->CallStaticObjectMethodA(prim_class, method, &jval); \
+ check_jni_abort_catcher.Check("unexpected " #c_type " value: " #bad_value); \
+ }
+#define DoGoodCall(boxed_type, shorty, c_type, bad_value) \
+ { \
+ jclass prim_class = env_->FindClass("java/lang/" #boxed_type); \
+ jmethodID method = env_->GetStaticMethodID(prim_class, "valueOf", \
+ "(" #shorty ")Ljava/lang/" #boxed_type ";"); \
+ EXPECT_NE(nullptr, method); \
+ EXPECT_FALSE(env_->ExceptionCheck()); \
+ jvalue jval; \
+ jval.i = bad_value; \
+ env_->CallStaticObjectMethodA(prim_class, method, &jval); \
+ }
+
+ DoFailCall(Boolean, Z, jboolean, 2);
+ DoGoodCall(Byte, B, jbyte, 128);
+ DoGoodCall(Byte, B, jbyte, -129);
+ DoGoodCall(Short, S, jshort, 32768);
+ DoGoodCall(Short, S, jshort, -32769);
+ DoGoodCall(Character, C, jchar, 65536);
+ DoGoodCall(Character, C, jchar, -1);
+#undef DoCall
+}
+
TEST_F(JniInternalTest, GetStaticMethodID) {
jclass jlobject = env_->FindClass("java/lang/Object");
jclass jlnsme = env_->FindClass("java/lang/NoSuchMethodError");
diff --git a/runtime/reflection.cc b/runtime/reflection.cc
index 635a03a..068bc28 100644
--- a/runtime/reflection.cc
+++ b/runtime/reflection.cc
@@ -35,6 +35,7 @@
#include "well_known_classes.h"
namespace art {
+namespace {
using android::base::StringPrintf;
@@ -157,6 +158,7 @@
Append(args[args_offset].s);
break;
case 'I':
+ FALLTHROUGH_INTENDED;
case 'F':
Append(args[args_offset].i);
break;
@@ -164,6 +166,7 @@
Append(soa.Decode<mirror::Object>(args[args_offset].l));
break;
case 'D':
+ FALLTHROUGH_INTENDED;
case 'J':
AppendWide(args[args_offset].j);
break;
@@ -361,7 +364,7 @@
std::unique_ptr<uint32_t[]> large_arg_array_;
};
-static void CheckMethodArguments(JavaVMExt* vm, ArtMethod* m, uint32_t* args)
+void CheckMethodArguments(JavaVMExt* vm, ArtMethod* m, uint32_t* args)
REQUIRES_SHARED(Locks::mutator_lock_) {
const DexFile::TypeList* params = m->GetParameterTypeList();
if (params == nullptr) {
@@ -436,13 +439,13 @@
}
}
-static ArtMethod* FindVirtualMethod(ObjPtr<mirror::Object> receiver, ArtMethod* method)
+ArtMethod* FindVirtualMethod(ObjPtr<mirror::Object> receiver, ArtMethod* method)
REQUIRES_SHARED(Locks::mutator_lock_) {
return receiver->GetClass()->FindVirtualMethodForVirtualOrInterface(method, kRuntimePointerSize);
}
-static void InvokeWithArgArray(const ScopedObjectAccessAlreadyRunnable& soa,
+void InvokeWithArgArray(const ScopedObjectAccessAlreadyRunnable& soa,
ArtMethod* method, ArgArray* arg_array, JValue* result,
const char* shorty)
REQUIRES_SHARED(Locks::mutator_lock_) {
@@ -453,6 +456,8 @@
method->Invoke(soa.Self(), args, arg_array->GetNumBytes(), result, shorty);
}
+} // anonymous namespace
+
JValue InvokeWithVarArgs(const ScopedObjectAccessAlreadyRunnable& soa, jobject obj, jmethodID mid,
va_list args)
REQUIRES_SHARED(Locks::mutator_lock_) {