fix(signal): Fix stacktrace resolution for native crashes on ARM32 #378

kattrali · 2018-10-17T00:10:23Z

Goal

When a signal is raised on 32-bit ARM, the current context is the signal
stack rather than the crash stack. To work around this, set the register
state before unwinding the first frame (using the program counter as the
first frame). Then the stack can be unwound normally.

Changeset

Add signal_context to bsg_libunwind_state
Set the signal_context to the unwind function signal context (which may be null, as it is in the event of using notify()
Use state.signal_context in the unwind callback to set the initial register state

Tests

Manually tested against:

Nexus 4 (ARM32) running API 21
Nexus 5P (ARM32) on API 24
Pixel 2 (ARM64) on API 28
Tested against various x86/x64 emulators to make sure the architecture macros are correct

Automated test verifying the correct stacktrace (file & method) for a given crash

Review

The following items are needed:

Reproducing the initial problem - Signal-based crashes on ARM32 using v4.8.2 (not using abort()) should produce an uninformative stacktrace without the original frames
Confirm consistency between the changeset and the goal stated above.
Explore and discover any crash cases where this change results in a worse stacktrace (or no stacktrace 😱 )

fractalwrench · 2018-10-18T09:36:36Z

ndk/src/main/jni/utils/string.h

+ * True if first and second are the same length and contain the same characters
+ * in the same order.
+ */
+bool bsg_strequal(char *first, char *second) __asyncsafe;


I can't find any usage of this function in the project. We should remove it if it's not used here, or add it in a separate PR if it's needed more generally.

fractalwrench · 2018-10-18T09:50:39Z

.../src/main/java/com/bugsnag/android/mazerunner/scenarios/CXXExternalStackElementScenario.java

+        String metadata = getEventMetaData();
+        if (metadata != null && metadata.equals("non-crashy")) {
+            return;
+        }


~30 files in the scenario package contain the same 4 lines as above, and nearly every scenario contains config.setAutoCaptureSessions(false);.

I think it'd be good to think about whether there are any abstractions/sensible defaults we could use to reduce this duplication. This could be addressed as a separate ticket and shouldn't hold up the work in this PR.

Its still helpful to know for every test case exactly what is going on, and then not fight it if for some reason its needed in a more complex case down the line.

To add some additional context, say we refactor this into a method, what then would it save? Maybe a line at most?

if (shouldNotCrash()) { return; }

Personally I think a method like shouldNotCrash would improve readability. The reader doesn't have to know what event metadata is, why it needs a null-check, or where 'non-crashy' has been set - they only need to know that the author didn't want the scenario to crash at this point.

Additionally, we wouldn't have to worry about updating 30 separate source files if we need to make a change to this feature, and could also add some brief JavaDoc to explain the convention of setting 'non-crashy' as an Environment Variable within a feature.

This doesn't need to be addressed as part of this PR but I do feel it's worth creating a ticket to address this at some future point.

I have taken a look at this and I think that my main problem with it is the name of the variable its stored in. It's too easily confused with event metadata. If we made this called something obviously environment variable like then this code seems fine to me.

features/native_crash_handling.feature

ndk/src/main/jni/utils/stack_unwinder_libunwind.c

fractalwrench · 2018-10-18T10:09:54Z

ndk/src/main/jni/utils/stack_unwinder_libunwind.c

@@ -1,4 +1,5 @@
 #include "../utils/build.h"
+#include "../utils/string.h"


this include doesn't appear to be used?

fractalwrench · 2018-10-18T10:55:42Z

I've left a few comments inline. A couple of other items that may need addressing include:

CI is not passing for this PR
The new mazerunner scenario does not pass on an x86, API 27 emulator

I've tested that this works by running the mazerunner scenario on an ARMv7, API 22 emulator. With the changes the scenario passes and the frames are as expected; without the changes, the stackframe is equal to the signal stack (and does contain a frame referencing libmonochrome, around 4 frames down).

When a signal is raised on 32-bit ARM, the current context is the signal stack rather than the crash stack. To work around this, set the register state before unwinding the first frame (using the program counter as the first frame). Then the stack can be unwound normally. In cases where the signal code is TKILL (6), libunwind hangs / crashes immediately after setting the register values. For these cases, the first frame is recorded and the stack is terminated early to prevent losing the report altogether. Alternative approaches would be to grab every word from the stack and inspect for symbols or to bundle a different unwinder altogether (libstackunwind maybe?). For stack buffer overflows (and potentially other variants of SEGV_MAPERR), libunwind crashes immediately after setting the register values as well.

Add new "third-party" library fixture to validate stacktraces from separate libraries. Add new step to validate stacktraces skipping insignificant frames.

While the stack enhancements added in #378 are intended only to be run on 32-bit devices and gated to the __arm__ build, when 64-bit devices are running the 32-bit library, the stacktraces for those devices were unintentionally affected. This is the case when an app is configured to only run 32-bit binaries (Android cannot run both 64-bit and 32-bit binaries at once), and many popular libraries (e.g. React Native) only distribute 32-bit libraries. Fixes #382

While the stack enhancements added in #378 are intended only to be run on 32-bit devices and gated to the __arm__ build, when 64-bit devices are running the 32-bit library, the stacktraces for those devices were unintentionally affected. This is the case when an app is configured to only run 32-bit binaries (Android cannot run both 64-bit and 32-bit binaries at once), and many popular libraries (e.g. React Native) only distribute 32-bit libraries. To further improve the stack quality on 32-bit, switches the underlying unwinder from gnu unwind to non-gnu libunwind. Improves frame resolution and does not have the crash in unwind when setting registers. Fixes #382

Uses the ABI list to conditionally switch unwinders on ARM. While the stack enhancements added in #378 are intended only to be run on 32-bit devices and gated to the __arm__ build, when 64-bit devices are running the 32-bit library, the stacktraces for those devices were unintentionally affected. This is the case when an app is configured to only run 32-bit binaries (Android cannot run both 64-bit and 32-bit binaries at once), and many popular libraries (e.g. React Native) only distribute 32-bit libraries. To further improve the stack quality on 32-bit, switches the underlying unwinder from gnu unwind to non-gnu libunwind. Improves frame resolution and does not have the crash in unwind when setting registers. Fixes #382

Small refactor of the API 21+ stack unwinding logic to restrict 32-bit unwinding style from 64-bit devices. While the stack enhancements added in #378 are intended only to be run on 32-bit devices and gated to the arm build, when 64-bit devices are running the 32-bit library, the stacktraces for those devices were unintentionally affected. This is the case when an app is configured to only run 32-bit binaries (Android cannot run both 64-bit and 32-bit binaries at once), and many popular libraries (e.g. React Native) only distribute 32-bit libraries. Added additional improvement to the 32-bit stacktrace resolution by switching to nongnu libunwind only for those devices. This significantly improves the number of frames which can be unwound without risk of a segfault.

* task(context): make Context a writable property of the client

kattrali added wip and removed wip labels Oct 17, 2018

$fractalwrench$

fractalwrench reviewed Oct 18, 2018

View reviewed changes

features/native_crash_handling.feature Show resolved Hide resolved

$fractalwrench$

fractalwrench reviewed Oct 18, 2018

View reviewed changes

ndk/src/main/jni/utils/stack_unwinder_libunwind.c Show resolved Hide resolved

$fractalwrench$

fractalwrench reviewed Oct 18, 2018

View reviewed changes

kattrali force-pushed the kattrali/fix-arm32-stack-resolver branch 2 times, most recently from 90edc1a to bd2016d Compare October 19, 2018 01:35

bugsnag deleted a comment from coveralls Oct 19, 2018

kattrali force-pushed the kattrali/fix-arm32-stack-resolver branch from bd2016d to a9509c5 Compare October 19, 2018 05:15

kattrali added 2 commits October 19, 2018 12:01

tests(e2e): Add additional specs for stacktrace quality

e624568

Add new "third-party" library fixture to validate stacktraces from separate libraries. Add new step to validate stacktraces skipping insignificant frames.

kattrali force-pushed the kattrali/fix-arm32-stack-resolver branch from a9509c5 to e624568 Compare October 19, 2018 19:02

kattrali merged commit 5ba5d22 into master Oct 24, 2018

kattrali deleted the kattrali/fix-arm32-stack-resolver branch October 24, 2018 00:09

kattrali mentioned this pull request Nov 3, 2018

fix(ndk): Ensure 64-bit devices running 32-bit binaries do not use 32-bit device unwinding logic #383

Merged

5 tasks

rich-bugsnag added a commit that referenced this pull request Sep 3, 2021

task(context): make Context a writable property of the client (#378)

ca798bd

* task(context): make Context a writable property of the client

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(signal): Fix stacktrace resolution for native crashes on ARM32 #378

fix(signal): Fix stacktrace resolution for native crashes on ARM32 #378

kattrali commented Oct 17, 2018 •

edited

Loading

$@fractalwrench$ fractalwrench Oct 18, 2018

$@fractalwrench$ fractalwrench Oct 18, 2018

kattrali Oct 19, 2018

kattrali Oct 19, 2018

$@fractalwrench$ fractalwrench Oct 19, 2018

snmaynard Oct 19, 2018

$@fractalwrench$ fractalwrench Oct 18, 2018

fractalwrench commented Oct 18, 2018

		@@ -1,4 +1,5 @@
		#include "../utils/build.h"
		#include "../utils/string.h"

fix(signal): Fix stacktrace resolution for native crashes on ARM32 #378

fix(signal): Fix stacktrace resolution for native crashes on ARM32 #378

Conversation

kattrali commented Oct 17, 2018 • edited Loading

Goal

Changeset

Tests

Review

fractalwrench Oct 18, 2018

Choose a reason for hiding this comment

fractalwrench Oct 18, 2018

Choose a reason for hiding this comment

kattrali Oct 19, 2018

Choose a reason for hiding this comment

kattrali Oct 19, 2018

Choose a reason for hiding this comment

fractalwrench Oct 19, 2018

Choose a reason for hiding this comment

snmaynard Oct 19, 2018

Choose a reason for hiding this comment

fractalwrench Oct 18, 2018

Choose a reason for hiding this comment

fractalwrench commented Oct 18, 2018

kattrali commented Oct 17, 2018 •

edited

Loading

$@fractalwrench$ fractalwrench Oct 18, 2018

$@fractalwrench$ fractalwrench Oct 18, 2018

$@fractalwrench$ fractalwrench Oct 19, 2018

$@fractalwrench$ fractalwrench Oct 18, 2018