From b5d3c589b31556fc776041e588cb79649dce0500 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 22 Jan 2021 15:57:14 +0000 Subject: [PATCH] fix: check JNI calls for pending exceptions and no-op --- CHANGELOG.md | 5 + .../src/main/CMakeLists.txt | 1 + .../src/main/jni/bugsnag.c | 90 +++++-- .../src/main/jni/bugsnag_ndk.c | 16 +- .../src/main/jni/metadata.c | 219 +++++++++++++++--- .../src/main/jni/safejni.c | 27 +++ .../src/main/jni/safejni.h | 50 ++++ 7 files changed, 360 insertions(+), 48 deletions(-) create mode 100644 bugsnag-plugin-android-ndk/src/main/jni/safejni.c create mode 100644 bugsnag-plugin-android-ndk/src/main/jni/safejni.h diff --git a/CHANGELOG.md b/CHANGELOG.md index e90da84b90..b48ea0cd48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## TBD + +* Check internal JNI calls for pending exceptions and no-op + [#1088](https://github.com/bugsnag/bugsnag-android/pull/1088) + ## 5.5.1 (2021-01-21) * Alter ANR SIGQUIT handler to stop interfering with Google's ANR reporting, and to avoid unsafe JNI calls from within a signal handler diff --git a/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt b/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt index beb07a3ae9..fd72ab9efe 100644 --- a/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt +++ b/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt @@ -9,6 +9,7 @@ add_library( # Specifies the name of the library. jni/bugsnag_ndk.c jni/bugsnag.c jni/metadata.c + jni/safejni.c jni/event.c jni/handlers/signal_handler.c jni/handlers/cpp_handler.cpp diff --git a/bugsnag-plugin-android-ndk/src/main/jni/bugsnag.c b/bugsnag-plugin-android-ndk/src/main/jni/bugsnag.c index d7d1e68681..64a1cd234a 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/bugsnag.c +++ b/bugsnag-plugin-android-ndk/src/main/jni/bugsnag.c @@ -4,6 +4,7 @@ #include "bugsnag_ndk.h" #include "event.h" #include "metadata.h" +#include "safejni.h" #include "utils/stack_unwinder.h" #include "utils/string.h" #include @@ -86,20 +87,45 @@ void bugsnag_notify_env(JNIEnv *env, char *name, char *message, ssize_t frame_count = bsg_unwind_stack(bsg_configured_unwind_style(), stacktrace, NULL, NULL); + // lookup com/bugsnag/android/NativeInterface jclass interface_class = - (*env)->FindClass(env, "com/bugsnag/android/NativeInterface"); - jmethodID notify_method = (*env)->GetStaticMethodID( + bsg_safe_find_class(env, "com/bugsnag/android/NativeInterface"); + if (interface_class == NULL) { + return; + } + + // lookup NativeInterface.notify() + jmethodID notify_method = bsg_safe_get_static_method_id( env, interface_class, "notify", "([B[BLcom/bugsnag/android/Severity;[Ljava/lang/StackTraceElement;)V"); - jclass trace_class = (*env)->FindClass(env, "java/lang/StackTraceElement"); + if (notify_method == NULL) { + return; + } + + // lookup java/lang/StackTraceElement + jclass trace_class = bsg_safe_find_class(env, "java/lang/StackTraceElement"); + if (trace_class == NULL) { + return; + } + + // lookup com/bugsnag/android/Severity jclass severity_class = - (*env)->FindClass(env, "com/bugsnag/android/Severity"); - jmethodID trace_constructor = (*env)->GetMethodID( + bsg_safe_find_class(env, "com/bugsnag/android/Severity"); + if (severity_class == NULL) { + return; + } + + // lookup StackTraceElement constructor + jmethodID trace_constructor = bsg_safe_get_method_id( env, trace_class, "", "(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;I)V"); + if (trace_constructor == NULL) { + return; + } + jobjectArray trace = (*env)->NewObjectArray( env, (jsize)frame_count, - (*env)->FindClass(env, "java/lang/StackTraceElement"), NULL); + bsg_safe_find_class(env, "java/lang/StackTraceElement"), NULL); for (int i = 0; i < frame_count; i++) { bugsnag_stackframe frame = stacktrace[i]; @@ -150,10 +176,19 @@ void bugsnag_notify_env(JNIEnv *env, char *name, char *message, } void bugsnag_set_binary_arch(JNIEnv *env) { + // lookup com/bugsnag/android/NativeInterface jclass interface_class = - (*env)->FindClass(env, "com/bugsnag/android/NativeInterface"); - jmethodID set_arch_method = (*env)->GetStaticMethodID( + bsg_safe_find_class(env, "com/bugsnag/android/NativeInterface"); + if (interface_class == NULL) { + return; + } + + // lookup NativeInterface.setBinaryArch() + jmethodID set_arch_method = bsg_safe_get_static_method_id( env, interface_class, "setBinaryArch", "(Ljava/lang/String;)V"); + if (set_arch_method == NULL) { + return; + } jstring arch = (*env)->NewStringUTF(env, bsg_binary_arch()); (*env)->CallStaticVoidMethod(env, interface_class, set_arch_method, arch); @@ -162,10 +197,19 @@ void bugsnag_set_binary_arch(JNIEnv *env) { } void bugsnag_set_user_env(JNIEnv *env, char *id, char *email, char *name) { + // lookup com/bugsnag/android/NativeInterface jclass interface_class = - (*env)->FindClass(env, "com/bugsnag/android/NativeInterface"); - jmethodID set_user_method = - (*env)->GetStaticMethodID(env, interface_class, "setUser", "([B[B[B)V"); + bsg_safe_find_class(env, "com/bugsnag/android/NativeInterface"); + if (interface_class == NULL) { + return; + } + + // lookup NativeInterface.setUser() + jmethodID set_user_method = bsg_safe_get_static_method_id( + env, interface_class, "setUser", "([B[B[B)V"); + if (set_user_method == NULL) { + return; + } jbyteArray jid = bsg_byte_ary_from_string(env, id); jbyteArray jemail = bsg_byte_ary_from_string(env, email); @@ -208,13 +252,27 @@ jfieldID bsg_parse_jcrumb_type(JNIEnv *env, bugsnag_breadcrumb_type type, void bugsnag_leave_breadcrumb_env(JNIEnv *env, char *message, bugsnag_breadcrumb_type type) { + // lookup com/bugsnag/android/NativeInterface jclass interface_class = - (*env)->FindClass(env, "com/bugsnag/android/NativeInterface"); - jmethodID leave_breadcrumb_method = - (*env)->GetStaticMethodID(env, interface_class, "leaveBreadcrumb", - "([BLcom/bugsnag/android/BreadcrumbType;)V"); + bsg_safe_find_class(env, "com/bugsnag/android/NativeInterface"); + if (interface_class == NULL) { + return; + } + + // lookup NativeInterface.leaveBreadcrumb() + jmethodID leave_breadcrumb_method = bsg_safe_get_static_method_id( + env, interface_class, "leaveBreadcrumb", + "([BLcom/bugsnag/android/BreadcrumbType;)V"); + if (interface_class == NULL) { + return; + } + + // lookup com/bugsnag/android/BreadcrumbType jclass type_class = - (*env)->FindClass(env, "com/bugsnag/android/BreadcrumbType"); + bsg_safe_find_class(env, "com/bugsnag/android/BreadcrumbType"); + if (interface_class == NULL) { + return; + } jobject jtype = (*env)->GetStaticObjectField( env, type_class, bsg_parse_jcrumb_type(env, type, type_class)); diff --git a/bugsnag-plugin-android-ndk/src/main/jni/bugsnag_ndk.c b/bugsnag-plugin-android-ndk/src/main/jni/bugsnag_ndk.c index 8d6a6f7b40..e2b837ef70 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/bugsnag_ndk.c +++ b/bugsnag-plugin-android-ndk/src/main/jni/bugsnag_ndk.c @@ -10,6 +10,7 @@ #include "handlers/cpp_handler.h" #include "handlers/signal_handler.h" #include "metadata.h" +#include "safejni.h" #include "utils/serializer.h" #include "utils/string.h" @@ -151,10 +152,21 @@ Java_com_bugsnag_android_ndk_NativeBridge_deliverReportAtPath( if (event != NULL) { char *payload = bsg_serialize_event_to_json_string(event); if (payload != NULL) { + + // lookup com/bugsnag/android/NativeInterface jclass interface_class = - (*env)->FindClass(env, "com/bugsnag/android/NativeInterface"); - jmethodID jdeliver_method = (*env)->GetStaticMethodID( + bsg_safe_find_class(env, "com/bugsnag/android/NativeInterface"); + if (interface_class == NULL) { + return; + } + + // lookup NativeInterface.deliverReport() + jmethodID jdeliver_method = bsg_safe_get_static_method_id( env, interface_class, "deliverReport", "([B[BLjava/lang/String;)V"); + if (jdeliver_method == NULL) { + return; + } + size_t payload_length = bsg_strlen(payload); jbyteArray jpayload = (*env)->NewByteArray(env, payload_length); (*env)->SetByteArrayRegion(env, jpayload, 0, payload_length, diff --git a/bugsnag-plugin-android-ndk/src/main/jni/metadata.c b/bugsnag-plugin-android-ndk/src/main/jni/metadata.c index 10f005f219..29cf85b01a 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/metadata.c +++ b/bugsnag-plugin-android-ndk/src/main/jni/metadata.c @@ -1,4 +1,5 @@ #include "metadata.h" +#include "safejni.h" #include "utils/string.h" #include #include @@ -40,55 +41,203 @@ void bsg_populate_metadata_value(JNIEnv *env, bugsnag_metadata *dst, bsg_jni_cache *jni_cache, char *section, char *name, jobject _value); +/** + * Creates a cache of JNI methods/classes that are commonly used. + * + * Class and method objects can be kept safely since they aren't moved or + * removed from the JVM - care should be taken not to load objects as local + * references here. + */ bsg_jni_cache *bsg_populate_jni_cache(JNIEnv *env) { bsg_jni_cache *jni_cache = malloc(sizeof(bsg_jni_cache)); - jni_cache->integer = (*env)->FindClass(env, "java/lang/Integer"); - jni_cache->boolean = (*env)->FindClass(env, "java/lang/Boolean"); - jni_cache->long_class = (*env)->FindClass(env, "java/lang/Long"); - jni_cache->float_class = (*env)->FindClass(env, "java/lang/Float"); - jni_cache->number = (*env)->FindClass(env, "java/lang/Number"); - jni_cache->string = (*env)->FindClass(env, "java/lang/String"); + + // lookup java/lang/Integer + jni_cache->integer = bsg_safe_find_class(env, "java/lang/Integer"); + if (jni_cache->integer == NULL) { + return NULL; + } + + // lookup java/lang/Boolean + jni_cache->boolean = bsg_safe_find_class(env, "java/lang/Boolean"); + if (jni_cache->boolean == NULL) { + return NULL; + } + + // lookup java/lang/Long + jni_cache->long_class = bsg_safe_find_class(env, "java/lang/Long"); + if (jni_cache->long_class == NULL) { + return NULL; + } + + // lookup java/lang/Float + jni_cache->float_class = bsg_safe_find_class(env, "java/lang/Float"); + if (jni_cache->float_class == NULL) { + return NULL; + } + + // lookup java/lang/Number + jni_cache->number = bsg_safe_find_class(env, "java/lang/Number"); + if (jni_cache->number == NULL) { + return NULL; + } + + // lookup java/lang/String + jni_cache->string = bsg_safe_find_class(env, "java/lang/String"); + if (jni_cache->string == NULL) { + return NULL; + } + + // lookup Integer.intValue() jni_cache->integer_int_value = - (*env)->GetMethodID(env, jni_cache->integer, "intValue", "()I"); + bsg_safe_get_method_id(env, jni_cache->integer, "intValue", "()I"); + if (jni_cache->integer_int_value == NULL) { + return NULL; + } + + // lookup Integer.floatValue() jni_cache->float_float_value = - (*env)->GetMethodID(env, jni_cache->float_class, "floatValue", "()F"); + bsg_safe_get_method_id(env, jni_cache->float_class, "floatValue", "()F"); + if (jni_cache->float_float_value == NULL) { + return NULL; + } + + // lookup Double.doubleValue() jni_cache->number_double_value = - (*env)->GetMethodID(env, jni_cache->number, "doubleValue", "()D"); + bsg_safe_get_method_id(env, jni_cache->number, "doubleValue", "()D"); + if (jni_cache->number_double_value == NULL) { + return NULL; + } + + // lookup Long.longValue() jni_cache->long_long_value = - (*env)->GetMethodID(env, jni_cache->integer, "longValue", "()J"); + bsg_safe_get_method_id(env, jni_cache->integer, "longValue", "()J"); + if (jni_cache->long_long_value == NULL) { + return NULL; + } + + // lookup Boolean.booleanValue() jni_cache->boolean_bool_value = - (*env)->GetMethodID(env, jni_cache->boolean, "booleanValue", "()Z"); - jni_cache->arraylist = (*env)->FindClass(env, "java/util/ArrayList"); - jni_cache->arraylist_init_with_obj = (*env)->GetMethodID( + bsg_safe_get_method_id(env, jni_cache->boolean, "booleanValue", "()Z"); + if (jni_cache->boolean_bool_value == NULL) { + return NULL; + } + + // lookup java/util/ArrayList + jni_cache->arraylist = bsg_safe_find_class(env, "java/util/ArrayList"); + if (jni_cache->arraylist == NULL) { + return NULL; + } + + // lookup ArrayList constructor + jni_cache->arraylist_init_with_obj = bsg_safe_get_method_id( env, jni_cache->arraylist, "", "(Ljava/util/Collection;)V"); - jni_cache->arraylist_get = (*env)->GetMethodID( + if (jni_cache->arraylist_init_with_obj == NULL) { + return NULL; + } + + // lookup ArrayList.get() + jni_cache->arraylist_get = bsg_safe_get_method_id( env, jni_cache->arraylist, "get", "(I)Ljava/lang/Object;"); - jni_cache->hash_map = (*env)->FindClass(env, "java/util/HashMap"); - jni_cache->map = (*env)->FindClass(env, "java/util/Map"); - jni_cache->hash_map_key_set = (*env)->GetMethodID( + if (jni_cache->arraylist_get == NULL) { + return NULL; + } + + // lookup java/util/HashMap + jni_cache->hash_map = bsg_safe_find_class(env, "java/util/HashMap"); + if (jni_cache->hash_map == NULL) { + return NULL; + } + + // lookup java/util/Map + jni_cache->map = bsg_safe_find_class(env, "java/util/Map"); + if (jni_cache->map == NULL) { + return NULL; + } + + // lookup java/util/Set + jni_cache->hash_map_key_set = bsg_safe_get_method_id( env, jni_cache->hash_map, "keySet", "()Ljava/util/Set;"); + if (jni_cache->hash_map_key_set == NULL) { + return NULL; + } + + // lookup HashMap.size() jni_cache->hash_map_size = - (*env)->GetMethodID(env, jni_cache->hash_map, "size", "()I"); + bsg_safe_get_method_id(env, jni_cache->hash_map, "size", "()I"); + if (jni_cache->hash_map_size == NULL) { + return NULL; + } + + // lookup HashMap.get() jni_cache->hash_map_get = - (*env)->GetMethodID(env, jni_cache->hash_map, "get", - "(Ljava/lang/Object;)Ljava/lang/Object;"); - jni_cache->map_key_set = - (*env)->GetMethodID(env, jni_cache->map, "keySet", "()Ljava/util/Set;"); - jni_cache->map_size = (*env)->GetMethodID(env, jni_cache->map, "size", "()I"); - jni_cache->map_get = (*env)->GetMethodID( + bsg_safe_get_method_id(env, jni_cache->hash_map, "get", + "(Ljava/lang/Object;)Ljava/lang/Object;"); + if (jni_cache->hash_map_get == NULL) { + return NULL; + } + + // lookup Map.keySet() + jni_cache->map_key_set = bsg_safe_get_method_id(env, jni_cache->map, "keySet", + "()Ljava/util/Set;"); + if (jni_cache->map_key_set == NULL) { + return NULL; + } + + // lookup Map.size() + jni_cache->map_size = + bsg_safe_get_method_id(env, jni_cache->map, "size", "()I"); + if (jni_cache->map_size == NULL) { + return NULL; + } + + // lookup Map.get() + jni_cache->map_get = bsg_safe_get_method_id( env, jni_cache->map, "get", "(Ljava/lang/Object;)Ljava/lang/Object;"); + if (jni_cache->map_get == NULL) { + return NULL; + } + + // lookup com/bugsnag/android/NativeInterface jni_cache->native_interface = - (*env)->FindClass(env, "com/bugsnag/android/NativeInterface"); - jni_cache->get_app_data = (*env)->GetStaticMethodID( + bsg_safe_find_class(env, "com/bugsnag/android/NativeInterface"); + if (jni_cache->native_interface == NULL) { + return NULL; + } + + // lookup NativeInterface.getApp() + jni_cache->get_app_data = bsg_safe_get_static_method_id( env, jni_cache->native_interface, "getApp", "()Ljava/util/Map;"); - jni_cache->get_device_data = (*env)->GetStaticMethodID( + if (jni_cache->get_app_data == NULL) { + return NULL; + } + + // lookup NativeInterface.getDevice() + jni_cache->get_device_data = bsg_safe_get_static_method_id( env, jni_cache->native_interface, "getDevice", "()Ljava/util/Map;"); - jni_cache->get_user_data = (*env)->GetStaticMethodID( + if (jni_cache->get_device_data == NULL) { + return NULL; + } + + // lookup NativeInterface.getUser() + jni_cache->get_user_data = bsg_safe_get_static_method_id( env, jni_cache->native_interface, "getUser", "()Ljava/util/Map;"); - jni_cache->get_metadata = (*env)->GetStaticMethodID( + if (jni_cache->get_user_data == NULL) { + return NULL; + } + + // lookup NativeInterface.getMetadata() + jni_cache->get_metadata = bsg_safe_get_static_method_id( env, jni_cache->native_interface, "getMetadata", "()Ljava/util/Map;"); - jni_cache->get_context = (*env)->GetStaticMethodID( + if (jni_cache->get_metadata == NULL) { + return NULL; + } + + // lookup NativeInterface.getContext() + jni_cache->get_context = bsg_safe_get_static_method_id( env, jni_cache->native_interface, "getContext", "()Ljava/lang/String;"); + if (jni_cache->get_context == NULL) { + return NULL; + } return jni_cache; } @@ -186,6 +335,10 @@ void bsg_populate_crumb_metadata(JNIEnv *env, bugsnag_breadcrumb *crumb, return; } bsg_jni_cache *jni_cache = bsg_populate_jni_cache(env); + if (jni_cache == NULL) { + return; + } + int map_size = (int)(*env)->CallIntMethod(env, metadata, jni_cache->map_size); jobject keyset = (*env)->CallObjectMethod(env, metadata, jni_cache->map_key_set); @@ -381,6 +534,9 @@ void bsg_populate_context(JNIEnv *env, bsg_jni_cache *jni_cache, void bsg_populate_event(JNIEnv *env, bugsnag_event *event) { bsg_jni_cache *jni_cache = bsg_populate_jni_cache(env); + if (jni_cache == NULL) { + return; + } bsg_populate_context(env, jni_cache, event); bsg_populate_app_data(env, jni_cache, event); bsg_populate_device_data(env, jni_cache, event); @@ -409,6 +565,9 @@ void bsg_populate_metadata_value(JNIEnv *env, bugsnag_metadata *dst, void bsg_populate_metadata(JNIEnv *env, bugsnag_metadata *dst, jobject metadata) { bsg_jni_cache *jni_cache = bsg_populate_jni_cache(env); + if (jni_cache == NULL) { + return; + } if (metadata == NULL) { metadata = (*env)->CallStaticObjectMethod(env, jni_cache->native_interface, jni_cache->get_metadata); diff --git a/bugsnag-plugin-android-ndk/src/main/jni/safejni.c b/bugsnag-plugin-android-ndk/src/main/jni/safejni.c new file mode 100644 index 0000000000..7d13d93c43 --- /dev/null +++ b/bugsnag-plugin-android-ndk/src/main/jni/safejni.c @@ -0,0 +1,27 @@ +#include "safejni.h" + +jclass bsg_safe_find_class(JNIEnv *env, const char *clz_name) { + jclass clz = (*env)->FindClass(env, clz_name); + if ((*env)->ExceptionCheck(env)) { + (*env)->ExceptionClear(env); + } + return clz; +} + +jmethodID bsg_safe_get_method_id(JNIEnv *env, jclass clz, const char *name, + const char *sig) { + jmethodID methodId = (*env)->GetMethodID(env, clz, name, sig); + if ((*env)->ExceptionCheck(env)) { + (*env)->ExceptionClear(env); + } + return methodId; +} + +jmethodID bsg_safe_get_static_method_id(JNIEnv *env, jclass clz, + const char *name, const char *sig) { + jmethodID methodId = (*env)->GetStaticMethodID(env, clz, name, sig); + if ((*env)->ExceptionCheck(env)) { + (*env)->ExceptionClear(env); + } + return methodId; +} diff --git a/bugsnag-plugin-android-ndk/src/main/jni/safejni.h b/bugsnag-plugin-android-ndk/src/main/jni/safejni.h new file mode 100644 index 0000000000..9ff654e0ab --- /dev/null +++ b/bugsnag-plugin-android-ndk/src/main/jni/safejni.h @@ -0,0 +1,50 @@ +#ifndef BUGSNAG_SAFEJNI_H +#define BUGSNAG_SAFEJNI_H + +#include + +/** + * A safe wrapper for the JNI's FindClass. This method checks if an exception is + * pending and if so clears it so that execution can continue. + * + * This method should be preferred to using the JNI methods directly. It is the + * responsibility of the caller to check whether the return value is NULL and + * handle this by no-oping. + * + * See + * https://docs.oracle.com/en/java/javase/11/docs/specs/jni/functions.html#findclass + * @return the class, or NULL if the class could not be found. + */ +jclass bsg_safe_find_class(JNIEnv *env, const char *clz_name); + +/** + * A safe wrapper for the JNI's GetMethodID. This method checks if an exception + * is pending and if so clears it so that execution can continue. + * + * This method should be preferred to using the JNI methods directly. It is the + * responsibility of the caller to check whether the return value is NULL and + * handle this by no-oping. + * + * See + * https://docs.oracle.com/en/java/javase/11/docs/specs/jni/functions.html#getmethodid + * @return the method ID, or NULL if the method could not be found. + */ +jmethodID bsg_safe_get_method_id(JNIEnv *env, jclass clz, const char *name, + const char *sig); + +/** + * A safe wrapper for the JNI's GetStaticMethodID. This method checks if an + * exception is pending and if so clears it so that execution can continue. + * + * This method should be preferred to using the JNI methods directly. It is the + * responsibility of the caller to check whether the return value is NULL and + * handle this by no-oping. + * + * See + * https://docs.oracle.com/en/java/javase/11/docs/specs/jni/functions.html#getstaticmethodid + * @return the method ID, or NULL if the method could not be found. + */ +jmethodID bsg_safe_get_static_method_id(JNIEnv *env, jclass clz, + const char *name, const char *sig); + +#endif