* [PATCH v2][BZ #22637] Fix stack guard size accounting
@ 2018-01-08 18:05 Szabolcs Nagy
2018-01-08 18:11 ` Carlos O'Donell
0 siblings, 1 reply; 2+ messages in thread
From: Szabolcs Nagy @ 2018-01-08 18:05 UTC (permalink / raw)
To: GNU C Library; +Cc: nd, Carlos O'Donell, Florian Weimer
[-- Attachment #1: Type: text/plain, Size: 908 bytes --]
v2:
- use separate bug number from the tls accounting issue.
- check for arithmetic overflows.
Previously if user requested S stack and G guard when creating a
thread, the total mapping was S and the actual available stack was
S - G - static_tls, which is not what the user requested.
This patch fixes the guard size accounting by pretending the user
requested S+G stack. This way all later logic works out except
when reporting the user requested stack size (pthread_getattr_np)
or when computing the minimal stack size (__pthread_get_minstack).
Normally this will increase thread stack allocations by one page.
TLS accounting is not affected, that will require a separate fix.
[BZ #22637]
* nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize.
* nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from
stacksize.
* nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise.
[-- Attachment #2: guard.diff --]
[-- Type: text/x-patch, Size: 1929 bytes --]
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 05b8ed331b..b374f4794d 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -532,6 +532,10 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
/* Make sure the size of the stack is enough for the guard and
eventually the thread descriptor. */
guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
+ if (guardsize < attr->guardsize || size + guardsize < guardsize)
+ /* Arithmetic overflow. */
+ return EINVAL;
+ size += guardsize;
if (__builtin_expect (size < ((guardsize + __static_tls_size
+ MINIMAL_REST_STACK + pagesize_m1)
& ~pagesize_m1),
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index c237a3675a..5a4b52419f 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -472,8 +472,5 @@ strong_alias (__pthread_initialize_minimal_internal,
size_t
__pthread_get_minstack (const pthread_attr_t *attr)
{
- struct pthread_attr *iattr = (struct pthread_attr *) attr;
-
- return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN
- + iattr->guardsize);
+ return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN;
}
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index 961d711fd2..c79fd7b4ec 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -57,9 +57,10 @@ pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr)
/* The sizes are subject to alignment. */
if (__glibc_likely (thread->stackblock != NULL))
{
- iattr->stacksize = thread->stackblock_size;
+ iattr->stacksize = thread->stackblock_size - thread->guardsize;
#if _STACK_GROWS_DOWN
- iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize;
+ iattr->stackaddr = (char *) thread->stackblock
+ + thread->stackblock_size;
#else
iattr->stackaddr = (char *) thread->stackblock;
#endif
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2][BZ #22637] Fix stack guard size accounting
2018-01-08 18:05 [PATCH v2][BZ #22637] Fix stack guard size accounting Szabolcs Nagy
@ 2018-01-08 18:11 ` Carlos O'Donell
0 siblings, 0 replies; 2+ messages in thread
From: Carlos O'Donell @ 2018-01-08 18:11 UTC (permalink / raw)
To: Szabolcs Nagy, GNU C Library; +Cc: nd, Florian Weimer
On 01/08/2018 10:04 AM, Szabolcs Nagy wrote:
> v2:
> - use separate bug number from the tls accounting issue.
> - check for arithmetic overflows.
>
> Previously if user requested S stack and G guard when creating a
> thread, the total mapping was S and the actual available stack was
> S - G - static_tls, which is not what the user requested.
>
> This patch fixes the guard size accounting by pretending the user
> requested S+G stack. This way all later logic works out except
> when reporting the user requested stack size (pthread_getattr_np)
> or when computing the minimal stack size (__pthread_get_minstack).
>
> Normally this will increase thread stack allocations by one page.
> TLS accounting is not affected, that will require a separate fix.
>
> [BZ #22637]
> * nptl/allocatestack.c (allocate_stack): Add guardsize to stacksize.
> * nptl/nptl-init.c (__pthread_get_minstack): Remove guardsize from
> stacksize.
> * nptl/pthread_getattr_np.c (pthread_getattr_np): Likewise.
>
OK with the comments added as requested.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 05b8ed331b..b374f4794d 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -532,6 +532,10 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
> /* Make sure the size of the stack is enough for the guard and
> eventually the thread descriptor. */
> guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
> + if (guardsize < attr->guardsize || size + guardsize < guardsize)
> + /* Arithmetic overflow. */
> + return EINVAL;
> + size += guardsize;
OK.
> if (__builtin_expect (size < ((guardsize + __static_tls_size
> + MINIMAL_REST_STACK + pagesize_m1)
> & ~pagesize_m1),
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index c237a3675a..5a4b52419f 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -472,8 +472,5 @@ strong_alias (__pthread_initialize_minimal_internal,
> size_t
> __pthread_get_minstack (const pthread_attr_t *attr)
> {
> - struct pthread_attr *iattr = (struct pthread_attr *) attr;
> -
> - return (GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN
> - + iattr->guardsize);
> + return GLRO(dl_pagesize) + __static_tls_size + PTHREAD_STACK_MIN;
OK. Happy to see this expression get simpler!
> }
> diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
> index 961d711fd2..c79fd7b4ec 100644
> --- a/nptl/pthread_getattr_np.c
> +++ b/nptl/pthread_getattr_np.c
> @@ -57,9 +57,10 @@ pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr)
> /* The sizes are subject to alignment. */
> if (__glibc_likely (thread->stackblock != NULL))
> {
This operation needs a comment explaining why we subtract guard size.
> - iattr->stacksize = thread->stackblock_size;
> + iattr->stacksize = thread->stackblock_size - thread->guardsize;
> #if _STACK_GROWS_DOWN
> - iattr->stackaddr = (char *) thread->stackblock + iattr->stacksize;
> + iattr->stackaddr = (char *) thread->stackblock
> + + thread->stackblock_size;
> #else
> iattr->stackaddr = (char *) thread->stackblock;
> #endif
You need to add a comment to nptl/descr.h to explain that stackblock_size
is the sum of the stack and guard.
--
Cheers,
Carlos.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-01-08 18:11 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-08 18:05 [PATCH v2][BZ #22637] Fix stack guard size accounting Szabolcs Nagy
2018-01-08 18:11 ` Carlos O'Donell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).