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 v2] Fix stacksize returned for main process thread whenRLIMIT_STACK is relevant


On 6/19/2012 11:44 AM, Siddhesh Poyarekar wrote:
> On Tue, 19 Jun 2012 11:15:18 -0400, Carlos wrote:
>> > Please repost the final version of the patch.
> Attached. The details were in the original thread, but I should have
> summarized them during submission, so here goes:
> 
>> > Please include two important details:
>> > 
>> > * Who is effected by the bug fixed in the patch.
> This patch is a follow-up to the discussion:
> 
> http://sourceware.org/ml/libc-alpha/2012-06/msg00310.html
> 
> where Kosaki Motohiro reported that he received complaints from the
> ruby folks that the fix to bz 12416 caused a regression. The ruby
> interpreter shows a special error message when a stack overflow is
> detected, which is done using pthread_getattr_np.
> 
> The stacksize returned by pthread_getattr_np is incorrect if the
> stacksize computed is based on rlimit since it should be adjusted
> against the pages we skip above __libc_stack_end. Further, if the
> stacksize is based on rlimit, it should be truncated to align to page
> size because the rlimit may not necessarily be page aligned.

Have we gotten confirmation from them that this patch fixes the issue?

>> > 
>> > * What risk is there with this fix.
> The fix is contained since it is limited to nptl and that too, to
> reading of stack bounds for the thread. As a result the scope for
> breakage due to untested regressions from this patch is contained. Not
> having this fix however has a slightly greater risk since the current
> state of pthread_getattr_np is such that it can return a stack top that
> is not accessible and hence cause segmentation faults in programs that
> were working earlier.

Agreed.
 
> Regards,
> Siddhesh
> 
> 
> nptl/ChangeLog:
> 
> 2012-06-19  Siddhesh Poyarekar  <siddhesh@redhat.com>

As a follow-up to BZ#12416 this patch should also carry the same BZ#.

> 	* Makefile (tests): Add test case.
> 	* pthread_getattr_np.c (pthread_getattr_np): Deduct pages below
> 	the __libc_stack_end page from stacksize.  Truncate stacksize to
> 	make it page aligned when it is computed from RLIMIT_STACK.
> 	* tst-pthread-getattr.c: New test case. Verify that stackaddr is
> 	accessible.

This is OK to checkin if we have confirmation that this fixes the reported use-case.

> pthread-getattr.patch
> 
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 2a36d01..ef8e874 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -245,7 +245,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-pthread-getattr \
>  	tst-unload \
>  	tst-dlsym1 \
>  	tst-sysconf \
> diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
> index 75d717b..7309185 100644
> --- a/nptl/pthread_getattr_np.c
> +++ b/nptl/pthread_getattr_np.c
> @@ -120,8 +120,15 @@ pthread_getattr_np (thread_id, attr)
>  		      && (uintptr_t) __libc_stack_end < to)
>  		    {
>  		      /* Found the entry.  Now we have the info we need.  */
> -		      iattr->stacksize = rl.rlim_cur;
>  		      iattr->stackaddr = stack_end;
> +		      iattr->stacksize =
> +		        rl.rlim_cur - (size_t) (to - (uintptr_t) stack_end);
> +
> +		      /* Cut it down to align it to page size since otherwise we
> +		         risk going beyond rlimit when the kernel rounds up the
> +		         stack extension request.  */
> +		      iattr->stacksize = (iattr->stacksize
> +					  & -(intptr_t) GLRO(dl_pagesize));

This looks OK (and is restricted).

Note:
- We need some standard macros for align up and align down.
http://sourceware.org/ml/libc-alpha/2012-04/msg00006.html

>  
>  		      /* The limit might be too high.  */
>  		      if ((size_t) iattr->stacksize
> diff --git a/nptl/tst-pthread-getattr.c b/nptl/tst-pthread-getattr.c
> new file mode 100644
> index 0000000..cf4f6e7
> --- /dev/null
> +++ b/nptl/tst-pthread-getattr.c
> @@ -0,0 +1,119 @@
> +/* Make sure that the stackaddr returned by pthread_getattr_np is
> +   reachable.
> +
> +   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 <stdio.h>
> +#include <string.h>
> +#include <sys/resource.h>
> +#include <pthread.h>
> +#include <alloca.h>
> +
> +/* Move the stack pointer so that stackaddr is accessible and then check if it
> +   really is accessible.  This will segfault if it fails.  */
> +static void
> +allocate_and_test (void *stackaddr)
> +{
> +  void *mem = &mem;
> +  mem = alloca ((size_t) (mem - stackaddr));
> +  *(int *)(mem) = 0;
> +}
> +
> +static int
> +get_self_pthread_attr (const char *id, void **stackaddr, size_t *stacksize)
> +{
> +  pthread_attr_t attr;
> +  int ret;
> +  pthread_t me = pthread_self ();
> +
> +  if ((ret = pthread_getattr_np (me, &attr)))
> +    {
> +      printf ("%s: pthread_getattr_np failed: %s\n", id, strerror (ret));
> +      return 1;
> +    }
> +
> +  if ((ret = pthread_attr_getstack (&attr, stackaddr, stacksize)))
> +    {
> +      printf ("%s: pthread_attr_getstack returned error: %s\n", id,
> +	      strerror (ret));
> +      return 1;
> +    }
> +
> +  return 0;
> +}
> +
> +/* Verify that the stack size returned by pthread_getattr_np is usable when
> +   the returned value is subject to rlimit.  */
> +static int
> +check_stack_top (void)
> +{
> +  struct rlimit stack_limit;
> +  void *stackaddr;
> +  size_t stacksize = 0;
> +  int ret;
> +
> +  puts ("Verifying that stack top is accessible");
> +
> +  ret = getrlimit (RLIMIT_STACK, &stack_limit);
> +  if (ret)
> +    {
> +      perror ("getrlimit failed");
> +      return 1;
> +    }
> +
> +  if (get_self_pthread_attr ("check_stack_top", &stackaddr, &stacksize))
> +    return 1;
> +
> +  /* Reduce the rlimit to a page less that what is currently being returned so
> +     that we ensure that pthread_getattr_np uses rlimit.  The figure is
> +     intentionally unaligned so to verify that pthread_getattr_np returns an
> +     aligned stacksize that correctly fits into the rlimit.  We don't bother
> +     about the case where the stack is limited by the vma below it and not by
> +     the rlimit because the stacksize returned in that case is computed from
> +     the end of that vma and is hence safe.  */
> +  stack_limit.rlim_cur = stacksize - 4095;
> +  printf ("Adjusting RLIMIT_STACK to %zu\n", stack_limit.rlim_cur);
> +  if ((ret = setrlimit (RLIMIT_STACK, &stack_limit)))
> +    {
> +      perror ("setrlimit failed");
> +      return 1;
> +    }
> +
> +  if (get_self_pthread_attr ("check_stack_top2", &stackaddr, &stacksize))
> +    return 1;
> +
> +  printf ("Adjusted rlimit: stacksize=%zu, stackaddr=%p\n", stacksize,
> +          stackaddr);
> +  allocate_and_test (stackaddr);
> +
> +  puts ("Stack top tests done");
> +
> +  return 0;
> +}
> +
> +/* TODO: Similar check for thread stacks once the thread stack sizes are
> +   fixed.  */
> +static int
> +do_test (void)
> +{
> +  return check_stack_top ();
> +}
> +
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"

Excellent test case! :-)

Cheers,
Carlos.
-- 
Carlos O'Donell
Mentor Graphics / CodeSourcery
carlos_odonell@mentor.com
carlos@codesourcery.com
+1 (613) 963 1026



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