Re: [PATCH] posix_spawn: use a larger min stack for -fstack-check [BZ #21253]

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 16/03/2017 19:41, Mike Frysinger wrote:
> On 16 Mar 2017 14:56, Andrew Pinski wrote:
>> On Thu, Mar 16, 2017 at 2:52 PM, Mike Frysinger wrote:
>>> On 16 Mar 2017 09:17, Florian Weimer wrote:
>>>> On 03/16/2017 08:30 AM, Mike Frysinger wrote:
>>>>> When glibc is built with -fstack-check, trying to use posix_spawn can
>>>>> lead to segfaults due to gcc internally probing stack memory too far.
>>>>> The new spawn API will allocate a minimum of 1 page, but the stack
>>>>> checking logic might probe a couple of pages.  When it tries to walk
>>>>> them, everything falls apart.
>>>>>
>>>>> The gcc internal docs [1] state the default interval checking is one
>>>>> page.  Which means we need two pages (the current one, and the next
>>>>> probed).  No target currently defines it larger.
>>>>
>>>> GCC miscomputes the offsets in some cases, so I would not rely on this.
>>>>
>>>> Would it be possible compile the functions involved without
>>>> -fstack-check instead?
>>>
>>> i mentioned in the bug that it's not feasible: compiling this one file
>>> doesn't help as it calls other glibc funcs which will have checking
>>> enabled.  so we'd have to manually track the full call stack here and
>>> disable it on all the files which is a fairly fragile/burdensome process.
>>>
>>>>>    /* Add a slack area for child's stack.  */
>>>>>    size_t argv_size = (argc * sizeof (void *)) + 512;
>>>>> -  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
>>>>> +  /* We need at least a few pages in case the compiler's stack checking is
>>>>> +     enabled.  In some configs, it is known to use at least 24KiB.  */
>>>>> +  size_t stack_size = ALIGN_UP (argv_size, 32 * 1024);
>>>>
>>>> 64 KiB pages are common, so this reduces the stack size in many cases.
>>>
>>> common where ?  are you talking about large pages ?
>>>
>>> going by PAGE_SHIFT in the kernel, looks to me like very few
>>> targets allow using 64 KiB at all, let alone being common.
>>
>> RHEL on ARM64 defaults to 64k.
>> For Cavium Octeon SDK, the mips kernel defaults to 64k page size.
> 
> i think it's fair to say that those userbases do not constitute anywhere
> close to a majority, or even a signficiantly large presence.
> 
> from what i can tell, gcc does not expose any CPP defines we can use to
> see if stack checking is enabled.  so we don't have a way of turning the
> extra allocations on conditionally.
> 
> wouldn't be easy to add a configure check either since -fstack-check can
> be turned on via default specs.
> -mike
> 

My main concern is to add some hackery on an implementation that should be
neutral to any compiler option and to keep try to adjust the initial stack
size based on possible compiler version/architecture.  In any way, I think
the following patch should the -fstack-check if compiler is using it:

diff --git a/config.h.in b/config.h.in
index fb2cc51..6bfa8f7 100644
--- a/config.h.in
+++ b/config.h.in
@@ -263,4 +263,7 @@
    in i386 6 argument syscall issue).  */
 #define CAN_USE_REGISTER_ASM_EBP 0
 
+/* Build glibc with gcc -fstack-check.  */
+#define BUILD_FSTACK_CHECK 0
+
 #endif
diff --git a/configure.ac b/configure.ac
index 4981bf9..26629e1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1626,6 +1626,22 @@ for opt in -ffp-contract=off -mno-fused-madd; do
 done])
 AC_SUBST(libc_cv_cc_nofma)
 
+
+dnl Determine if -fstack-check is being used
+AC_CACHE_CHECK(for -fstack-check, libc_cv_fstack_check, [dnl
+cat > conftest.c << \EOF
+void foo (void) {};
+EOF
+dnl
+libc_cv_fstack_check=no
+if ${CC-cc} $CFLAGS -Q -v conftest.c -c 2>&1 | sed -e '/cc1/!d;s/(")|(^.* - )//g' | grep '\-fstack\-check' >/dev/null; then
+  libc_cv_fstack_check=yes
+fi
+rm -f conftest*])
+if test x"$libc_cv_fstack_check" = xyes; then
+  AC_DEFINE(BUILD_FSTACK_CHECK)
+fi
+
 if test -n "$submachine"; then
   AC_CACHE_CHECK([for compiler option for CPU variant],
 		 libc_cv_cc_submachine, [dnl
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
index 24f75db..bfc033f 100644
--- a/sysdeps/unix/sysv/linux/spawni.c
+++ b/sysdeps/unix/sysv/linux/spawni.c
@@ -317,8 +317,20 @@ __spawnix (pid_t * pid, const char *file,
 	     | ((GL (dl_stack_flags) & PF_X) ? PROT_EXEC : 0));
 
   /* Add a slack area for child's stack.  */
+#if BUILD_FSTACK_CHECK
+/* The gcc -fstack-check internal docs state the default interval checking is
+   one page, which means we need two pages (the current one, and the next
+   probed).  No target currently defines it larger.  Further, it mentions that
+   the default minimum stack size needed to recover from an overflow is 4/8KiB
+   for sjlj or 8/12KiB for others.  But some Linux targets (like mips and
+   ppc) go up to 16KiB (and some non-Linux targets go up to 24KiB).  So
+   we allocate at least 4 pages for such cases*/
+# define MIN_PAGES 4
+#else
+# define MIn_PAGES 1
+#endif
   size_t argv_size = (argc * sizeof (void *)) + 512;
-  size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize));
+  size_t stack_size = ALIGN_UP (argv_size, MIN_PAGES * GLRO(dl_pagesize));
   void *stack = __mmap (NULL, stack_size, prot,
 			MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0);
   if (__glibc_unlikely (stack == MAP_FAILED))

Attachment: signature.asc
Description: OpenPGP digital signature