fix native stack checking on Android/Bionic #1538
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: Related to: facebook#1535 It turns out that on Android/Bionic `pthread_attr_getstack()` includes the stack guard pages in the returned size. Whether that is a bug or not is debatable, but it differs in behavior from Linux/glibc. Plus, returning a stack range that is not actually not all accessible is less than useful. In practice it worked with our approach because the default stack guard size is 4KB on Android, while our "native gap" is 64KB. Our gap is larger than the stack guard size, so we would still correctly detect and catch stack overflow. However, if the stack guard size is 64KB (or larger), we would not detect the overflow and crash. It is possible to manually set a larger guard size, plus security-oriented OS-es like GrapheneOS do it by default. The fix is to subtract the guard size from the available stack size when running on Bionic. Added tests checking the behavior in the main thread, in a default thread and in a thread with a 64KB guard. Two separate tests are performed under these circumstances: - Unbounded recursion checking for native stack overflow - Manually reading the available stack range to ensure it is all accessible. Differential Revision: D64308925
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D64308925 |
|
This pull request has been merged in a5b2bbc. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Related to: #1535
It turns out that on Android/Bionic
pthread_attr_getstack()includesthe stack guard pages in the returned size. Whether that is a bug or not
is debatable, but it differs in behavior from Linux/glibc. Plus,
returning a stack range that is not actually not all accessible is less
than useful.
In practice it worked with our approach because the default stack guard
size is 4KB on Android, while our "native gap" is 64KB. Our gap is
larger than the stack guard size, so we would still correctly detect and
catch stack overflow.
However, if the stack guard size is 64KB (or larger), we would not
detect the overflow and crash. It is possible to manually set a larger
guard size, plus security-oriented OS-es like GrapheneOS do it by
default.
The fix is to subtract the guard size from the available stack size when
running on Bionic.
Added tests checking the behavior in the main thread, in a default
thread and in a thread with a 64KB guard. Two separate tests are
performed under these circumstances:
accessible.
Differential Revision: D64308925