From a41a5f33e63d12df80bcd886c8eda3d497ef5eae Mon Sep 17 00:00:00 2001 From: Jason Date: Tue, 21 Jun 2022 11:22:04 +0100 Subject: [PATCH] feat(ndk): synchronize complex metadata values to the C layer, and cleanup old OPAQUE values when they are no longer reachable --- bugsnag-plugin-android-ndk/proguard-rules.pro | 3 ++ .../src/main/CMakeLists.txt | 1 + .../com/bugsnag/android/ndk/NativeBridge.kt | 18 ++++++-- .../com/bugsnag/android/ndk/OpaqueValue.kt | 45 +++++++++++++++++++ .../src/main/jni/bugsnag_ndk.c | 20 +++++++++ .../src/main/jni/event.c | 44 +++++++++++++++--- .../src/main/jni/event.h | 3 ++ .../src/main/jni/jni_cache.c | 4 ++ .../src/main/jni/jni_cache.h | 3 ++ .../src/main/jni/metadata.c | 9 ++++ .../src/main/jni/utils/memory.c | 31 +++++++++++++ .../src/main/jni/utils/memory.h | 18 ++++++++ ...figurationMetadataNativeCrashScenario.java | 28 ++++++++++++ features/full_tests/native_metadata.feature | 5 ++- features/smoke_tests/unhandled.feature | 3 +- 15 files changed, 224 insertions(+), 11 deletions(-) create mode 100644 bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/OpaqueValue.kt create mode 100644 bugsnag-plugin-android-ndk/src/main/jni/utils/memory.c create mode 100644 bugsnag-plugin-android-ndk/src/main/jni/utils/memory.h diff --git a/bugsnag-plugin-android-ndk/proguard-rules.pro b/bugsnag-plugin-android-ndk/proguard-rules.pro index 7020a10281..3fa50bcdb9 100644 --- a/bugsnag-plugin-android-ndk/proguard-rules.pro +++ b/bugsnag-plugin-android-ndk/proguard-rules.pro @@ -1,3 +1,6 @@ -keepattributes LineNumberTable,SourceFile +-keep class com.bugsnag.android.ndk.OpaqueValue { + java.lang.String getJson(); +} -keep class com.bugsnag.android.ndk.NativeBridge { *; } -keep class com.bugsnag.android.NdkPlugin { *; } diff --git a/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt b/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt index c3c7f84a84..37274eb046 100644 --- a/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt +++ b/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt @@ -24,6 +24,7 @@ add_library( # Specifies the name of the library. jni/utils/serializer.c jni/utils/string.c jni/utils/threads.c + jni/utils/memory.c jni/deps/parson/parson.c ) diff --git a/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/NativeBridge.kt b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/NativeBridge.kt index 0525f84238..cfff82b863 100644 --- a/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/NativeBridge.kt +++ b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/NativeBridge.kt @@ -63,6 +63,7 @@ class NativeBridge : StateObserver { external fun addMetadataString(tab: String, key: String, value: String) external fun addMetadataDouble(tab: String, key: String, value: Double) external fun addMetadataBoolean(tab: String, key: String, value: Boolean) + external fun addMetadataOpaque(tab: String, key: String, value: String) external fun addHandledEvent() external fun addUnhandledEvent() external fun clearMetadataTab(tab: String) @@ -98,7 +99,7 @@ class NativeBridge : StateObserver { makeSafe(event.message), makeSafe(event.type.toString()), makeSafe(event.timestamp), - event.metadata + makeSafeMetadata(event.metadata) ) NotifyHandled -> addHandledEvent() NotifyUnhandled -> addUnhandledEvent() @@ -122,7 +123,10 @@ class NativeBridge : StateObserver { updateUserName(makeSafe(event.user.name ?: "")) updateUserEmail(makeSafe(event.user.email ?: "")) } - is StateEvent.UpdateMemoryTrimEvent -> updateLowMemory(event.isLowMemory, event.memoryTrimLevelDescription) + is StateEvent.UpdateMemoryTrimEvent -> updateLowMemory( + event.isLowMemory, + event.memoryTrimLevelDescription + ) is StateEvent.AddFeatureFlag -> addFeatureFlag( makeSafe(event.name), event.variant?.let { makeSafe(it) } @@ -132,6 +136,13 @@ class NativeBridge : StateObserver { } } + private fun makeSafeMetadata(metadata: Map): Map { + if (metadata.isEmpty()) return metadata + return object : Map by metadata { + override fun get(key: String): Any? = OpaqueValue.makeSafe(metadata[key]) + } + } + private fun isInvalidMessage(msg: Any?): Boolean { if (msg == null || msg !is StateEvent) { return true @@ -190,10 +201,11 @@ class NativeBridge : StateObserver { private fun handleAddMetadata(arg: AddMetadata) { if (arg.key != null) { - when (val newValue = arg.value) { + when (val newValue = OpaqueValue.makeSafe(arg.value)) { is String -> addMetadataString(arg.section, arg.key!!, makeSafe(newValue)) is Boolean -> addMetadataBoolean(arg.section, arg.key!!, newValue) is Number -> addMetadataDouble(arg.section, arg.key!!, newValue.toDouble()) + is OpaqueValue -> addMetadataOpaque(arg.section, arg.key!!, newValue.json) else -> Unit } } diff --git a/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/OpaqueValue.kt b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/OpaqueValue.kt new file mode 100644 index 0000000000..779e5eb00f --- /dev/null +++ b/bugsnag-plugin-android-ndk/src/main/java/com/bugsnag/android/ndk/OpaqueValue.kt @@ -0,0 +1,45 @@ +package com.bugsnag.android.ndk + +import com.bugsnag.android.JsonStream +import java.io.StringWriter + +/** + * Marker class for values that are `BSG_METADATA_OPAQUE_VALUE` in the C layer + */ +internal class OpaqueValue(val json: String) { + companion object { + private const val MAX_NDK_STRING_LENGTH = 64 + private const val US_ASCII_MAX_CODEPOINT = 127 + + private fun isStringNDKSupported(value: String): Boolean { + // anything over 63 characters is definitely not supported + if (value.length >= MAX_NDK_STRING_LENGTH) return false + + // all chars are US-ASCII valid (0-127)? + if (value.all { ch: Char -> ch.toInt() <= US_ASCII_MAX_CODEPOINT }) { + // US-ASCII values shorter than 64 characters are supported directly + return true + } + + // easiest way to figure it out at this stage is to UTF-8 encode the string and check + // it's length as a byte-array + return value.toByteArray().size < MAX_NDK_STRING_LENGTH + } + + private fun encode(value: Any): String = + StringWriter().use { JsonStream(it).value(value, true) }.toString() + + /** + * Ensure that the given `value` is compatible with the Bugsnag C layer by ensuring it + * is both a compatible type and fits into the available space. This method can return + * any one of: `Boolean`, `Number`, `String`, `OpaqueValue` or `null`. + */ + fun makeSafe(value: Any?): Any? = when { + value is Boolean -> value + value is Number -> value + value is String && isStringNDKSupported(value) -> value + value is String || value is Map<*, *> || value is List<*> -> OpaqueValue(encode(value)) + else -> null + } + } +} 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 afdb7ab939..b0ea007d4c 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/bugsnag_ndk.c +++ b/bugsnag-plugin-android-ndk/src/main/jni/bugsnag_ndk.c @@ -667,6 +667,26 @@ Java_com_bugsnag_android_ndk_NativeBridge_addMetadataBoolean( bsg_safe_release_string_utf_chars(env, key_, key); } +JNIEXPORT void JNICALL +Java_com_bugsnag_android_ndk_NativeBridge_addMetadataOpaque( + JNIEnv *env, jobject _this, jstring tab_, jstring key_, jstring value_) { + if (bsg_global_env == NULL) { + return; + } + char *tab = (char *)bsg_safe_get_string_utf_chars(env, tab_); + char *key = (char *)bsg_safe_get_string_utf_chars(env, key_); + char *value = (char *)bsg_safe_get_string_utf_chars(env, value_); + if (tab != NULL && key != NULL) { + request_env_write_lock(); + bsg_add_metadata_value_opaque(&bsg_global_env->next_event.metadata, tab, + key, value); + release_env_write_lock(); + } + bsg_safe_release_string_utf_chars(env, tab_, tab); + bsg_safe_release_string_utf_chars(env, key_, key); + bsg_safe_release_string_utf_chars(env, value_, value); +} + JNIEXPORT void JNICALL Java_com_bugsnag_android_ndk_NativeBridge_clearMetadataTab(JNIEnv *env, jobject _this, diff --git a/bugsnag-plugin-android-ndk/src/main/jni/event.c b/bugsnag-plugin-android-ndk/src/main/jni/event.c index 5f68a5ee58..81c0c94c44 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/event.c +++ b/bugsnag-plugin-android-ndk/src/main/jni/event.c @@ -2,6 +2,7 @@ #include "utils/string.h" #include #include +#include /** * Compact the given metadata array by removing all duplicate entries and NONE @@ -27,12 +28,19 @@ static bool bsg_metadata_compact(bugsnag_metadata *const metadata) { const char *name = metadata->values[primaryIndex].name; for (int searchIndex = primaryIndex - 1; searchIndex >= 0; searchIndex--) { - if (strcmp(metadata->values[searchIndex].section, section) == 0 && - strcmp(metadata->values[searchIndex].name, name) == 0) { + bsg_metadata_value *value = &(metadata->values[searchIndex]); + if (strcmp(value->section, section) == 0 && + strcmp(value->name, name) == 0) { + + // ensure we free any OPAQUE values we're going to remove + if (value->type == BSG_METADATA_OPAQUE_VALUE) { + bsg_free(value->opaque_value); + value->opaque_value_size = 0; + } // this will be picked up by our primaryIndex search on a future // iteration - metadata->values[searchIndex].type = BSG_METADATA_NONE_VALUE; + value->type = BSG_METADATA_NONE_VALUE; } } } @@ -89,9 +97,16 @@ static bool bsg_event_clear_metadata(bugsnag_metadata *metadata, int clearedCount = 0; // remove *all* values for the given key, and then compact the entire dataset for (int i = 0; i < metadata->value_count; ++i) { - if (strcmp(metadata->values[i].section, section) == 0 && - strcmp(metadata->values[i].name, name) == 0) { - metadata->values[i].type = BSG_METADATA_NONE_VALUE; + bsg_metadata_value *value = &(metadata->values[i]); + if (strcmp(value->section, section) == 0 && + strcmp(value->name, name) == 0) { + + if (value->type == BSG_METADATA_OPAQUE_VALUE) { + bsg_free(value->opaque_value); + value->opaque_value_size = 0; + } + + value->type = BSG_METADATA_NONE_VALUE; clearedCount++; } } @@ -157,6 +172,23 @@ void bsg_add_metadata_value_bool(bugsnag_metadata *metadata, } } +void bsg_add_metadata_value_opaque(bugsnag_metadata *metadata, + const char *section, const char *name, + const char *json) { + int index = allocate_metadata_index(metadata, section, name); + if (index >= 0) { + char *duplicate = strdup(json); + + if (duplicate == NULL) { + return; + } + + metadata->values[index].opaque_value = duplicate; + metadata->values[index].type = BSG_METADATA_OPAQUE_VALUE; + metadata->values[index].opaque_value_size = bsg_strlen(json); + } +} + void bugsnag_event_add_metadata_double(void *event_ptr, const char *section, const char *name, double value) { bugsnag_event *event = (bugsnag_event *)event_ptr; diff --git a/bugsnag-plugin-android-ndk/src/main/jni/event.h b/bugsnag-plugin-android-ndk/src/main/jni/event.h index f7903503e5..8383b57793 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/event.h +++ b/bugsnag-plugin-android-ndk/src/main/jni/event.h @@ -268,6 +268,9 @@ void bsg_add_metadata_value_str(bugsnag_metadata *metadata, const char *section, void bsg_add_metadata_value_bool(bugsnag_metadata *metadata, const char *section, const char *name, bool value); +void bsg_add_metadata_value_opaque(bugsnag_metadata *metadata, + const char *section, const char *name, + const char *json); /********************************* * (end) NDK-SPECIFIC BITS diff --git a/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.c b/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.c index 99c7a26772..04966d79a8 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.c +++ b/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.c @@ -159,6 +159,10 @@ bool bsg_jni_cache_init(JNIEnv *env) { CACHE_CLASS(BreadcrumbType, "com/bugsnag/android/BreadcrumbType"); + CACHE_CLASS(OpaqueValue, "com/bugsnag/android/ndk/OpaqueValue"); + CACHE_METHOD(OpaqueValue, OpaqueValue_getJson, "getJson", + "()Ljava/lang/String;"); + pthread_key_create(&jni_cleanup_key, detach_java_env); bsg_jni_cache->initialized = true; diff --git a/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.h b/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.h index 410d7f7d60..1f121f0018 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.h +++ b/bugsnag-plugin-android-ndk/src/main/jni/jni_cache.h @@ -59,6 +59,9 @@ typedef struct { jclass Severity; jclass BreadcrumbType; + + jclass OpaqueValue; + jmethodID OpaqueValue_getJson; } bsg_jni_cache_t; extern bsg_jni_cache_t *const bsg_jni_cache; diff --git a/bugsnag-plugin-android-ndk/src/main/jni/metadata.c b/bugsnag-plugin-android-ndk/src/main/jni/metadata.c index 5894703753..1516d459d8 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/metadata.c +++ b/bugsnag-plugin-android-ndk/src/main/jni/metadata.c @@ -358,6 +358,15 @@ static void populate_metadata_value(JNIEnv *env, bugsnag_metadata *dst, if (value != NULL) { bsg_add_metadata_value_str(dst, section, name, value); } + } else if (bsg_safe_is_instance_of(env, _value, bsg_jni_cache->OpaqueValue)) { + jstring _json = bsg_safe_call_object_method( + env, _value, bsg_jni_cache->OpaqueValue_getJson); + const char *json = bsg_safe_get_string_utf_chars(env, _json); + + if (json != NULL) { + bsg_add_metadata_value_opaque(dst, section, name, json); + bsg_safe_release_string_utf_chars(env, _json, json); + } } } diff --git a/bugsnag-plugin-android-ndk/src/main/jni/utils/memory.c b/bugsnag-plugin-android-ndk/src/main/jni/utils/memory.c new file mode 100644 index 0000000000..64d23b02f4 --- /dev/null +++ b/bugsnag-plugin-android-ndk/src/main/jni/utils/memory.c @@ -0,0 +1,31 @@ +#include + +#include "bugsnag_ndk.h" +#include "memory.h" + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * Global shared context for Bugsnag reports + */ +static bsg_environment *bsg_global_env; + +void bsg_free(void *ptr) { + /* + * free is only "active" when there are no signal handlers active, if there is + * a crash in progress the memory cannot be safely released within the process + * (free is not async safe), and the heap will be released along with the rest + * of the process when the crash is over. + */ + if (bsg_global_env->handling_crash) { + return; + } + + free(ptr); +} + +#ifdef __cplusplus +} +#endif diff --git a/bugsnag-plugin-android-ndk/src/main/jni/utils/memory.h b/bugsnag-plugin-android-ndk/src/main/jni/utils/memory.h new file mode 100644 index 0000000000..ebe9b5c3a2 --- /dev/null +++ b/bugsnag-plugin-android-ndk/src/main/jni/utils/memory.h @@ -0,0 +1,18 @@ +#ifndef BUGSNAG_ANDROID_MEMORY_H +#define BUGSNAG_ANDROID_MEMORY_H + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * Crash-safe 'free' that won't cause deadlocks if a signal handler is active. + * @param ptr + */ +void bsg_free(void *ptr); + +#ifdef __cplusplus +} +#endif + +#endif // BUGSNAG_ANDROID_MEMORY_H diff --git a/features/fixtures/mazerunner/cxx-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/CXXConfigurationMetadataNativeCrashScenario.java b/features/fixtures/mazerunner/cxx-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/CXXConfigurationMetadataNativeCrashScenario.java index 33ed65a491..81653b2c19 100644 --- a/features/fixtures/mazerunner/cxx-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/CXXConfigurationMetadataNativeCrashScenario.java +++ b/features/fixtures/mazerunner/cxx-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/CXXConfigurationMetadataNativeCrashScenario.java @@ -1,5 +1,7 @@ package com.bugsnag.android.mazerunner.scenarios; +import static java.lang.Math.PI; + import com.bugsnag.android.Configuration; import android.content.Context; @@ -7,6 +9,10 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; + public class CXXConfigurationMetadataNativeCrashScenario extends Scenario { static { @@ -26,6 +32,28 @@ public CXXConfigurationMetadataNativeCrashScenario(@NonNull Configuration config config.addMetadata("fruit", "apple", "gala"); config.addMetadata("fruit", "counters", 47); config.addMetadata("fruit", "ripe", true); + + config.addMetadata("complex", "message", + "That might've been one of the shortest assignments in the history of " + + "Starfleet. The Enterprise computer system is controlled by three " + + "primary main processor cores, cross-linked with a redundant " + + "melacortz ramistat, fourteen kiloquad interface modules."); + + Map map = new HashMap<>(); + map.put("location", "you are here"); + map.put("inventory", Arrays.asList( + "lots of string", + PI, + true + )); + + config.addMetadata("complex", "maps", map); + config.addMetadata("complex", "list", Arrays.asList( + "summer", + "winter", + "spring", + "autumn" + )); } } diff --git a/features/full_tests/native_metadata.feature b/features/full_tests/native_metadata.feature index 06d220eae4..cbec6d92d9 100644 --- a/features/full_tests/native_metadata.feature +++ b/features/full_tests/native_metadata.feature @@ -16,9 +16,12 @@ Feature: Native Metadata API And the event "metaData.fruit.apple" equals "gala" And the event "metaData.fruit.ripe" is true And the event "metaData.fruit.counters" equals 47 + And the event "metaData.complex.message" is null + And the event "metaData.complex.maps.location" is null + And the event "metaData.complex.maps.inventory" is null + And the event "metaData.complex.list" is null And the event "unhandled" is true -#TODO up to here Scenario: Remove MetaData from the NDK layer When I run "CXXRemoveDataScenario" And I wait to receive 2 errors diff --git a/features/smoke_tests/unhandled.feature b/features/smoke_tests/unhandled.feature index 8ee2a817de..df8bca8348 100644 --- a/features/smoke_tests/unhandled.feature +++ b/features/smoke_tests/unhandled.feature @@ -179,7 +179,8 @@ Feature: Unhandled smoke tests And the event "context" equals "Some custom context" # Metadata - And the event "metaData.Riker Ipsum.examples" equals "I'll be sure to note that in my log. You enjoyed that. They wer" + # Riker Ipsum is null until PLAT-8581 (store / load opaque metadata) is complete + And the event "metaData.Riker Ipsum.examples" is null And the event "metaData.fruit.apple" equals "gala" And the event "metaData.fruit.ripe" is true And the event "metaData.fruit.counters" equals 47