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


> 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.

That sounds reasonable.

> 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.

The only _STACK_GROWS_UP machine is hppa.

> 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;

Both the argument and the local are unused.

> +  if (__builtin_frame_address (0) > __builtin_frame_address (1))
> +    return true;
> +  else {
> +    return false;
> +  }

A brace goes on its own line.  When you need braces, that is.
Here, you don't.  In fact, you don't need an if at all.
Just return the result of the test.

I don't think it's reliable to use __builtin_frame_address with a nonzero
argument, depending on optimization behavior and such.

Why don't you just use #if _STACK_GROWS_{DOWN,UP} here?

> +  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;
> +    }

The second message names the wrong function.

> +  if (stack_grows_down(stack_size))

Space before paren.

> +#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;
> +    }

Wrong function in the message again.  But also, I'd use a different message
to distinguish a failing first call from a failing second call.

> +  if (stack_grows_down(stack_size))

Space before paren.

> +    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.  */

This says we're not checking them separately but doesn't really say why,
or at least not clearly enough for me.  If it's really the case that the
two could change such that their sum/difference remains the same and
that would be kosher, then explain it more thoroughly.  If not, then
just check them separately.

> +	      /* 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.  */

DSO in caps.

> +	      void *real_stack_end;
> +
> +	      real_stack_end = (void *) ((uintptr_t) __libc_stack_end
> +					 & -(uintptr_t) GLRO(dl_pagesize));

Just use an initializer here.


Thanks,
Roland


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