sentry-native icon indicating copy to clipboard operation
sentry-native copied to clipboard

Log explanatory string of std::exception when logging uncaught C++ exceptions

Open triplef opened this issue 2 years ago • 12 comments

When an unhandled C++ exception is logged, it would in some cases be very helpful to also see the exception message returned by std::exception::what().

Would it be possible for Sentry in its exception handler to check the exception type, and if it’s of type std::exception log its explanatory string as the error message?

triplef avatar Jun 30 '23 09:06 triplef

Hi @triplef, the best way to achieve this is to implement a terminate_handler in the SDK and allow users to enable it via the options. This is a C++-specific handler where we can be sure that we are processing a C++ exception instead of any abort() (which the default terminate_handler uses to close the program).

It should be relatively easy to implement this for the inproc backend (I guess you are asking for Android?). I would love to have this in all backends, but we can start with one. I'll look into it, but I can't promise how soon we'll tackle this.

supervacuus avatar Jul 01 '23 10:07 supervacuus

Sounds great, thank you @supervacuus! We'd be interested in this on Android and Windows (particularly on Windows as we have more 3rd party C++ code there which might throw exceptions like that).

triplef avatar Jul 01 '23 12:07 triplef

If we implemented this on the app side would something like this work?

std::set_terminate([](){
	sentry_value_t stacktrace = sentry_value_new_stacktrace(NULL, 0);

	std::exception_ptr e = std::current_exception();
	const char *message = "Unknown exception";
	try {
		std::rethrow_exception(e);
	} catch (std::exception &e) {
		message = e.what();
#if defined(_WIN32)
	} catch (winrt::hresult_error const& e) {
		message = winrt::to_string(e.message()).c_str();
#endif
	}

	sentry_value_t exc = sentry_value_new_exception(typeid(e).name(), message);
	sentry_value_set_by_key(exc, "stacktrace", stacktrace);
	sentry_value_t event = sentry_value_new_event();
	sentry_event_add_exception(event, exc);
	sentry_capture_event(event);
	sentry_close();

	std::abort();
});

triplef avatar Oct 24 '23 06:10 triplef

Hi @triplef. The obvious answer is: it depends ;-).

If you rethrow, you must ensure you catch all possible exceptions. Not all exceptions inherit from std::exception or winrt::hresult_error, so a "catch-all" is missing. Be aware that what() in many cases of the standard library contains very little information (like "vector", i.e., only containing a reference to the type that threw).

The upside of using rethrow in an application is that you can add exception types to improve your handling of these exceptions. Of course, for this to work, you need to know these exceptions at compile time.

Using typeid() to get the - often much more informative - exception type depends on your dependencies being built/packaged with type information. typeid() can return a nullptr and should be checked accordingly.

Otherwise, minimize the use of the C++ standard library as much as possible or anything that could throw another C++ exception from within the terminate handler (this is not an issue in the example).

supervacuus avatar Oct 25 '23 09:10 supervacuus

Thank you @supervacuus, that’s super helpful feedback. 🙏

triplef avatar Oct 27 '23 13:10 triplef

This is what I ended up with after adding a catch-all in case this is useful to anyone else.

std::set_terminate([](){
	// try to figure out exception type and possible message
	std::exception_ptr e = std::current_exception();
	const char *type = nullptr;
	const char *message = nullptr;
	try {
		std::rethrow_exception(e);
	} catch (const std::exception& e) {
		type = typeid(e).name();
		message = strdup(e.what());
#if defined(_WIN32)
	} catch (winrt::hresult_error const& e) {
		type = typeid(e).name();
		message = strdup(winrt::to_string(e.message()).c_str());
#endif
	} catch (...) {
		// unknown exception type
		message = "unknown";
	}
	if (!type) {
		type = "Unknown exception";
	}

	sentry_value_t exc = sentry_value_new_exception(type, message);
	sentry_value_t stacktrace = sentry_value_new_stacktrace(NULL, 0);
	sentry_value_set_by_key(exc, "stacktrace", stacktrace);
	sentry_value_t mechanism = sentry_value_new_object();
	sentry_value_set_by_key(mechanism, "type", sentry_value_new_string("cpp_exception"));
	sentry_value_set_by_key(mechanism, "handled", sentry_value_new_bool(false));
	sentry_value_set_by_key(exc, "mechanism", mechanism);
	sentry_value_t event = sentry_value_new_event();
	sentry_value_set_by_key(event, "level", sentry_value_new_string("fatal"));
	sentry_event_add_exception(event, exc);
	sentry_capture_event(event);
	sentry_close();

	fprintf(stderr, "Uncaught exception %s, message: %s\n", type, message);
	fflush(stderr);

	std::abort();
}

typeid() can return a nullptr and should be checked accordingly.

Did you mean that typeid().name() can return a nullptr? I think the return value of typeid() is a struct and not a pointer.

Interestingly I found that on macOS C++ exceptions are already reported to Sentry (see SentryCrashMonitor_CPPException.cpp), so this solution is only needed for sentry-native.

triplef avatar Dec 06 '23 10:12 triplef

@supervacuus unfortunately we found that using set_terminate on the app side doesn’t seem to work on Windows (any more?) – when Sentry is enabled the terminate handler never gets called when a C++ exception is thrown (but it is called when Sentry is disabled), so we’re unable to log C++ exception messages.

Even calling set_terminate right before throwing an exception doesn’t work, so I guess Crashpad is catching exceptions another way. Is there any other hook we could use to implement this?

We’re using sentry-native v7.3.5.

triplef avatar Jul 05 '24 14:07 triplef

@triplef we'll investigate and get back to you.

We’re using sentry-native v7.3.5.

The latest version is 0.7.6 - did you mean you're using 0.7.5? And did you upgrade recently?

kahest avatar Jul 09 '24 14:07 kahest

Thank you! We haven’t updated to 0.7.6 yet, but from what I can see in the release notes it seemed unlikely to affect this.

triplef avatar Jul 09 '24 15:07 triplef

Hi @triplef, sorry for the delay, I was on vacation.

unfortunately we found that using set_terminate on the app side doesn’t seem to work on Windows (any more?) – when Sentry is enabled the terminate handler never gets called when a C++ exception is thrown (but it is called when Sentry is disabled), so we’re unable to log C++ exception messages.

This is not a regression. The effort in implementing this feature in Native SDK cross-platform is also related to the wildly diverging interactions between the operating system's low-level error mechanisms and how C++ exception handling is implemented on that platform.

On Windows, all C++ exceptions are only special SEH exceptions. So whenever a C++ exception is raised, the SEH machinery is used for propagation. Concretely, this means that setting a terminate handler on Windows conflicts with installing your own UnhandledExceptionFilter (or, in our case, the one provided by our crash backends). The default CRT SEH handler supports terminate handlers, but when, for instance, crashpad overrides it, then nothing propagates a C++ exception to a terminate handler installed at any point of the execution.

Considering you still want to use an implementation similar to the one you proposed earlier (i.e., manually constructing and sending a crash event from inside a terminate handler), one thing you can do is to store the default UnhandledExceptionFilter installed by the CRT before you initialize the Native SDK and then call it from within an on_crash hook. The sequence would look a bit like this:

#define EH_EXCEPTION_NUMBER 0xE06D7363

// 1. Define an on_crash hook that checks in the crash context whether the SEH exception is a C++ exception
// in which case it executes the default handler, which will execute your terminate handler.
// This assumes that the handler will terminate the program.
sentry_value_t
on_crash(const sentry_ucontext_t *uctx, sentry_value_t event, void *closure)
{
    EXCEPTION_RECORD *exception_record = uctx->exception_ptrs.ExceptionRecord;
    // Check whether this was a C++ exception
    if (exception_record->ExceptionCode == EH_EXCEPTION_NUMBER) {
        // invoke the default_filter and pass it the exception_ptrs from our crash context.
        auto default_filter = reinterpret_cast<LPTOP_LEVEL_EXCEPTION_FILTER>(closure);
        if (default_filter) {
            default_filter(const_cast<struct _EXCEPTION_POINTERS *>(&uctx->exception_ptrs));
            // This will never execute because the terminate handler should exit
            return sentry_value_new_null();
        }
    }

    // if it wasn't a C++ exception continue with a minidump + upload
    return event;
}

// 2. Retrieve the default UEF
LPTOP_LEVEL_EXCEPTION_FILTER default_filter = SetUnhandledExceptionFilter(nullptr);
set_terminate([]() {
    // your handler code, ensure to exit or TerminateProcess here rather than abort()
    sentry_close();
    std::exit(EXIT_FAILURE);
});

// 3. Init sentry with an on_crash hook and pass `default_filter` as the `closure` argument
sentry_options_set_on_crash(options, on_crash, reinterpret_cast<void *>(default_filter));
sentry_init(options);

Of course, this is unsupported usage, but it is the only way I can imagine fixing the issue on Windows without doing the steps required to implement the feature itself.

supervacuus avatar Jul 16 '24 19:07 supervacuus

Thank you @supervacuus, this is incredibly helpful.

One more somewhat related question: WinRT errors also have a code which we’d like to log as well (in addition to the message string, which might be localized). Does misusing the meta.errno object for this sound ok, or is there a better way to add it to the exception object?

sentry_value_t errnoObj = sentry_value_new_object();
sentry_value_set_by_key(errnoObj, "number", sentry_value_new_int32(code));
sentry_value_t meta = sentry_value_new_object();
sentry_value_set_by_key(meta, "errno", errnoObj);
sentry_value_set_by_key(mechanism, "meta", meta);

triplef avatar Jul 17 '24 08:07 triplef

Does misusing the meta.errno object for this sound ok, or is there a better way to add it to the exception object?

No, I wouldn't recommend that. AFAIK relay normalizes errno codes with descriptive strings. Even with no numerical overlap, you always open yourself to wrong interpretations of this field.

mechanism.data allows you to send an object with arbitrary entries of your liking.

supervacuus avatar Jul 20 '24 11:07 supervacuus