This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH 2/2 Take-2][BZ #12416] Use stack boundaries from/proc/PID/maps to make stack executable
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Fri, 4 May 2012 16:08:45 -0700 (PDT)
- Subject: Re: [PATCH 2/2 Take-2][BZ #12416] Use stack boundaries from/proc/PID/maps to make stack executable
- References: <20120419120021.4780e8c8@spoyarek><20120425203424.A744A2C0CA@topped-with-meat.com><20120427084258.02317347@spoyarek>
> (_dl_make_stack_executable): Also mark pages below (or above if
> _STACK_GROWS_UP) as executable. Consolidate code to
Two spaces between sentences.
> +#if USE_PTHREADS
> + void *old_stack_addr, *new_stack_addr;
> + size_t stack_size;
> + pthread_t me = pthread_self ();
> + pthread_attr_t attr;
> + pthread_getattr_np (me, &attr);
> + pthread_attr_getstack (&attr, &old_stack_addr, &stack_size);
Check for errors.
> + old_stack_addr += stack_size;
That looks like it's assuming the stack grows down. I think your actual
intent is just to verify that both the address and size were the same in
both calls. But you're not doing that, you're only verifying the sum.
Why not just save both values and check them both below?
If your intent is something subtler than that, then it needs comments.
> --- a/sysdeps/unix/sysv/linux/dl-execstack.c
> +++ b/sysdeps/unix/sysv/linux/dl-execstack.c
> @@ -30,6 +30,52 @@
> extern int __stack_prot attribute_relro attribute_hidden;
> +/* Mark the stack as executable, a few pages at a time. If we encounter an
> + ENOMEM, we try to mprotect lower number of pages till we're sure that we
"a lower number" or "lower numbers"
> + have reached the end or beginning of stack. DIRECTION is -1 for down and 1
"of the stack"
> + for up, which allows us to go to either side of the page address. */
Describe all the arguments and the return value in the comment.
AFAIK there is no reason to have the RESULT parameter at all.
Just return an errno value or zero.
> +static int
> +_incremental_make_stack_executable (uintptr_t page, size_t size, int *result,
> + int direction)
Don't use an _ on a static function name. If it were a global function,
it would get __ instead. Don't use an int of 1/-1 when it's really just
a Boolean. Make it "bool down" (or up, you choose).
Don't use uintptr_t and cast it all the time. You can do arithmetic on a
void * in GNU code, so just do that.
> + /* We need to mark the vma below the stack end as well, since the kernel
> + sets up some pages with elf info, environment variables and program
> + arguments below the end. Not doing this will split the vma. */
ELF, in caps. But here you can just say what you mean specifically,
which is "auxv".