am 67750c85: Merge "Fix pthread_getattr_np, pthread_attr_setguardsize, and pthread_attr_setstacksize."

* commit '67750c8515573cba294b2f3840249dd7f7d817d5':
  Fix pthread_getattr_np, pthread_attr_setguardsize, and pthread_attr_setstacksize.

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;
 }

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, &param) == -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));

tests/pthread_test.cpp

@@ -17,6 +17,7 @@
 #include <gtest/gtest.h>
 
 #include <errno.h>
+#include <limits.h>
 #include <pthread.h>
 #include <unistd.h>
 
@@ -339,3 +340,95 @@ TEST(pthread, pthread_join__multijoin) {
   ASSERT_EQ(0, pthread_join(t2, &join_result));
   ASSERT_EQ(0, reinterpret_cast<int>(join_result));
 }
+
+static void* GetActualGuardSizeFn(void* arg) {
+  pthread_attr_t attributes;
+  pthread_getattr_np(pthread_self(), &attributes);
+  pthread_attr_getguardsize(&attributes, reinterpret_cast<size_t*>(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<size_t*>(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
+}