Merge pull request #1708 from bugsnag/PLAT-8582/encode-opaque-metadata

Encode opaque metadata

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 { *; }

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
              )
 

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<String, Any?>): Map<String, Any?> {
+        if (metadata.isEmpty()) return metadata
+        return object : Map<String, Any?> 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
             }
         }

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
+        }
+    }
+}

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,

bugsnag-plugin-android-ndk/src/main/jni/event.c

@@ -2,6 +2,7 @@
 #include "utils/string.h"
 #include <string.h>
 #include <utils/logger.h>
+#include <utils/memory.h>
 
 /**
  * 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;

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

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;

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;

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);
+    }
   }
 }
 

bugsnag-plugin-android-ndk/src/main/jni/utils/memory.c

@@ -0,0 +1,31 @@
+#include <malloc.h>
+
+#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

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

features/fixtures/mazerunner/cxx-scenarios/src/main/java/com/bugsnag/android/mazerunner/scenarios/CXXConfigurationMetadataNativeCrashScenario.java

@@ -1,12 +1,18 @@
 package com.bugsnag.android.mazerunner.scenarios;
 
+import static java.lang.Math.PI;
+
 import com.bugsnag.android.Configuration;
 
 import android.content.Context;
 
 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<String, Object> 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"
+            ));
         }
     }
 

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

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