Skip to content

Commit

Permalink
Merge pull request #1801 from bugsnag/PLAT-9577/fix-corrupt-feature-f…
Browse files Browse the repository at this point in the history
…lags

Fix rare cases of corrupt feature flags in NDK reports
  • Loading branch information
lemnik committed Feb 7, 2023
2 parents 4c840fd + ef6d5a4 commit d28e909
Show file tree
Hide file tree
Showing 14 changed files with 833 additions and 28 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

* Fixed a rare race-condition in libunwindstack where reading memory-maps could buffer-overrun
[#1798](https://github.com/bugsnag/bugsnag-android/pull/1798)
* Fixed an extremely rare NDK race-condition where feature flags in native crashes would be corrupted
[#1801](https://github.com/bugsnag/bugsnag-android/pull/1801)

## 5.28.3 (2022-11-16)

Expand Down
1 change: 1 addition & 0 deletions bugsnag-plugin-android-ndk/detekt-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<ID>ComplexMethod:NativeBridge.kt$NativeBridge$override fun onStateChange(event: StateEvent)</ID>
<ID>LongMethod:EventMigrationV10Tests.kt$EventMigrationV10Tests$@Test fun testMigrateEventToLatest()</ID>
<ID>LongMethod:EventMigrationV11Tests.kt$EventMigrationV11Tests$@Test fun testMigrateEventToLatest()</ID>
<ID>LongMethod:EventMigrationV12Tests.kt$EventMigrationV12Tests$@Test fun testMigrateEventToLatest()</ID>
<ID>LongMethod:EventMigrationV4Tests.kt$EventMigrationV4Tests$@Test fun testMigrateEventToLatest()</ID>
<ID>LongMethod:EventMigrationV5Tests.kt$EventMigrationV5Tests$@Test fun testMigrateEventToLatest()</ID>
<ID>LongMethod:EventMigrationV6Tests.kt$EventMigrationV6Tests$@Test fun testMigrateEventToLatest()</ID>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
package com.bugsnag.android.ndk.migrations

import org.junit.Assert.assertEquals
import org.junit.Assert.assertNotEquals
import org.junit.Assert.fail
import org.junit.Test

/** Migration v12 added telemetry data */
class EventMigrationV12Tests : EventMigrationTest() {

@Test
/** check notifier and api key, since they aren't included in event JSON */
fun testMigrationPayloadInfo() {
val infoFile = createTempFile()

val info = migratePayloadInfo(infoFile.absolutePath)

assertEquals(
mapOf(
"apiKey" to "5d1e5fbd39a74caa1200142706a90b20",
"notifierName" to "Test Library",
"notifierURL" to "https://example.com/test-lib",
"notifierVersion" to "2.0.11"
),
parseJSON(info)
)
}

@Test
fun testMigrateEventToLatest() {
val eventFile = createTempFile()

migrateEvent(eventFile.absolutePath)
assertNotEquals(0, eventFile.length())

val output = parseJSON(eventFile)

assertEquals(
"00000000000m0r3.61ee9e6e099d3dd7448f740d395768da6b2df55d5.m4g1c",
output["context"]
)
assertEquals(
"a1d34088a096987361ee9e6e099d3dd7448f740d395768da6b2df55d5160f33",
output["groupingHash"]
)
assertEquals("info", output["severity"])

// app
assertEquals(
mapOf(
"binaryArch" to "mips",
"buildUUID" to "1234-9876-adfe",
"duration" to 81395165021L,
"durationInForeground" to 81395165010L,
"id" to "com.example.PhotoSnapPlus",
"inForeground" to true,
"isLaunching" to true,
"releaseStage" to "リリース",
"type" to "red",
"version" to "2.0.52",
"versionCode" to 8139512718L
),
output["app"]
)

// breadcrumbs
val crumbs = output["breadcrumbs"]
if (crumbs is List<Any?>) {
assertEquals(50, crumbs.size)
crumbs.forEachIndexed { index, crumb ->
assertEquals(
mapOf(
"type" to "state",
"name" to "mission $index",
"timestamp" to "2021-12-08T19:43:50.014Z",
"metaData" to mapOf(
"message" to "Now we know what they mean by 'advanced' tactical training."
)
),
crumb
)
}
} else {
fail("breadcrumbs is not a list of crumb objects?!")
}

// device
assertEquals(
mapOf(
"cpuAbi" to listOf("mipsx"),
"id" to "ffffa",
"locale" to "en_AU#Melbun",
"jailbroken" to true,
"manufacturer" to "HI-TEC™",
"model" to "🍨",
"orientation" to "sideup",
"osName" to "BOX BOX",
"osVersion" to "98.7",
"runtimeVersions" to mapOf(
"osBuild" to "beta1-2",
"androidApiLevel" to "32"
),
"time" to "2021-12-08T19:43:50Z",
"totalMemory" to 3839512576L
),
output["device"]
)

// feature flags
assertEquals(
listOf(
mapOf(
"featureFlag" to "bluebutton",
"variant" to "on"
),
mapOf(
"featureFlag" to "redbutton",
"variant" to "off"
),
mapOf("featureFlag" to "nobutton"),
mapOf(
"featureFlag" to "switch",
"variant" to "left"
)
),
output["featureFlags"]
)

// exceptions
assertEquals(
listOf(
mapOf(
"errorClass" to "SIGBUS",
"message" to "POSIX is serious about oncoming traffic",
"type" to "c",
"stacktrace" to listOf(
mapOf(
"frameAddress" to "0xfffffffe",
"lineNumber" to 4194967233L,
"loadAddress" to "0x242023",
"symbolAddress" to "0x308",
"method" to "makinBacon",
"file" to "lib64/libfoo.so",
"isPC" to true
),
mapOf(
"frameAddress" to "0xb37a644b",
"lineNumber" to 0L,
"loadAddress" to "0x0",
"symbolAddress" to "0x0",
"method" to "0xb37a644b" // test address to method hex
)
)
)
),
output["exceptions"]
)

// metadata
assertEquals(
mapOf(
"app" to mapOf(
"activeScreen" to "Menu",
"weather" to "rain"
),
"metrics" to mapOf(
"experimentX" to false,
"subject" to "percy",
"counter" to 47.5.toBigDecimal()
)
),
output["metaData"]
)

// session info
assertEquals(
mapOf(
"id" to "aaaaaaaaaaaaaaaa",
"startedAt" to "2031-07-09T11:08:21+00:00",
"events" to mapOf(
"handled" to 5L,
"unhandled" to 2L
)
),
output["session"]
)

// threads
val threads = output["threads"]
if (threads is List<Any?>) {
assertEquals(8, threads.size)
threads.forEachIndexed { index, thread ->
assertEquals(
mapOf(
"name" to "Thread #$index",
"state" to "paused-$index",
"id" to 1000L + index,
"type" to "c"
),
thread
)
}
} else {
fail("threads is not a list of thread objects?!")
}

// user
assertEquals(
mapOf(
"email" to "fenton@io.example.com",
"name" to "Fenton",
"id" to "fex01"
),
output["user"]
)
}

/** Migrate an event to the latest format, writing JSON to tempFilePath */
external fun migrateEvent(tempFilePath: String)

/** Migrate notifier and apiKey info to a bespoke structure (apiKey and
* notifier are not included in event info written to disk) */
external fun migratePayloadInfo(tempFilePath: String): String
}
1 change: 1 addition & 0 deletions bugsnag-plugin-android-ndk/src/main/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ add_library( # Specifies the name of the library.
jni/utils/serializer/event_writer.c
jni/utils/serializer/json_writer.c
jni/utils/stack_unwinder.cpp
jni/utils/seqlock.c
jni/utils/serializer.c
jni/utils/string.c
jni/utils/threads.c
Expand Down
2 changes: 1 addition & 1 deletion bugsnag-plugin-android-ndk/src/main/jni/event.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
/**
* Version of the bugsnag_event struct. Serialized to report header.
*/
#define BUGSNAG_EVENT_VERSION 11
#define BUGSNAG_EVENT_VERSION 12

#ifdef __cplusplus
extern "C" {
Expand Down
27 changes: 20 additions & 7 deletions bugsnag-plugin-android-ndk/src/main/jni/featureflags.c
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "bugsnag_ndk.h"
#include "featureflags.h"
#include "utils/seqlock.h"
#include "utils/string.h"

#ifdef __cplusplus
Expand All @@ -26,7 +27,9 @@ extern "C" {
* overhead reasonable.
*/

static const int INDEX_NOT_FOUND = -1;
#define INDEX_NOT_FOUND (-1)

bsg_seqlock_t bsg_feature_flag_lock;

static int index_of_flag_named(const bugsnag_event *const event,
const char *const name) {
Expand Down Expand Up @@ -93,33 +96,43 @@ static void modify_at_index_and_reinsert(bugsnag_event *const event,

void bsg_set_feature_flag(bugsnag_event *event, const char *const name,
const char *const variant) {
bsg_seqlock_acquire_write(&bsg_feature_flag_lock);
const int index = index_of_flag_named(event, name);
if (index == INDEX_NOT_FOUND) {
insert_new(event, name, variant);
} else {
modify_at_index_and_reinsert(event, index, variant);
}
bsg_seqlock_release_write(&bsg_feature_flag_lock);
}

void bsg_clear_feature_flag(bugsnag_event *const event,
const char *const name) {
bsg_seqlock_acquire_write(&bsg_feature_flag_lock);
const int index = index_of_flag_named(event, name);
if (index != INDEX_NOT_FOUND) {
free_flag_contents(&event->feature_flags[index]);
remove_at_index_and_compact(event, index);
event->feature_flag_count--;
}
bsg_seqlock_release_write(&bsg_feature_flag_lock);
}

void bsg_free_feature_flags(bugsnag_event *const event) {
for (int index = 0; index < event->feature_flag_count; index++) {
free_flag_contents(&event->feature_flags[index]);
}
bsg_seqlock_acquire_write(&bsg_feature_flag_lock);
const size_t old_flag_count = event->feature_flag_count;
bsg_feature_flag *old_flags = event->feature_flags;

free(event->feature_flags);

event->feature_flags = NULL;
event->feature_flag_count = 0;
event->feature_flags = NULL;
bsg_seqlock_release_write(&bsg_feature_flag_lock);

// we release the actual memory outside of the lock
for (int index = 0; index < old_flag_count; index++) {
free_flag_contents(&old_flags[index]);
}

free(old_flags);
}

#ifdef __cplusplus
Expand Down
66 changes: 66 additions & 0 deletions bugsnag-plugin-android-ndk/src/main/jni/utils/seqlock.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
#include "seqlock.h"

/*
* Similar to a Sequence Lock (seqlock) in the Linux Kernel, implemented as a
* pure-spinning lock.
*
* When the value of the lock (counter) is odd then a write is in progress. This
* allows us to also perform optimistic-reads without waiting for the lock.
*
* This implementation does *not* support blocking reads, and will not block
* optimistic reads when a write is in-progress. This allows for reads of
* invalid/stale data within a signal handler boundary.
*/

#ifndef spin

#if (defined(__i386__) || defined(__amd64__))
#define spin() __builtin_ia32_pause()
#elif (defined(__aarch64__) || defined(__arm__))
#define spin() asm inline("yield")
#else
#define spin()
#endif

#endif // #ifndef spin

void bsg_seqlock_init(bsg_seqlock_t *lock) { atomic_init(lock, 0uLL); }

void bsg_seqlock_acquire_write(bsg_seqlock_t *lock) {
atomic_thread_fence(memory_order_acquire);
bsg_seqlock_status_t current = atomic_load(lock);

for (;;) {
if (bsg_seqlock_is_write_locked(current)) {
spin();
} else if (atomic_compare_exchange_strong(lock, &current, current + 1)) {
break;
}
}
}

void bsg_seqlock_release_write(bsg_seqlock_t *lock) {
bsg_seqlock_status_t current = atomic_load(lock);

for (;;) {
if (!bsg_seqlock_is_write_locked(current)) {
break;
} else if (atomic_compare_exchange_strong(lock, &current, current + 1)) {
break;
}
}

atomic_thread_fence(memory_order_release);
}

bsg_seqlock_status_t bsg_seqlock_optimistic_read(bsg_seqlock_t *lock) {
bsg_seqlock_status_t status = atomic_load(lock);
// optimistic reads are never valid during a write, and we return 0 in these
// cases validate will always return `false` in these cases
return bsg_seqlock_is_write_locked(status) ? 0 : status;
}

bool bsg_seqlock_validate(bsg_seqlock_t *lock, bsg_seqlock_status_t expected) {
atomic_thread_fence(memory_order_acquire);
return atomic_load(lock) == expected;
}
Loading

0 comments on commit d28e909

Please sign in to comment.