Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter out NSException in cxa_throw #493

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Conversation

xuezhulian
Copy link
Contributor

@xuezhulian xuezhulian commented May 28, 2024

There are numerous C++ exceptions in the iOS system that are caught and then rethrown by the system, or caught and directly terminated. Therefore, obtaining a C++ crash stack trace on iOS is relatively troublesome. A common solution is to hook __cxa_throw.

In KSCrash, there are two ways to implement the hook. When g_cxaSwapEnabled is true, a technique similar to fishhook or dyld interposing is used to implement the hook. At the same time, __cxa_throw is also overridden. This method only works for the main executable file. When obtaining the stack trace in the hooked __cxa_throw method, it is stored in a global variable and is not thread-local. When multiple crashes are triggered, incorrect stack traces may be obtained. Therefore, it is necessary to reduce method reentry for __cxa_throw.

For NSException, the stack trace can be obtained through system APIs, and it is not necessary to obtain it in __cxa_throw. Therefore, this filtering is valuable and also provides some performance optimization.

@GLinnik21 GLinnik21 requested a review from bamx23 May 28, 2024 12:47
@GLinnik21
Copy link
Collaborator

Please provide a detailed description of the problem. I am particularly interested in the monitors and flags used in the installation.

{
return orig_cxa_throw(thrown_exception, tinfo, dest);
}
if (g_cxaSwapEnabled == false)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if cxaSwap enabled?

@xuezhulian
Copy link
Contributor Author

Additionally, even if we hook __cxa_throw, due to the dyld shared cache, some system libraries will still fail, resulting in some C++ stack traces being unobtainable. I am currently working on solving this issue, and once the solution is stable and reliable, I will discuss it with you further.

@GLinnik21
Copy link
Collaborator

It looks like __attribute__ ((weak)) doesn't work with SPM, probably because of Clang modules. When only CocoaPods was used and a podspec was a single module, this attribute worked fine. It does work with SPM if you remove the attribute, but that can cause symbol duplicates, which isn't ideal. So, the only good method left for SPM seems to be a fishhook-like swap. Both @bamx23 and I are thinking of enabling this by default for C++. The latest update has made this solution more flexible, keeping up with Apple's constant changes to the Mach-O layout.

@xuezhulian
Copy link
Contributor Author

👍🏻 In fact, we've been using fishhook to replace __cxa_throw for quite some time now. Here's a suggestion: don't perform this operation at startup, as it might trigger the watchdog.

@GLinnik21
Copy link
Collaborator

GLinnik21 commented May 29, 2024

Here's a suggestion: don't perform this operation at startup, as it might trigger the watchdog.

We thought about moving it to a separate thread. Would that help with startup performance?

@xuezhulian
Copy link
Contributor Author

The approach I'm currently using involves an asynchronous thread with a delay. Fishhook frequently calls dyld's APIs, which use global locks. During the app startup process, loading dyld also involves these locks. Executing fishhook during this process might cause the main thread to wait for these locks.

@GLinnik21
Copy link
Collaborator

For some reason, my checks show that accessing symbols and checking memory protection takes up most of the time.

image

@xuezhulian
Copy link
Contributor Author

Due to time constraints, I am unable to locate the previous watchdog stack trace. From what I remember, this issue has never been reproduced offline, and the lengthy duration is caused by the page fault.

Copy link
Collaborator

@bamx23 bamx23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@GLinnik21
Copy link
Collaborator

@xuezhulian let's fix PR checks and merge

if (tinfo != nullptr && strcmp(tinfo->name(), "NSException") == 0)
{
return orig_cxa_throw(thrown_exception, tinfo, dest);
}
if (g_cxaSwapEnabled == false)
{
captureStackTrace(NULL, NULL, NULL);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return is not allowed in this function.

And I would also suggest just calling captureStackTrace with parameters and keep the NSException check there (to also awoid code duplication).

Suggested change
if (tinfo != nullptr && strcmp(tinfo->name(), "NSException") == 0)
{
return orig_cxa_throw(thrown_exception, tinfo, dest);
}
if (g_cxaSwapEnabled == false)
{
captureStackTrace(NULL, NULL, NULL);
}
if (g_cxaSwapEnabled == false)
{
captureStackTrace(thrown_exception, tinfo, dest);
}

@xuezhulian
Copy link
Contributor Author

@GLinnik21 @bamx23 Thank you for your modification suggestions. It seems that the lint failure is due to a timeout, but I haven't found an option to retry.

@bamx23
Copy link
Collaborator

bamx23 commented Jun 18, 2024

@xuezhulian thanks for your contribution!

@bamx23 bamx23 merged commit 5d68d4e into kstenerud:master Jun 18, 2024
19 checks passed
bamx23 pushed a commit that referenced this pull request Jun 18, 2024
Co-authored-by: yuencong <yuencong@kuaishou.com>
# Conflicts:
#	Source/KSCrash/Recording/Monitors/KSCrashMonitor_CPPException.cpp
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants