From e97f1a99cf54b42a6485ea65653971b8f079668b Mon Sep 17 00:00:00 2001 From: Justine Tunney Date: Fri, 9 Sep 2022 06:19:05 -0700 Subject: [PATCH] Fix pthread stacks with larger guard size --- libc/intrin/describemapflags.c | 1 - libc/intrin/describemapping.c | 1 - libc/intrin/pthread.h | 2 +- libc/intrin/pthread_attr_init.c | 3 -- libc/runtime/clone.c | 4 +- libc/sysv/consts/map.h | 2 - libc/thread/pthread_create.c | 53 +++++++++++++------------- test/libc/thread/pthread_create_test.c | 39 +++++++++++++++++++ third_party/libcxx/thread.cc | 4 -- tool/build/lib/xlat.c | 1 - 10 files changed, 68 insertions(+), 42 deletions(-) diff --git a/libc/intrin/describemapflags.c b/libc/intrin/describemapflags.c index 8271d8f9a..fedbf81ca 100644 --- a/libc/intrin/describemapflags.c +++ b/libc/intrin/describemapflags.c @@ -30,7 +30,6 @@ const char *(DescribeMapFlags)(char buf[64], int x) { {MAP_SHARED, "SHARED"}, // {MAP_FIXED, "FIXED"}, // {MAP_FIXED_NOREPLACE, "FIXED_NOREPLACE"}, // - {MAP_GROWSDOWN, "GROWSDOWN"}, // {MAP_CONCEAL, "CONCEAL"}, // {MAP_HUGETLB, "HUGETLB"}, // {MAP_LOCKED, "LOCKED"}, // diff --git a/libc/intrin/describemapping.c b/libc/intrin/describemapping.c index 328ec5cb6..f6e6964db 100644 --- a/libc/intrin/describemapping.c +++ b/libc/intrin/describemapping.c @@ -49,7 +49,6 @@ const char *(DescribeMapping)(char p[8], int prot, int flags) { DescribeProt(p, prot); p[3] = DescribeMapType(flags); p[4] = (flags & MAP_ANONYMOUS) ? 'a' : '-'; - p[5] = (flags & MAP_GROWSDOWN) ? 'G' : '-'; p[6] = (flags & MAP_FIXED) ? 'F' : '-'; p[7] = 0; return p; diff --git a/libc/intrin/pthread.h b/libc/intrin/pthread.h index 8e74f763f..23e4aefda 100644 --- a/libc/intrin/pthread.h +++ b/libc/intrin/pthread.h @@ -85,7 +85,7 @@ typedef struct pthread_attr_s { char detachstate; size_t stacksize; size_t guardsize; - void *stackaddr; + char *stackaddr; int scope; int schedpolicy; int inheritsched; diff --git a/libc/intrin/pthread_attr_init.c b/libc/intrin/pthread_attr_init.c index b5471795e..ee00ed36f 100644 --- a/libc/intrin/pthread_attr_init.c +++ b/libc/intrin/pthread_attr_init.c @@ -30,8 +30,5 @@ int pthread_attr_init(pthread_attr_t *attr) { .stacksize = GetStackSize(), .guardsize = PAGESIZE, }; - if (attr->stacksize >= 1048576) { - attr->guardsize = FRAMESIZE; - } return 0; } diff --git a/libc/runtime/clone.c b/libc/runtime/clone.c index ebddb8d08..e869c4f13 100644 --- a/libc/runtime/clone.c +++ b/libc/runtime/clone.c @@ -595,9 +595,7 @@ int clone(void *func, void *stk, size_t stksz, int flags, void *arg, int *ptid, ((flags & CLONE_VM) && (stksz < PAGESIZE || (stksz & 15)))) { rc = einval(); } else if (IsAsan() && - ((stksz > PAGESIZE && - !__asan_is_valid((char *)stk + PAGESIZE, stksz - PAGESIZE)) || - ((flags & CLONE_SETTLS) && !__asan_is_valid(tls, 64)) || + (((flags & CLONE_SETTLS) && !__asan_is_valid(tls, 64)) || ((flags & CLONE_SETTLS) && !__asan_is_valid(tls, sizeof(long))) || ((flags & CLONE_PARENT_SETTID) && !__asan_is_valid(ptid, sizeof(*ptid))) || diff --git a/libc/sysv/consts/map.h b/libc/sysv/consts/map.h index 30f0bfd02..5dd4482d4 100644 --- a/libc/sysv/consts/map.h +++ b/libc/sysv/consts/map.h @@ -13,7 +13,6 @@ extern const int MAP_EXECUTABLE; extern const int MAP_FILE; extern const int MAP_FIXED; extern const int MAP_FIXED_NOREPLACE; -extern const int MAP_GROWSDOWN; extern const int MAP_HASSEMAPHORE; extern const int MAP_HUGETLB; extern const int MAP_HUGE_MASK; @@ -44,7 +43,6 @@ COSMOPOLITAN_C_END_ #define MAP_DENYWRITE SYMBOLIC(MAP_DENYWRITE) #define MAP_EXECUTABLE SYMBOLIC(MAP_EXECUTABLE) #define MAP_FIXED_NOREPLACE SYMBOLIC(MAP_FIXED_NOREPLACE) -#define MAP_GROWSDOWN SYMBOLIC(MAP_GROWSDOWN) #define MAP_HASSEMAPHORE SYMBOLIC(MAP_HASSEMAPHORE) #define MAP_HUGETLB SYMBOLIC(MAP_HUGETLB) #define MAP_HUGE_MASK SYMBOLIC(MAP_HUGE_MASK) diff --git a/libc/thread/pthread_create.c b/libc/thread/pthread_create.c index 4def12abb..a248e18c8 100644 --- a/libc/thread/pthread_create.c +++ b/libc/thread/pthread_create.c @@ -96,8 +96,8 @@ static int FixupCustomStackOnOpenbsd(pthread_attr_t *attr) { n = ROUNDDOWN(n, PAGESIZE); e = errno; if (__sys_mmap((void *)y, n, PROT_READ | PROT_WRITE, - MAP_PRIVATE | MAP_ANON_OPENBSD | MAP_STACK_OPENBSD, -1, 0, - 0) != MAP_FAILED) { + MAP_PRIVATE | MAP_FIXED | MAP_ANON_OPENBSD | MAP_STACK_OPENBSD, + -1, 0, 0) == (void *)y) { attr->stackaddr = (void *)y; attr->stacksize = n; return 0; @@ -215,8 +215,28 @@ int pthread_create(pthread_t *thread, const pthread_attr_t *attr, _pthread_free(pt); return EINVAL; } - pt->attr.stackaddr = mmap(0, pt->attr.stacksize, PROT_READ | PROT_WRITE, - MAP_STACK | MAP_ANONYMOUS, -1, 0); + if (pt->attr.guardsize == PAGESIZE) { + // user is wisely using smaller stacks with default guard size + pt->attr.stackaddr = mmap(0, pt->attr.stacksize, PROT_READ | PROT_WRITE, + MAP_STACK | MAP_ANONYMOUS, -1, 0); + } else { + // user is tuning things, performance may suffer + pt->attr.stackaddr = mmap(0, pt->attr.stacksize, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + if (pt->attr.stackaddr != MAP_FAILED) { + if (IsOpenbsd() && + __sys_mmap( + pt->attr.stackaddr, pt->attr.stacksize, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_FIXED | MAP_ANON_OPENBSD | MAP_STACK_OPENBSD, + -1, 0, 0) != pt->attr.stackaddr) { + notpossible; + } + if (pt->attr.guardsize && !IsWindows() && + mprotect(pt->attr.stackaddr, pt->attr.guardsize, PROT_NONE)) { + notpossible; + } + } + } if (pt->attr.stackaddr == MAP_FAILED) { rc = errno; _pthread_free(pt); @@ -227,27 +247,8 @@ int pthread_create(pthread_t *thread, const pthread_attr_t *attr, return EAGAIN; } } - // mmap(MAP_STACK) creates a 4096 guard by default - if (pt->attr.guardsize != PAGESIZE) { - if (pt->attr.guardsize) { - // user requested special guard size - rc = mprotect(pt->attr.stackaddr, pt->attr.guardsize, PROT_NONE); - } else { - // user explicitly disabled guard page - rc = mprotect(pt->attr.stackaddr, PAGESIZE, PROT_READ | PROT_WRITE); - } - if (rc) { - notpossible; - } - } - if (IsAsan()) { - if (pt->attr.guardsize) { - __asan_poison(pt->attr.stackaddr, pt->attr.guardsize, - kAsanStackOverflow); - } - __asan_poison((char *)pt->attr.stackaddr + pt->attr.stacksize - - 16 /* openbsd:stackbound */, - 16, kAsanStackOverflow); + if (IsAsan() && pt->attr.guardsize) { + __asan_poison(pt->attr.stackaddr, pt->attr.guardsize, kAsanStackOverflow); } } @@ -267,7 +268,7 @@ int pthread_create(pthread_t *thread, const pthread_attr_t *attr, // launch PosixThread(pt) in new thread if (clone(PosixThread, pt->attr.stackaddr, - pt->attr.stacksize - 16 /* openbsd:stackbound */, + pt->attr.stacksize - (IsOpenbsd() ? 16 : 0), CLONE_VM | CLONE_THREAD | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_SETTLS | CLONE_PARENT_SETTID | CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID, diff --git a/test/libc/thread/pthread_create_test.c b/test/libc/thread/pthread_create_test.c index 1e10d99ce..15e6259e4 100644 --- a/test/libc/thread/pthread_create_test.c +++ b/test/libc/thread/pthread_create_test.c @@ -17,6 +17,7 @@ │ PERFORMANCE OF THIS SOFTWARE. │ ╚─────────────────────────────────────────────────────────────────────────────*/ #include "libc/calls/calls.h" +#include "libc/calls/struct/sigaction.h" #include "libc/dce.h" #include "libc/intrin/kprintf.h" #include "libc/intrin/pthread.h" @@ -25,13 +26,31 @@ #include "libc/runtime/runtime.h" #include "libc/runtime/stack.h" #include "libc/sysv/consts/prot.h" +#include "libc/sysv/consts/sa.h" +#include "libc/sysv/consts/sig.h" #include "libc/testlib/ezbench.h" #include "libc/testlib/subprocess.h" #include "libc/testlib/testlib.h" #include "libc/thread/thread.h" +void OnTrap(int sig, struct siginfo *si, void *vctx) { + struct ucontext *ctx = vctx; +} + +void SetUp(void) { + struct sigaction sig = {.sa_sigaction = OnTrap, .sa_flags = SA_SIGINFO}; + sigaction(SIGTRAP, &sig, 0); +} + +void TriggerSignal(void) { + sched_yield(); + DebugBreak(); + sched_yield(); +} + static void *Increment(void *arg) { ASSERT_EQ(gettid(), pthread_getthreadid_np()); + TriggerSignal(); return (void *)((uintptr_t)arg + 1); } @@ -44,6 +63,7 @@ TEST(pthread_create, testCreateReturnJoin) { } static void *IncExit(void *arg) { + TriggerSignal(); pthread_exit((void *)((uintptr_t)arg + 1)); } @@ -71,6 +91,7 @@ TEST(pthread_detach, testDetachUponCreation) { static void *CheckStack(void *arg) { char buf[1024 * 1024]; + TriggerSignal(); CheckLargeStackAllocation(buf, 1024 * 1024); return 0; } @@ -85,6 +106,24 @@ TEST(pthread_detach, testBigStack) { ASSERT_EQ(0, pthread_join(id, 0)); } +static void *CheckStack2(void *arg) { + char buf[57244]; + TriggerSignal(); + CheckLargeStackAllocation(buf, sizeof(buf)); + return 0; +} + +TEST(pthread_detach, testBiggerGuardSize) { + pthread_t id; + pthread_attr_t attr; + ASSERT_EQ(0, pthread_attr_init(&attr)); + ASSERT_EQ(0, pthread_attr_setstacksize(&attr, 65536)); + ASSERT_EQ(0, pthread_attr_setguardsize(&attr, 8192)); + ASSERT_EQ(0, pthread_create(&id, &attr, CheckStack2, 0)); + ASSERT_EQ(0, pthread_attr_destroy(&attr)); + ASSERT_EQ(0, pthread_join(id, 0)); +} + TEST(pthread_detach, testCustomStack_withReallySmallSize) { char *stk; size_t siz; diff --git a/third_party/libcxx/thread.cc b/third_party/libcxx/thread.cc index 6e026f8dd..c621bb48c 100644 --- a/third_party/libcxx/thread.cc +++ b/third_party/libcxx/thread.cc @@ -52,10 +52,6 @@ #include "third_party/getopt/getopt.h" #endif // defined(__unix__) || (defined(__APPLE__) && defined(__MACH__)) || defined(__CloudABI__) || defined(__Fuchsia__) || defined(__wasi__) -#if defined(__NetBSD__) -#pragma weak pthread_create // Do not create libpthread dependency -#endif - #if defined(_LIBCPP_WIN32API) #include "libc/nt/accounting.h" #include "libc/nt/automation.h" diff --git a/tool/build/lib/xlat.c b/tool/build/lib/xlat.c index d5f81f6b9..4afb2b22d 100644 --- a/tool/build/lib/xlat.c +++ b/tool/build/lib/xlat.c @@ -372,7 +372,6 @@ int XlatMapFlags(int x) { if (x & 2) r |= MAP_PRIVATE; if (x & 16) r |= MAP_FIXED; if (x & 32) r |= MAP_ANONYMOUS; - if (x & 256) r |= MAP_GROWSDOWN; return r; }