From a1dec755f7ed577273c6cb396621a306ea1a572e Mon Sep 17 00:00:00 2001 From: Tzvetan Mikov Date: Sun, 13 Oct 2024 17:10:02 -0700 Subject: [PATCH] fix native stack checking on Android/Bionic Summary: Related to: https://github.com/facebook/hermes/issues/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 --- lib/Support/OSCompatPosix.cpp | 19 ++- unittests/Support/CMakeLists.txt | 1 + unittests/Support/StackBoundsTest.cpp | 159 ++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 unittests/Support/StackBoundsTest.cpp diff --git a/lib/Support/OSCompatPosix.cpp b/lib/Support/OSCompatPosix.cpp index fd2e83e5574..8db2e57ec3c 100644 --- a/lib/Support/OSCompatPosix.cpp +++ b/lib/Support/OSCompatPosix.cpp @@ -642,7 +642,24 @@ std::pair thread_stack_bounds(unsigned gap) { void *origin; size_t size; - pthread_attr_getstack(&attr, &origin, &size); + if (pthread_attr_getstack(&attr, &origin, &size)) + hermes_fatal("Unable to obtain native stack bounds"); + +#ifdef __BIONIC__ + // It appears that on Android/Bionic, the range returned by + // pthread_attr_getstack() includes the stack guard pages. We must remove them + // from the bounds. + size_t guardSize; + if (pthread_attr_getguardsize(&attr, &guardSize)) { + // Don't give up in case of error. + guardSize = 0; + } + if (guardSize > size) + guardSize = size; + // Exclude the guard pages from the available stack. + origin = (char *)origin + guardSize; + size -= guardSize; +#endif pthread_attr_destroy(&attr); diff --git a/unittests/Support/CMakeLists.txt b/unittests/Support/CMakeLists.txt index 41b01823ade..be97ed4a001 100644 --- a/unittests/Support/CMakeLists.txt +++ b/unittests/Support/CMakeLists.txt @@ -21,6 +21,7 @@ set(SupportSources RegexTest.cpp SNPrintfBufTest.cpp SourceErrorManagerTest.cpp + StackBoundsTest.cpp StatsAccumulatorTest.cpp StringSetVectorTest.cpp UnicodeTest.cpp diff --git a/unittests/Support/StackBoundsTest.cpp b/unittests/Support/StackBoundsTest.cpp new file mode 100644 index 00000000000..e9523447f4d --- /dev/null +++ b/unittests/Support/StackBoundsTest.cpp @@ -0,0 +1,159 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include "hermes/Support/OSCompat.h" + +#include "gtest/gtest.h" + +#include + +using namespace hermes; + +namespace { + +/// Upper bound on the stack, nullptr if currently unknown. +const void *nativeStackHigh{nullptr}; +/// This has already taken \c nativeStackGap into account, +/// so any stack outside [nativeStackHigh-nativeStackSize, nativeStackHigh] +/// is overflowing. +size_t nativeStackSize{0}; +/// Native stack remaining before assuming overflow. +unsigned nativeStackGap = 16 * 1024; + +/// Slow path for \c isOverflowing. +/// Sets \c stackLow_ \c stackHigh_. +/// \return true if the native stack is overflowing the bounds of the +/// current thread. +bool isStackOverflowingSlowPath() { + auto [highPtr, size] = oscompat::thread_stack_bounds(nativeStackGap); + nativeStackHigh = (const char *)highPtr; + nativeStackSize = size; + void *sp = __builtin_frame_address(0); + return (uintptr_t)nativeStackHigh - (uintptr_t)sp > nativeStackSize; +} + +/// \return true if the native stack is overflowing the bounds of the +/// current thread. Updates the stack bounds if the thread which Runtime +/// is executing on changes. +inline bool isOverflowing() { + void *sp = __builtin_frame_address(0); + // Check for overflow by subtracting the sp from the high pointer. + // If the sp is outside the valid stack range, the difference will + // be greater than the known stack size. + // This is clearly true when 0 < sp < nativeStackHigh_ - size. + // If nativeStackHigh_ < sp, then the subtraction will wrap around. + // We know that nativeStackSize_ <= nativeStackHigh_ + // (because otherwise the stack wouldn't fit in the memory), + // so the overflowed difference will be greater than nativeStackSize_. + if (!((uintptr_t)nativeStackHigh - (uintptr_t)sp > nativeStackSize)) { + // Fast path: quickly check the stored stack bounds. + // NOTE: It is possible to have a false negative here (highly unlikely). + // If the program creates many threads and destroys them, a new + // thread's stack could overlap the saved stack so we'd be checking + // against the wrong bounds. + return false; + } + // Slow path: might be overflowing, but update the stack bounds first + // in case execution has changed threads. + return isStackOverflowingSlowPath(); +} + +/// Helper variable for recursiveCall(). +volatile unsigned recCount = 0; +volatile char *volatile dataPtr; + +/// A recursive call that reliably stops when it is about to overflow the stack. +unsigned recursiveCall() { + volatile char data[2000]; + dataPtr = data; + + ++recCount; + if (isOverflowing()) { + printf("Stack overflow, count=%u\n", recCount); + return recCount; + } + + // Prevent a tall call. + return recursiveCall() + (--recCount); +} + +void unboundedRecursion() { + if (!oscompat::thread_stack_bounds(0).first) // Unsupported on this platform + return; + recursiveCall(); +} + +volatile unsigned sum = 0; + +/// Ensure that we can read from the entire extent of the reported stack +void manualStackScan() { + auto [high, size] = oscompat::thread_stack_bounds(0); + if (!high) // Unsupported on this platform + return; + const char *low = (const char *)high - size; + for (const char *p = (const char *)high - 16; p > low; p -= 2048) { + volatile char x = *((volatile const char *)p); + sum += x; + } +} + +TEST(StackBoundsTest, manualStackScan_mainThread) { + manualStackScan(); +} +TEST(StackBoundsTest, unboundRecursion_mainThread) { + unboundedRecursion(); +} +TEST(StackBoundsTest, manualStackScan_thread) { + std::thread t(manualStackScan); + t.join(); +} +TEST(StackBoundsTest, unboundRecursion_thread) { + std::thread t(unboundedRecursion); + t.join(); +} + +#if !defined(_WINDOWS) && !defined(__EMSCRIPTEN__) +void runInThreadWith64KGuard(void *(threadFunc)(void *)) { + pthread_t thread; + pthread_attr_t attr; + size_t guard_size = 64 * 1024; // 64KB + + // Initialize thread attributes object + pthread_attr_init(&attr); + + // Set the guard size attribute + if (pthread_attr_setguardsize(&attr, guard_size) != 0) { + perror("Failed to set guard size"); + exit(EXIT_FAILURE); + } + + if (pthread_create(&thread, &attr, threadFunc, nullptr) != 0) { + perror("Failed to create thread"); + exit(EXIT_FAILURE); + } + + // Clean up + pthread_attr_destroy(&attr); + pthread_join(thread, nullptr); +} + +TEST(StackBoundsTest, manualStackScan_thread64KGuard) { + runInThreadWith64KGuard([](void *) -> void * { + manualStackScan(); + return nullptr; + }); +} +TEST(StackBoundsTest, unboundRecursion_thread64KGuard) { + runInThreadWith64KGuard([](void *) -> void * { + unboundedRecursion(); + return nullptr; + }); +} + +#endif + +} // namespace