[PATCH] BZ 6973, 6976: Correct stack size and guard sizecomputations for new threads

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

Hi,

The POSIX specification requires that the stack allocated by the
implementation be at least equal to the size specified by the user in
pthread_attr_setstacksize. NPTL however gets the guard from within the
stack size, hence reducing the size. The specification says that
additional memory should be allocated at the overflow end of the stack:

"If a threadâs stack is created with guard protection, the
implementation allocates extra memory at the overflow end of the stack
as a buffer against stack overflow of the stack pointer."

Further, we currently align the stack size *down* instead of *up* to
__static_tls_align, again leading to the problem of allocating less
than the user request.

The implementation also has subtle problems like lying to the user
about the size of the guard size that is being used in an (inconsistent)
attempt to only give the user the amount of memory requested. This
should also be fixed.

The proposed patch allocates extra memory for guardsize and makes sure
that stack size is always greater than equal to the requested size. I
have also added a test case to verify this. I have verified that the
patch does not cause any regressions in the testsuite on x86_64.

So the behaviour with stack allocation now should be:

1) The guardsize will always be equal to the requested guardsize
rounded up to page size
2) The stacksize will be greater than or equal to the requested
stacksize.

Regards,
Siddhesh

nptl/ChangeLog:

2012-05-22  Siddhesh Poyarekar  <siddhesh@redhat.com>

	[BZ #6973]
	[BZ #6976]
	* Makefile (tests): Add new test case target.
	* allocatestack.c (allocate_stack): Align request size upwards
	to __static_tls_align. Allocate additional space for guard.
	* descr.h (struct pthread): Mention that reported_guardsize is
	unused.
	* pthread_getattr_np.c (pthread_getattr_np): Use
	pthread->guardsize value to return guardsize. Return stacksize
	after reducing guardsize.
	* tst-attr-stack.c: New test case.

diff --git a/nptl/Makefile b/nptl/Makefile
index 2a36d01..ee44637 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -196,7 +196,7 @@ CFLAGS-pt-system.c = -fexceptions
 
 
 tests = tst-typesizes \
-	tst-attr1 tst-attr2 tst-attr3 \
+	tst-attr1 tst-attr2 tst-attr3 tst-attr-stack \
 	tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 tst-mutex5 tst-mutex6 \
 	tst-mutex7 tst-mutex8 tst-mutex9 tst-mutex5a tst-mutex7a \
 	tst-mutexpi1 tst-mutexpi2 tst-mutexpi3 tst-mutexpi4 tst-mutexpi5 \
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 79c4531..e4da008 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -463,8 +463,8 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	size += pagesize_m1 + 1;
 #endif
 
-      /* Adjust the stack size for alignment.  */
-      size &= ~__static_tls_align_m1;
+      /* Adjust the stack size upwards for alignment.  */
+      size = (size + __static_tls_align_m1) & ~__static_tls_align_m1;
       assert (size != 0);
 
       /* Make sure the size of the stack is enough for the guard and
@@ -478,8 +478,8 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	return EINVAL;
 
       /* Try to get a stack from the cache.  */
-      reqsize = size;
-      pd = get_cached_stack (&size, &mem);
+      reqsize = size + guardsize;
+      pd = get_cached_stack (&reqsize, &mem);
       if (pd == NULL)
 	{
 	  /* To avoid aliasing effects on a larger scale than pages we
@@ -491,7 +491,9 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	    size += pagesize_m1 + 1;
 #endif
 
-	  mem = mmap (NULL, size, prot,
+	  /* Allocate enough memory for the minimum required size and the
+	     stack guard.  */
+	  mem = mmap (NULL, reqsize, prot,
 		      MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
 
 	  if (__builtin_expect (mem == MAP_FAILED, 0))
@@ -526,9 +528,9 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 
 	  /* Place the thread descriptor at the end of the stack.  */
 #if TLS_TCB_AT_TP
-	  pd = (struct pthread *) ((char *) mem + size - coloring) - 1;
+	  pd = (struct pthread *) ((char *) mem + reqsize - coloring) - 1;
 #elif TLS_DTV_AT_TP
-	  pd = (struct pthread *) ((((uintptr_t) mem + size - coloring
+	  pd = (struct pthread *) ((((uintptr_t) mem + reqsize - coloring
 				    - __static_tls_size)
 				    & ~__static_tls_align_m1)
 				   - TLS_PRE_TCB_SIZE);
@@ -536,7 +538,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 
 	  /* Remember the stack-related values.  */
 	  pd->stackblock = mem;
-	  pd->stackblock_size = size;
+	  pd->stackblock_size = reqsize;
 
 	  /* We allocated the first block thread-specific data array.
 	     This address will not change for the lifetime of this
@@ -573,7 +575,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	      assert (errno == ENOMEM);
 
 	      /* Free the stack memory we just allocated.  */
-	      (void) munmap (mem, size);
+	      (void) munmap (mem, reqsize);
 
 	      return errno;
 	    }
@@ -603,7 +605,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 	      if (err != 0)
 		{
 		  /* Free the stack memory we just allocated.  */
-		  (void) munmap (mem, size);
+		  (void) munmap (mem, reqsize);
 
 		  return err;
 		}
@@ -622,7 +624,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
       if (__builtin_expect (guardsize > pd->guardsize, 0))
 	{
 #ifdef NEED_SEPARATE_REGISTER_STACK
-	  char *guard = mem + (((size - guardsize) / 2) & ~pagesize_m1);
+	  char *guard = mem + (((reqsize - guardsize) / 2) & ~pagesize_m1);
 #elif _STACK_GROWS_DOWN
 	  char *guard = mem;
 # elif _STACK_GROWS_UP
@@ -646,21 +648,19 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 		 of memory caused problems we better do not use it
 		 anymore.  Uh, and we ignore possible errors.  There
 		 is nothing we could do.  */
-	      (void) munmap (mem, size);
+	      (void) munmap (mem, reqsize);
 
 	      return errno;
 	    }
-
-	  pd->guardsize = guardsize;
 	}
-      else if (__builtin_expect (pd->guardsize - guardsize > size - reqsize,
-				 0))
+      else if (__builtin_expect (guardsize < pd->guardsize, 0))
 	{
-	  /* The old guard area is too large.  */
+	  /* The old guard area is larger.  */
 
 #ifdef NEED_SEPARATE_REGISTER_STACK
-	  char *guard = mem + (((size - guardsize) / 2) & ~pagesize_m1);
-	  char *oldguard = mem + (((size - pd->guardsize) / 2) & ~pagesize_m1);
+	  char *guard = mem + (((reqsize - guardsize) / 2) & ~pagesize_m1);
+	  char *oldguard = mem + (((reqsize - pd->guardsize) / 2)
+				  & ~pagesize_m1);
 
 	  if (oldguard < guard
 	      && mprotect (oldguard, guard - oldguard, prot) != 0)
@@ -679,13 +679,8 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
 			pd->guardsize - guardsize, prot) != 0)
 	    goto mprot_error;
 #endif
-
-	  pd->guardsize = guardsize;
 	}
-      /* The pthread_getattr_np() calls need to get passed the size
-	 requested in the attribute, regardless of how large the
-	 actually used guardsize is.  */
-      pd->reported_guardsize = guardsize;
+      pd->guardsize = guardsize;
     }
 
   /* Initialize the lock.  We have to do this unconditionally since the
diff --git a/nptl/descr.h b/nptl/descr.h
index 60d2d22..143530e 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -370,9 +370,9 @@ struct pthread
      size.  */
   void *stackblock;
   size_t stackblock_size;
-  /* Size of the included guard area.  */
+  /* Size of the guard area.  */
   size_t guardsize;
-  /* This is what the user specified and what we will report.  */
+  /* Unused.  Retained only to maintain offsets.  */
   size_t reported_guardsize;
 
   /* Thread Priority Protection data.  */
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index f1268dd..2aecb0a 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -53,14 +53,13 @@ pthread_getattr_np (thread_id, attr)
   if (IS_DETACHED (thread))
     iattr->flags |= ATTR_FLAG_DETACHSTATE;
 
-  /* This is the guardsize after adjusting it.  */
-  iattr->guardsize = thread->reported_guardsize;
+  iattr->guardsize = thread->guardsize;
 
   /* The sizes are subject to alignment.  */
   if (__builtin_expect (thread->stackblock != NULL, 1))
     {
-      iattr->stacksize = thread->stackblock_size;
-      iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize;
+      iattr->stacksize = thread->stackblock_size - thread->guardsize;
+      iattr->stackaddr = (char *) thread->stackblock + thread->stackblock_size;
     }
   else
     {
diff --git a/nptl/tst-attr-stack.c b/nptl/tst-attr-stack.c
new file mode 100644
index 0000000..d25d566
--- /dev/null
+++ b/nptl/tst-attr-stack.c
@@ -0,0 +1,126 @@
+/* Copyright (C) 2012 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+/* Unaligned on purpose to verify that pthread_create aligns the stack size
+   upwards instead of truncating it.  */
+#define STACKSIZE (1024 * 1024 * 4 + 1)
+#define GUARDSIZE 1024 * 8
+
+void *thr (void *arg)
+{
+  size_t stacksize, guardsize;
+  void *stackaddr;
+  pthread_attr_t attr;
+  int *retval = arg;
+  int ret;
+
+  pthread_t t = pthread_self ();
+
+  if ((ret = pthread_getattr_np (t, &attr)))
+    {
+      printf ("pthread_getattr_np failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  if ((ret = pthread_attr_getstack (&attr, &stackaddr, &stacksize)))
+    {
+      printf ("pthread_attr_getstack failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  if ((ret = pthread_attr_getguardsize (&attr, &guardsize)))
+    {
+      printf ("pthread_attr_getguardsize failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  *retval = 0;
+
+  if (stacksize < STACKSIZE)
+    {
+      printf ("Minimum stack size reduced: "
+	      "expected greater than %zd but got %zd\n", STACKSIZE, stacksize);
+      *retval = 1;
+    }
+  else
+    printf ("Stack size aligned up correctly: "
+	    "expected greater than %zd and got %zd\n", STACKSIZE, stacksize);
+
+  /* We get a segfault if this fails.  */
+  printf ("I should be able to access %p to see %x\n", stackaddr,
+	  *(unsigned long *)stackaddr);
+
+  if (guardsize != GUARDSIZE)
+    {
+      printf ("Guard size mismatch: Expected %zd but got %zd\n", GUARDSIZE,
+	      guardsize);
+      *retval = 1;
+    }
+
+  return retval;
+}
+
+static int
+do_test (void)
+{
+  pthread_t t;
+  pthread_attr_t attr;
+  int *retvalp;
+  int retval;
+  int ret;
+
+  if ((ret = pthread_attr_init (&attr)))
+    {
+      printf ("pthread_attr_init failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  if ((ret = pthread_attr_setstacksize (&attr, STACKSIZE)))
+    {
+      printf ("pthread_attr_setstacksize failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  if ((pthread_attr_setguardsize (&attr, GUARDSIZE)))
+    {
+      printf ("pthread_attr_setguardsize failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  if ((ret = pthread_create (&t, &attr, thr, &retval)))
+    {
+      printf ("pthread_create failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  if ((ret = pthread_join (t, (void **)&retvalp)))
+    {
+      printf ("pthread_join failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  return *retvalp;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"