This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/2][BZ #12416] Use stack boundaries from /proc/PID/mapsto make stack executable
On Tue, 15 May 2012 09:30:33 -0700 (PDT), Roland wrote:
> Do you mean just below the stack (lower address)? If it's actually
> the dlopen'd module, then that is a file mapping rather than an
> anonymous mapping like the stack. You can distinguish those in
> /proc/self/maps (the fourth column is 00:00 for anonymous mappings and
> not that for file mappings). So pthread_getattr_np need never be
> confused by that case.
Yes I meant below, sorry about that.
I interpreted the pthread_getattr_np logic to consider the end of
previous vma for stack size to mean that we're looking at the potential
for the stack to grow, subject to rlimit. Whether or not the mapping
above is anonymous should not matter in that case. If we consider that
the file mapping happens below the stack, the potential for growth of
the stack changes to the end of that mapping and if rlimit is high
enough, it will change the stacksize and stackaddr returned.
I've attached an updated patch with the rest of the changes. If we
don't want to account for a too high rlimit (that is a pre-requisite
for this situation to occur) then I can change the check to individual
verification of stack size and stack address and send an updated patch.
Thanks,
Siddhesh
ChangeLog:
2012-05-16 Siddhesh Poyarekar <siddhesh@redhat.com>
* elf/tst-execstack.c: Include stackinfo.h.
(do_test): Adjust test case to ensure that pthread_getattr_np
behaviour remains the same after marking stack executable.
nptl/ChangeLog:
2012-05-16 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..02cc270 100644
--- a/elf/tst-execstack.c
+++ b/elf/tst-execstack.c
@@ -7,6 +7,7 @@
#include <string.h>
#include <unistd.h>
#include <error.h>
+#include <stackinfo.h>
static void
print_maps (void)
@@ -46,7 +47,6 @@ waiter_thread (void *arg)
}
#endif
-
static bool allow_execstack = true;
@@ -107,6 +107,35 @@ 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 ("before execstack: pthread_getattr_np returned error: %s\n",
+ strerror (ret));
+ return 1;
+ }
+
+ ret = pthread_attr_getstack (&attr, &old_stack_addr, &stack_size);
+ if (ret)
+ {
+ printf ("before execstack: pthread_attr_getstack returned error: %s\n",
+ strerror (ret));
+ return 1;
+ }
+# if _STACK_GROWS_DOWN
+ old_stack_addr += stack_size;
+# else
+ old_stack_addr -= stack_size;
+# endif
+#endif
+
/* Loading this module should force stacks to become executable. */
void *h = dlopen ("tst-execstack-mod.so", RTLD_LAZY);
if (h == NULL)
@@ -129,6 +158,46 @@ do_test (void)
print_maps ();
+#if USE_PTHREADS
+ ret = pthread_getattr_np (me, &attr);
+ if (ret)
+ {
+ printf ("after execstack: pthread_getattr_np returned error: %s\n",
+ strerror (ret));
+ return 1;
+ }
+
+ ret = pthread_attr_getstack (&attr, &new_stack_addr, &stack_size);
+ if (ret)
+ {
+ printf ("after execstack: pthread_attr_getstack returned error: %s\n",
+ strerror (ret));
+ return 1;
+ }
+
+# if _STACK_GROWS_DOWN
+ new_stack_addr += stack_size;
+# else
+ new_stack_addr -= stack_size;
+# endif
+
+ /* It is possible that the dlopen'd module may have been mmapped just below
+ the stack. The stack size is taken as MIN(stack rlimit size, end of last
+ vma) in pthread_getattr_np. If rlimit is set high enough, it is possible
+ that the size may have changed. A subsequent call to
+ pthread_attr_getstack returns the size and (bottom - size) as the
+ stacksize and stackaddr respectively. If the size changes due to the
+ above, then both stacksize and stackaddr can change, but the stack bottom
+ should remain the same, which is computed as stackaddr + stacksize. */
+ 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..7f9a514 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -84,6 +84,18 @@ 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 beyond
+ 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 *stack_end = (void *) ((uintptr_t) __libc_stack_end
+ & -(uintptr_t) GLRO(dl_pagesize));
+#if _STACK_GROWS_DOWN
+ stack_end += GLRO(dl_pagesize);
+#endif
/* We need no locking. */
__fsetlocking (fp, FSETLOCKING_BYCALLER);
@@ -109,7 +121,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 = stack_end;
/* The limit might be too high. */
if ((size_t) iattr->stacksize
--
1.7.7.6