Skip to content

Commit

Permalink
Let MemoryIdler not madvise away main thread stack
Browse files Browse the repository at this point in the history
Summary:
[folly] Let `MemoryIdler` not madvise away main thread stack on Linux since it poses the risk of zeroing arbitrary heap memory.

Fixes #1488.

Reviewed By: ot, simpkins

Differential Revision: D28164126

fbshipit-source-id: 597f17310b707db663ba4b037879373c0f674186
  • Loading branch information
yfeldblum authored and facebook-github-bot committed Jun 6, 2023
1 parent ff7a464 commit 1b7cd10
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 0 deletions.
24 changes: 24 additions & 0 deletions folly/detail/MemoryIdler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
#include <folly/portability/PThread.h>
#include <folly/portability/SysMman.h>
#include <folly/portability/Unistd.h>
#include <folly/system/Pid.h>
#include <folly/system/ThreadId.h>

FOLLY_GFLAGS_DEFINE_bool(
folly_memory_idler_purge_arenas,
Expand All @@ -43,6 +45,24 @@ namespace detail {
AtomicStruct<std::chrono::steady_clock::duration>
MemoryIdler::defaultIdleTimeout(std::chrono::seconds(5));

bool MemoryIdler::isUnmapUnusedStackAvailable() noexcept {
// Linux uses an automatic stack expansion mechanism to expand the main thread
// stack on demand. Before the main thread stack grows to its full extent, the
// vma corresponding to the main thread stack is not yet fully allocated. It's
// possible for the kernel to allocate the not-yet-allocated main thread stack
// vma to ramdon sbrk() or mmap() requests, and for the resulting regions from
// these requests to be used by other user code. If this happens, the madvise-
// dontneed here is dangerous - it can zero arbitrary heap buffers! So it must
// be skipped. In the case where this runs a fork() child in that thread which
// returned from fork(), the os-thread-id will coincide with the pid, which is
// a harmless false positive where the madvise-dontneed will be skipped.
if (kIsLinux && getOSThreadID() == static_cast<uint64_t>(get_cached_pid())) {
return false;
}

return true;
}

void MemoryIdler::flushLocalMallocCaches() {
if (!usingJEMalloc()) {
return;
Expand Down Expand Up @@ -163,6 +183,10 @@ FOLLY_NOINLINE static uintptr_t getStackPtr() {
}

void MemoryIdler::unmapUnusedStack(size_t retain) {
if (!isUnmapUnusedStackAvailable()) {
return;
}

if (tls_stackSize == 0) {
fetchStackLimits();
}
Expand Down
2 changes: 2 additions & 0 deletions folly/detail/MemoryIdler.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ struct MemoryIdler {
kDefaultStackToRetain = 1024,
};

static bool isUnmapUnusedStackAvailable() noexcept;

/// Uses madvise to discard the portion of the thread's stack that
/// currently doesn't hold any data, trying to ensure that no page
/// faults will occur during the next retain bytes of stack allocation
Expand Down
10 changes: 10 additions & 0 deletions folly/test/MemoryIdlerTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@
using namespace folly::detail;
using namespace testing;

TEST(MemoryIdler, isUnmapUnusedStackAvailableInMainThread) {
// presume this is the main thread
EXPECT_NE(folly::kIsLinux, MemoryIdler::isUnmapUnusedStackAvailable());
}

TEST(MemoryIdler, isUnmapUnusedStackAvailableInAltThread) {
auto fn = [&] { EXPECT_TRUE(MemoryIdler::isUnmapUnusedStackAvailable()); };
std::thread(fn).join(); // definitely not the main thread
}

TEST(MemoryIdler, releaseStack) {
MemoryIdler::unmapUnusedStack();
}
Expand Down

0 comments on commit 1b7cd10

Please sign in to comment.