[PATCH] fix allocation of NPTL thread stack guard area

["Chris Demetriou" <cgd at google dot com>]

I was doing something the other day that required me to set my
threads' stack and guard sizes.  I was surprised to find that NPTL was
taking the guard area out of the requested stack size, rather than
tacking the guard area on to the end of the stack.

LinuxThreads allocated the guard area after the end of the requested
stack.  I checked the Solaris 10 pthreads implementation, it does the
same.

My reading of POSIX.1-2001 pthread_attr_setguardsize says that that
behavior is right:

31274 The guardsize attribute controls the size of the guard area for
the created thread's stack. The
31275 guardsize attribute provides protection against overflow of the
stack pointer. If a thread's stack is
31276 created with guard protection, the implementation allocates
extra memory at the overflow end
31277 of the stack as a buffer against stack overflow of the stack pointer.

(Note specifically "allocates *extra* memory.")  So, I'm thinking this
is a bug in NPTL.


I've attached a fix for this, along with a test case.  I'm a bit
surprised (glad, though! 8-) that this was so easy to fix.

I've also included output from the new test before the allocatestack.c
change, output from a run after the change, and output from the
Solaris run.

This was tested on a CentOS 5 system (uname -msrv -> Linux
2.6.18-53.1.13.el5xen #1 SMP Tue Feb 12 13:33:07 EST 2008 x86_64),
full glibc tests before/after for 64-bit.  It was also tested on
32-bit x86 using a ubuntu-based system + 32-bit headers.

The patch & test runs were made with glibc sources as of this evening.

(FYI, I've tried to make the test work on stack-grows-up systems, but
i don't have any way to test on them.)



chris
---
2008-05-19  Chris Demetriou  <cgd@google.com>

        * nptl/allocatestack.c (allocate_stack): Add guard page size to
        requested stack size, per POSIX.1-2001.
        * nptl/tst-stack4.c: New test for corrected guard page behavior.
        * nptl/Makefile (tests): Add new test.
        * nptl/TODO-testing: Remove note about need to test
        pthread_attr_setguardsize.

2008-05-19  Chris Demetriou  <cgd@google.com>

	* nptl/allocatestack.c (allocate_stack): Add guard page size to
	requested stack size, per POSIX.1-2001.
	* nptl/tst-stack4.c: New test for corrected guard page behavior.
	* nptl/Makefile (tests): Add new test.
	* nptl/TODO-testing: Remove note about need to test
	pthread_attr_setguardsize.

Index: nptl/Makefile
===================================================================
RCS file: /cvs/glibc/libc/nptl/Makefile,v
retrieving revision 1.193
diff -u -u -p -r1.193 Makefile
--- nptl/Makefile	1 Feb 2008 00:01:18 -0000	1.193
+++ nptl/Makefile	20 May 2008 04:53:07 -0000
@@ -242,7 +242,7 @@ tests = tst-typesizes \
 	tst-exec1 tst-exec2 tst-exec3 tst-exec4 \
 	tst-exit1 tst-exit2 tst-exit3 \
 	tst-stdio1 tst-stdio2 \
-	tst-stack1 tst-stack2 tst-stack3 \
+	tst-stack1 tst-stack2 tst-stack3 tst-stack4 \
 	tst-unload \
 	tst-dlsym1 \
 	tst-sysconf \
Index: nptl/TODO-testing
===================================================================
RCS file: /cvs/glibc/libc/nptl/TODO-testing,v
retrieving revision 1.1
diff -u -u -p -r1.1 TODO-testing
--- nptl/TODO-testing	15 Feb 2003 22:50:01 -0000	1.1
+++ nptl/TODO-testing	20 May 2008 05:23:24 -0000
@@ -1,7 +1,3 @@
-pthread_attr_setguardsize
-
-  test effectiveness
-
 pthread_attr_[sg]etschedparam
 
   what to test?
Index: nptl/allocatestack.c
===================================================================
RCS file: /cvs/glibc/libc/nptl/allocatestack.c,v
retrieving revision 1.80
diff -u -u -p -r1.80 allocatestack.c
--- nptl/allocatestack.c	14 Dec 2007 16:33:32 -0000	1.80
+++ nptl/allocatestack.c	20 May 2008 04:53:07 -0000
@@ -429,15 +429,20 @@ allocate_stack (const struct pthread_att
       size &= ~__static_tls_align_m1;
       assert (size != 0);
 
-      /* Make sure the size of the stack is enough for the guard and
-	 eventually the thread descriptor.  */
-      guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
-      if (__builtin_expect (size < ((guardsize + __static_tls_size
-				     + MINIMAL_REST_STACK + pagesize_m1)
+      /* Make sure the size of the stack is enough for the thread
+	 descriptor.  */
+      if (__builtin_expect (size < ((__static_tls_size + MINIMAL_REST_STACK
+				     + pagesize_m1)
 				    & ~pagesize_m1),
 			    0))
-	/* The stack is too small (or the guard too large).  */
+	/* The stack is too small.  */
+	return EINVAL;
+
+      /* Add the guard into the amount of memory we allocate.  */
+      guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
+      if (size + guardsize < size)
 	return EINVAL;
+      size += guardsize;
 
       /* Try to get a stack from the cache.  */
       reqsize = size;
Index: nptl/tst-stack4.c
===================================================================
RCS file: nptl/tst-stack4.c
diff -N nptl/tst-stack4.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ nptl/tst-stack4.c	20 May 2008 04:53:07 -0000
@@ -0,0 +1,218 @@
+/* Copyright (C) 2008 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Chris Demetriou <cgd@google.com>, 2008.
+
+   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, write to the Free
+   Software Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
+   02111-1307 USA.  */
+
+/* This test attempts to verify that thread stack and guard regions are
+   sized correctly (as requested by pthread_attr_setstacksize and
+   pthread_attr_setguardsize).  */
+    
+#include <pthread.h>
+#include <errno.h>
+#include <limits.h>
+#include <stackinfo.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+int pipe_fds[2];
+
+/* Check whether the given pointer points to memory that is readable
+   by writing the byte the it points to to a pipe.
+
+   We expect the write to return either success (if memory at that
+   address is readable) or EFAULT if not.  If any other error occurs,
+   something unexpected has gone wrong and we bail out.  */
+int
+pointer_readable (void *addr)
+{
+  int result = 1;
+  if (write (pipe_fds[1], addr, 1) == 1)
+    {
+      char ch;
+      if (read (pipe_fds[0], &ch, 1) != 1)
+        {
+          printf ("pipe read: %s\n", strerror (errno));
+          exit (1);
+        }
+    }
+  else if (errno == EFAULT)
+    {
+      result = 0;
+    }
+  else
+    {
+      printf ("pipe write: %s\n", strerror (errno));
+      exit (1);
+    }
+  return result;
+}
+
+/* Verify whether stack and guard sizes are sized correctly.  Returns
+   0 if successful, non-zero on failure.  */
+void *
+thread_func (void *arg)
+{
+  char *stack_addr = (char *) &stack_addr;
+  size_t requested_stack_size = ((int *) arg)[0];
+  size_t requested_guard_size = ((int *) arg)[1];
+  /* "Stuff" on the stack may have already consumed up to approximately
+     PTHREAD_STACK_MIN bytes of space.  */
+  size_t expected_stack_size = requested_stack_size - PTHREAD_STACK_MIN;
+  size_t expected_guard_size = requested_guard_size;
+  ssize_t stack_size, guard_size, stack_incr;
+  int bad = 0;
+
+#if _STACK_GROWS_DOWN
+  stack_incr = -1;
+#elif _STACK_GROWS_UP
+  stack_incr = 1;
+#else
+# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP"
+#endif
+
+  /* There may an arbitrary amount of extra stack, but in these tests
+     we assume that there will always be at least a bit of a guard.  */
+  stack_size = 0;
+  while (pointer_readable (stack_addr + stack_size))
+    stack_size += stack_incr;
+
+  guard_size = stack_incr;
+  while (! pointer_readable (stack_addr + stack_size + guard_size)
+         && (guard_size * stack_incr) < requested_guard_size)
+    guard_size += stack_incr;
+
+  /* normalize back to positive.  */
+  stack_size *= stack_incr;
+  guard_size *= stack_incr;
+
+  printf ("detected available stack size %#zx, guard size >= %#zx\n",
+          stack_size, guard_size);
+
+  if (stack_size >= expected_stack_size)
+    printf ("stack size OK (must be >= %#zx)\n", expected_stack_size);
+  else
+    {
+      printf ("stack size BAD (must be >= %#zx)\n", expected_stack_size);
+      bad = 1;
+    }
+
+  if (guard_size >= expected_guard_size)
+    printf ("guard size OK (must be >= %#zx)\n", expected_guard_size);
+  else
+    {
+      printf ("guard size BAD (must be >= %#zx)\n", expected_guard_size);
+      bad = 1;
+    }
+
+  return bad ? ((void *) (intptr_t) 1) : 0;
+}
+
+/* Test thread creation with the given stack and guard size.  Returns
+   zero if successful, non-zero on failure.  */
+int
+test_one_thread (size_t stacksize, size_t guardsize)
+{
+  size_t requested_sizes[2];
+  pthread_attr_t attr;
+  pthread_t thread;
+  void *thread_result;
+  int result;
+
+  puts ("");
+  printf ("testing thread with stack size = %#zx, guard size = %#zx\n",
+          stacksize, guardsize);
+
+  if (guardsize == 0)
+    {
+      puts ("guard size must be > 0");
+      return 1;
+    }
+
+  if ((result = pthread_attr_init (&attr)) != 0)
+    {
+      printf ("pthread_attr_init: %s\n", strerror (result));
+      return 1;
+    }
+
+  if ((result = pthread_attr_setstacksize (&attr, stacksize)) != 0)
+    {
+      printf ("pthread_attr_setstacksize: %s\n", strerror (result));
+      return 1;
+    }
+
+  if ((result = pthread_attr_setguardsize (&attr, guardsize)) != 0)
+    {
+      printf ("pthread_attr_setguardsize: %s\n", strerror (result));
+      return 1;
+    }
+
+  requested_sizes[0] = stacksize;
+  requested_sizes[1] = guardsize;
+  if ((result = pthread_create (&thread, &attr, thread_func,
+                               requested_sizes)) != 0)
+    {
+      printf ("pthread_create: %s\n", strerror (result));
+      return 1;
+    }
+
+  if ((result = pthread_join (thread, &thread_result)) != 0)
+    {
+      printf ("pthread_join: %s\n", strerror (result));
+      return 1;
+    }
+
+  return (thread_result != 0);
+}
+
+int
+do_test (void)
+{
+  int errs_seen = 0;
+  int page_size = getpagesize ();
+
+  puts ("testing pthread_attr_setguardsize guards.");
+
+  if (pipe (pipe_fds) == -1)
+    {
+      puts ("pipe init failed");
+      exit (1);
+    } 
+
+  errs_seen |= test_one_thread (256 * page_size, 1);
+  errs_seen |= test_one_thread (256 * page_size, page_size);
+  errs_seen |= test_one_thread (256 * page_size, 16 * page_size);
+  errs_seen |= test_one_thread (256 * page_size, 256 * page_size);
+
+  close (pipe_fds[0]);
+  close (pipe_fds[1]);
+
+  puts ("");
+  if (errs_seen)
+    {
+      puts ("TEST FAILED, see above for errors.");
+      exit (1);
+    }
+  puts ("TEST PASSED.");
+  return 0;
+}
+
+
+#define TIMEOUT 20  /* Takes 6.5 seconds on a 2.4GHz Intel Core 2 Duo.  */
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

Attachment: tst-stack4.out.BROKEN
Description: Binary data

Attachment: tst-stack4.out.FIXED
Description: Binary data

Attachment: tst-stack4.out.SOLARIS
Description: Binary data