UnhandledCPPExceptionsV2 GA

[philipphofmann]

Description

#5256 added caputring unhandled C++ Exceptions via swapping __cxa_throw as an opt in experimental via.

During the PR review, @supervacuus, pointed out the following problems we should address before making the feature GA:

1. Lack of synchronization: cxa_throw itself can and will be called from multiple threads simultaneously, which might be less of a problem if we only read.
2. Throwing during init: When you rebind __cxa_throw with your decorator, another thread could already be throwing and entering your decorator. At the same time, the g_cxa_originals structure is still not finished. The problem with that is that we must call an "original"; otherwise, we crash. We can either guarantee that the SDK initialization is completed before any other thread can throw, or we create an explicit abort() path that logs the situation, so users aren't confronted with an indecipherable crash in this scenario.
3. Throwing an C++ Exception from Swift async: We should test what happens when throwing a C++ Exception from Swift async because backtrace might fail in that case here

   sentry-cocoa/Sources/SentryCrash/Recording/Tools/SentryCrashCxaThrowSwapper.c

   Lines 145 to 154 in 877a487

   	void *backtraceArr[k_requiredFrames];
   	int count = backtrace(backtraceArr, k_requiredFrames);
   	Dl_info info;
   	if (count < k_requiredFrames) {
   	// This can happen if the throw happened in a signal handler. This is an edge case we ignore
   	// for now. It can also happen with concurrency frameworks for which backtrace does not work
   	// reliably, such as Swift async. It can be that we have to use backtrace_async which uses
   	// the Swift concurrency continuation stack if invoked from within an async context. Again
   	// we ignore this edge case for now.

4. Throwing an C++ Exception from a signal handler: We should test what happens when throwing a C++ Exception from a signal handler. This is strictly speaking not allowed and an edge case, but we already experienced this from customers using the native SDK. One idea is to detect this here and somehow abort or otherwise communicate with an error message that users shouldn't do this

   sentry-cocoa/Sources/SentryCrash/Recording/Tools/SentryCrashCxaThrowSwapper.c

   Lines 149 to 159 in 877a487

   	if (count < k_requiredFrames) {
   	// This can happen if the throw happened in a signal handler. This is an edge case we ignore
   	// for now. It can also happen with concurrency frameworks for which backtrace does not work
   	// reliably, such as Swift async. It can be that we have to use backtrace_async which uses
   	// the Swift concurrency continuation stack if invoked from within an async context. Again
   	// we ignore this edge case for now.
   	// Returning early here and not calling cxa_throw is fatal, but we cannot do anything else.
   	SENTRY_ASYNC_SAFE_LOG_ERROR("Received only %d frames from backtrace. We can't identify "
   	"throwsite and therefore can't call the original cxa_throw.",
   	count);

5. Add a test ensuring to keep the orignial __cxa_throw pairs to ensure we call the original __cxa_throw methods, even when unswapping fails. We currently only have a comment explaining this, but this can break easily. We had crashes when running test on Mac Catalyst when clearing out the pairs. Finding this wasn't easy and this can break again in the future:

   sentry-cocoa/Sources/SentryCrash/Recording/Tools/SentryCrashCxaThrowSwapper.c

   Lines 412 to 414 in 26d6d49

   	// We MUST NOT clear the pairs because a if we can't unswap one of the cxa_throw handlers, we
   	// still MUST call the original cxa_throw handler.
   	g_cxa_throw_handler = NULL;

6. Find the root cause why the test testGenerateVideo failed in feat: Capturing fatal CPPExceptions via cxa_throw #5256 when clearing the SentryCrashImageToOriginalCxaThrowPair when unswapping. One idea is to enable async safe logs while still removing the pairs and check the log output.
7. Check what happens when we throw an unintentional exception in SentryCrashMonitor_CPPException.captureStacktrace. Does this then call the __cxa_throw_decorator and we end up in a potential endless loop?
8. Consider handling realloc in SentryCrashCxaThrowSwapper.addPair correctly as pointed out here feat: Capturing fatal CPPExceptions via cxa_throw #5256 (comment).

[noahsmartin]

One thing to note about this: Using fishhook is not supported by Apple, and it doesn't work if a system library is calling the method. So for example: if a framework like "AttributeGraph", which is c++ and made by Apple, throws and exception, it will use the dyld closure and not lookup the address of __cxa_throw the way that works with fishhook. In future OS versions it may change more. So for now this solution will only work for calls to cxa_throw that originate in first party code. It may be possible to use something like hardware breakpoints to hook this method regardless of the caller

[philipphofmann]

Thanks for the update, @noahsmartin. Maybe we should consider keeping this feature always as an opt-in due to your explained limitation. Our users can choose whether they want to use it or not. If it works for them, we can keep it.