This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/2][BZ #12416] Use stack boundaries from /proc/PID/mapsto make stack executable


On Mon,  7 May 2012 13:02:21 -0700 (PDT), Roland wrote:
> What "kernel stack accounting" are you talking about?  The [...]
> monikers for /proc/PID/maps output are just meant to be vaguely
> informative in a minimal-best-effort sort of way and should not be
> construed as signs of anything deeper AFAIK.  If you think there is
> some concrete sense in which the kernel purports to grok what is and
> isn't part of your stack and let that affect its behavior in any
> particular way, be explicit about what you mean.
> 

I was talking about mm->vm_stack that accounts for all vmas allocated
with MAP_GROWS*. But I'm wrong anyway, since that accounting is not
affected by vma splits, since the vmas still remain MAP_GROWS*.

So I propose the following simpler patch instead in the interest of
marking as less of the stack as possible as executable. The patch
modifies pthread_getattr_np to use the page end containing
__libc_stack_end as the end of stack and hence return stack_size and
stack_end + stack_size as the size and address of stack respectively.

One other thing I realized when I wrote the test case is that
__pthread_attr_getstackaddr simply subtracts stacksize from stackaddr
and returns it, which is wrong for architectures with _STACK_GROWS_UP.
In fact, pthread_getattr_np also probably has a similar problem. I am
currently surrounded by x86 boxes, so I'll see if I find myself an ia64
box to confirm this. I'll post patches for those problems separately if
they're applicable.

Regards,
Siddhesh


ChangeLog:
2012-05-08  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* elf/tst-execstack.c (do_test): Adjust test case to ensure that
	pthread_getattr_np behaviour remains the same after marking
	stack executable.

nptl/ChangeLog
2012-05-08  Siddhesh Poyarekar  <siddhesh@redhat.com>

	* nptl/pthread_getattr_np.c (pthread_getattr_np): Use
	__libc_stack_end rounded to the end of containing page as the
	real stack end.
diff --git a/elf/tst-execstack.c b/elf/tst-execstack.c
index 6632e53..c3479ee 100644
--- a/elf/tst-execstack.c
+++ b/elf/tst-execstack.c
@@ -46,6 +46,17 @@ waiter_thread (void *arg)
 }
 #endif
 
+static bool
+stack_grows_down (const bool arg)
+{
+  bool local;
+
+  if (__builtin_frame_address (0) > __builtin_frame_address (1))
+    return true;
+  else {
+    return false;
+  }
+}
 
 static bool allow_execstack = true;
 
@@ -107,6 +118,32 @@ do_test (void)
 
   print_maps ();
 
+#if USE_PTHREADS
+  void *old_stack_addr, *new_stack_addr;
+  size_t stack_size;
+  pthread_t me = pthread_self ();
+  pthread_attr_t attr;
+  int ret = 0;
+
+  ret = pthread_getattr_np (me, &attr);
+  if (ret)
+    {
+      printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+      return 1;
+    }
+
+  ret = pthread_attr_getstack (&attr, &old_stack_addr, &stack_size);
+  if (ret)
+    {
+      printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+      return 1;
+    }
+  if (stack_grows_down(stack_size))
+    old_stack_addr += stack_size;
+  else
+    old_stack_addr -= stack_size;
+#endif
+
   /* Loading this module should force stacks to become executable.  */
   void *h = dlopen ("tst-execstack-mod.so", RTLD_LAZY);
   if (h == NULL)
@@ -129,6 +166,39 @@ do_test (void)
 
   print_maps ();
 
+#if USE_PTHREADS
+  ret = pthread_getattr_np (me, &attr);
+  if (ret)
+    {
+      printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+      return 1;
+    }
+
+  ret = pthread_attr_getstack (&attr, &new_stack_addr, &stack_size);
+  if (ret)
+    {
+      printf ("pthread_getattr_np returned error: %s\n", strerror (ret));
+      return 1;
+    }
+
+  if (stack_grows_down(stack_size))
+    new_stack_addr += stack_size;
+  else
+    new_stack_addr -= stack_size;
+
+  /* Make sure that the stack end address did not change.  We do not check the
+     stack top and size separately because we evaluate the stack top as
+     stack_end + size and as a result of that, both may change when the vma
+     above the stack changes.  */
+  if (old_stack_addr != new_stack_addr)
+    {
+      printf ("Stack end changed, old: %p, new: %p\n",
+	      old_stack_addr, new_stack_addr);
+      return 1;
+    }
+  printf ("Stack address remains the same: %p\n", old_stack_addr);
+#endif
+
   /* Test that growing the stack region gets new executable pages too.  */
   deeper ((void (*) (void)) f);
 
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index f1268dd..742bc4f 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -84,6 +84,20 @@ pthread_getattr_np (thread_id, attr)
 	    ret = errno;
 	  else
 	    {
+	      /* We consider the main process stack to have ended with
+	         the page containing __libc_stack_end.  There is stuff below
+		 it in the stack too, like the program arguments, environment
+		 variables and auxv info, but we ignore those pages when
+		 returning size so that the output is consistent when the
+		 stack is marked executable due to a loaded dso requiring
+		 it.  */
+	      void *real_stack_end;
+
+	      real_stack_end = (void *) ((uintptr_t) __libc_stack_end
+					 & -(uintptr_t) GLRO(dl_pagesize));
+#if _STACK_GROWS_DOWN
+	      real_stack_end += GLRO(dl_pagesize);
+#endif
 	      /* We need no locking.  */
 	      __fsetlocking (fp, FSETLOCKING_BYCALLER);
 
@@ -109,7 +123,7 @@ pthread_getattr_np (thread_id, attr)
 		    {
 		      /* Found the entry.  Now we have the info we need.  */
 		      iattr->stacksize = rl.rlim_cur;
-		      iattr->stackaddr = (void *) to;
+		      iattr->stackaddr = real_stack_end;
 
 		      /* The limit might be too high.  */
 		      if ((size_t) iattr->stacksize

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]