search

getsentry / sentry-native

Sentry SDK for C, C++ and native applications.
MIT License
405 stars 170 forks source link

fix: correctly support stack overflows across platforms #982

Closed supervacuus closed 6 months ago

supervacuus commented 7 months ago

Currently, stack overflows on Windows die somewhere in the handler because we run out of stack space. This is the first attempt to fix the situation by reserving stack space before it is exhausted. This is an analog to sigaltstack() on Linux, but Windows only provides a "reserved amount" interface, and with that, you have fewer options to shoot yourself in the foot.

Fixes #987. Fixes #977.

supervacuus commented 7 months ago

The macOS tests fail because AppleClang 13.0.0.13000029 seemingly ignores -fno-optimize-sibling-calls. However, locally with 15.0.0.15000309, the tests run successfully. I will add a corresponding @pytest.mark.skipif until we update our macOS runner images (which is a bigger change than I am willing to introduce here).

supervacuus commented 6 months ago

The macOS tests fail because AppleClang 13.0.0.13000029 seemingly ignores -fno-optimize-sibling-calls. However, locally with 15.0.0.15000309, the tests run successfully. I will add a corresponding @pytest.mark.skipif until we update our macOS runner images (which is a bigger change than I am willing to introduce here).

After rebasing master to fix the GitHub-runner issue, the same problem is apparent with clang from the older NDK.

supervacuus commented 6 months ago

This has been quite a revealing exercise. That a runtime approach to the sigaltstack fixed the "Old NDK"-build means that the bionic-libc implementations differ between the NDK versions (or rather API system image versions) regarding whether a sigaltstack was installed for each thread. This doesn't necessarily mean this will also be true when these threads are started through the ART. Still, it is noteworthy, especially given the recent influx of issues with older devices.

This started as a Windows fix, but since I tried to make the stack overflow tests run on all builds, we also have improvements on Linux and Android. So, renaming the PR and additional change-log entries might be in order.

cc: @kahest