diff --git a/libc/bionic/pthread_attr.cpp b/libc/bionic/pthread_attr.cpp index c47f95e22..fe1ed4a98 100644 --- a/libc/bionic/pthread_attr.cpp +++ b/libc/bionic/pthread_attr.cpp @@ -84,7 +84,7 @@ int pthread_attr_getschedparam(pthread_attr_t const* attr, struct sched_param* p } int pthread_attr_setstacksize(pthread_attr_t* attr, size_t stack_size) { - if ((stack_size & (PAGE_SIZE - 1) || stack_size < PTHREAD_STACK_MIN)) { + if (stack_size < PTHREAD_STACK_MIN) { return EINVAL; } attr->stack_size = stack_size; @@ -128,9 +128,6 @@ int pthread_attr_getstack(pthread_attr_t const* attr, void** stack_base, size_t* } int pthread_attr_setguardsize(pthread_attr_t* attr, size_t guard_size) { - if (guard_size & (PAGE_SIZE - 1) || guard_size < PAGE_SIZE) { - return EINVAL; - } attr->guard_size = guard_size; return 0; } diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp index 19009e75e..f45f5e769 100644 --- a/libc/bionic/pthread_create.cpp +++ b/libc/bionic/pthread_create.cpp @@ -104,8 +104,8 @@ int _init_thread(pthread_internal_t* thread, bool add_to_thread_list) { if (sched_setscheduler(thread->tid, thread->attr.sched_policy, ¶m) == -1) { // For backwards compatibility reasons, we just warn about failures here. // error = errno; - const char* msg = "pthread_create sched_setscheduler call failed: %s\n"; - __libc_format_log(ANDROID_LOG_WARN, "libc", msg, strerror(errno)); + __libc_format_log(ANDROID_LOG_WARN, "libc", + "pthread_create sched_setscheduler call failed: %s", strerror(errno)); } } @@ -119,20 +119,27 @@ int _init_thread(pthread_internal_t* thread, bool add_to_thread_list) { return error; } -static void* __create_thread_stack(size_t stack_size, size_t guard_size) { +static void* __create_thread_stack(pthread_internal_t* thread) { ScopedPthreadMutexLocker lock(&gPthreadStackCreationLock); // Create a new private anonymous map. int prot = PROT_READ | PROT_WRITE; int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE; - void* stack = mmap(NULL, stack_size, prot, flags, -1, 0); + void* stack = mmap(NULL, thread->attr.stack_size, prot, flags, -1, 0); if (stack == MAP_FAILED) { + __libc_format_log(ANDROID_LOG_WARN, + "libc", + "pthread_create failed: couldn't allocate %zd-byte stack: %s", + thread->attr.stack_size, strerror(errno)); return NULL; } // Set the guard region at the end of the stack to PROT_NONE. - if (mprotect(stack, guard_size, PROT_NONE) == -1) { - munmap(stack, stack_size); + if (mprotect(stack, thread->attr.guard_size, PROT_NONE) == -1) { + __libc_format_log(ANDROID_LOG_WARN, "libc", + "pthread_create failed: couldn't mprotect PROT_NONE %zd-byte stack guard region: %s", + thread->attr.guard_size, strerror(errno)); + munmap(stack, thread->attr.stack_size); return NULL; } @@ -164,15 +171,15 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr, attr = NULL; // Prevent misuse below. } - // Make sure the stack size is PAGE_SIZE aligned. - size_t stack_size = (thread->attr.stack_size + (PAGE_SIZE-1)) & ~(PAGE_SIZE-1); + // Make sure the stack size and guard size are multiples of PAGE_SIZE. + thread->attr.stack_size = (thread->attr.stack_size + (PAGE_SIZE-1)) & ~(PAGE_SIZE-1); + thread->attr.guard_size = (thread->attr.guard_size + (PAGE_SIZE-1)) & ~(PAGE_SIZE-1); if (thread->attr.stack_base == NULL) { // The caller didn't provide a stack, so allocate one. - thread->attr.stack_base = __create_thread_stack(stack_size, thread->attr.guard_size); + thread->attr.stack_base = __create_thread_stack(thread); if (thread->attr.stack_base == NULL) { free(thread); - __libc_format_log(ANDROID_LOG_WARN, "libc", "pthread_create failed: couldn't allocate %zd-byte stack", stack_size); return EAGAIN; } } else { @@ -181,7 +188,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr, } // Make room for TLS. - void** tls = (void**)((uint8_t*)(thread->attr.stack_base) + stack_size - BIONIC_TLS_SLOTS * sizeof(void*)); + void** tls = (void**)((uint8_t*)(thread->attr.stack_base) + thread->attr.stack_size - BIONIC_TLS_SLOTS * sizeof(void*)); // Create a mutex for the thread in TLS_SLOT_SELF to wait on once it starts so we can keep // it from doing anything until after we notify the debugger about it @@ -201,7 +208,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr, if (tid < 0) { int clone_errno = errno; if ((thread->attr.flags & PTHREAD_ATTR_FLAG_USER_STACK) == 0) { - munmap(thread->attr.stack_base, stack_size); + munmap(thread->attr.stack_base, thread->attr.stack_size); } free(thread); __libc_format_log(ANDROID_LOG_WARN, "libc", "pthread_create failed: clone failed: %s", strerror(errno)); diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp index d754312f9..c7dbdc78c 100644 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -339,3 +340,95 @@ TEST(pthread, pthread_join__multijoin) { ASSERT_EQ(0, pthread_join(t2, &join_result)); ASSERT_EQ(0, reinterpret_cast(join_result)); } + +static void* GetActualGuardSizeFn(void* arg) { + pthread_attr_t attributes; + pthread_getattr_np(pthread_self(), &attributes); + pthread_attr_getguardsize(&attributes, reinterpret_cast(arg)); + return NULL; +} + +static size_t GetActualGuardSize(const pthread_attr_t& attributes) { + size_t result; + pthread_t t; + pthread_create(&t, &attributes, GetActualGuardSizeFn, &result); + void* join_result; + pthread_join(t, &join_result); + return result; +} + +static void* GetActualStackSizeFn(void* arg) { + pthread_attr_t attributes; + pthread_getattr_np(pthread_self(), &attributes); + pthread_attr_getstacksize(&attributes, reinterpret_cast(arg)); + return NULL; +} + +static size_t GetActualStackSize(const pthread_attr_t& attributes) { + size_t result; + pthread_t t; + pthread_create(&t, &attributes, GetActualStackSizeFn, &result); + void* join_result; + pthread_join(t, &join_result); + return result; +} + +TEST(pthread, pthread_attr_setguardsize) { + pthread_attr_t attributes; + ASSERT_EQ(0, pthread_attr_init(&attributes)); + + // Get the default guard size. + size_t default_guard_size; + ASSERT_EQ(0, pthread_attr_getguardsize(&attributes, &default_guard_size)); + + // No such thing as too small: will be rounded up to one page by pthread_create. + ASSERT_EQ(0, pthread_attr_setguardsize(&attributes, 128)); + size_t guard_size; + ASSERT_EQ(0, pthread_attr_getguardsize(&attributes, &guard_size)); + ASSERT_EQ(128U, guard_size); + ASSERT_EQ(4096U, GetActualGuardSize(attributes)); + + // Large enough and a multiple of the page size. + ASSERT_EQ(0, pthread_attr_setguardsize(&attributes, 32*1024)); + ASSERT_EQ(0, pthread_attr_getguardsize(&attributes, &guard_size)); + ASSERT_EQ(32*1024U, guard_size); + + // Large enough but not a multiple of the page size; will be rounded up by pthread_create. + ASSERT_EQ(0, pthread_attr_setguardsize(&attributes, 32*1024 + 1)); + ASSERT_EQ(0, pthread_attr_getguardsize(&attributes, &guard_size)); + ASSERT_EQ(32*1024U + 1, guard_size); +} + +TEST(pthread, pthread_attr_setstacksize) { + pthread_attr_t attributes; + ASSERT_EQ(0, pthread_attr_init(&attributes)); + + // Get the default stack size. + size_t default_stack_size; + ASSERT_EQ(0, pthread_attr_getstacksize(&attributes, &default_stack_size)); + + // Too small. + ASSERT_EQ(EINVAL, pthread_attr_setstacksize(&attributes, 128)); + size_t stack_size; + ASSERT_EQ(0, pthread_attr_getstacksize(&attributes, &stack_size)); + ASSERT_EQ(default_stack_size, stack_size); + ASSERT_GE(GetActualStackSize(attributes), default_stack_size); + + // Large enough and a multiple of the page size. + ASSERT_EQ(0, pthread_attr_setstacksize(&attributes, 32*1024)); + ASSERT_EQ(0, pthread_attr_getstacksize(&attributes, &stack_size)); + ASSERT_EQ(32*1024U, stack_size); + ASSERT_EQ(GetActualStackSize(attributes), 32*1024U); + + // Large enough but not a multiple of the page size; will be rounded up by pthread_create. + ASSERT_EQ(0, pthread_attr_setstacksize(&attributes, 32*1024 + 1)); + ASSERT_EQ(0, pthread_attr_getstacksize(&attributes, &stack_size)); + ASSERT_EQ(32*1024U + 1, stack_size); +#if __BIONIC__ + // Bionic rounds up, which is what POSIX allows. + ASSERT_EQ(GetActualStackSize(attributes), (32 + 4)*1024U); +#else + // glibc rounds down, in violation of POSIX. They document this in their BUGS section. + ASSERT_EQ(GetActualStackSize(attributes), 32*1024U); +#endif +}