Re: [PATCH] i386: In makecontext, align the stack before calling exit [BZ #22667]

[Carlos O'Donell <carlos at redhat dot com>]

On 01/04/2018 09:03 AM, Florian Weimer wrote:
> Before this change, if glibc was compiled with SSE instructions and a
> sufficiently recent GCC, an unaligned stack access in
> __run_exit_handlers would cause stdlib/tst-makecontext to crash.
> 
> 2018-01-04  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #22667]
> 	* sysdeps/unix/sysv/linux/i386/makecontext.S (__makecontext):
> 	Align the stack before calling exit.
> 	* stdlib/tst-makecontext-align.c: New file.
> 	* stdlib/Makefile (tests): Add tst-makecontext-align.

I don't know why makecontext continues to be written in assembly when
it can be written reasonably in portable C, but oh well, it would have
avoided this issue entirely. This isn't your problem to fix though.

This patch looks good to me.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/stdlib/Makefile b/stdlib/Makefile
> index 691325b91b..7c363a6e4d 100644
> --- a/stdlib/Makefile
> +++ b/stdlib/Makefile
> @@ -83,7 +83,8 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
>  		   tst-getrandom tst-atexit tst-at_quick_exit 		    \
>  		   tst-cxa_atexit tst-on_exit test-atexit-race 		    \
>  		   test-at_quick_exit-race test-cxa_atexit-race             \
> -		   test-on_exit-race test-dlclose-exit-race
> +		   test-on_exit-race test-dlclose-exit-race 		    \
> +		   tst-makecontext-align

OK. Thank you very much for the new test.

>  
>  tests-internal	:= tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \
>  		   tst-tls-atexit tst-tls-atexit-nodelete
> diff --git a/stdlib/tst-makecontext-align.c b/stdlib/tst-makecontext-align.c
> new file mode 100644
> index 0000000000..82394b4f6b
> --- /dev/null
> +++ b/stdlib/tst-makecontext-align.c
> @@ -0,0 +1,241 @@
> +/* Check stack alignment provided by makecontext.
> +   Copyright (C) 2018 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 <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <support/check.h>
> +#include <support/namespace.h>
> +#include <support/xunistd.h>
> +#include <sys/mman.h>
> +#include <ucontext.h>
> +

OK. This test is an epic tour de force for testing alignment of the stack
after calling setcontext. I appreciate the attention to detail.


> +/* Used for error reporting.  */
> +static const char *context;
> +
> +/* Check that ADDRESS is aligned to ALIGNMENT bytes, behind a compiler
> +   barrier.  */
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align (void *address, size_t alignment)
> +{
> +  uintptr_t uaddress = (uintptr_t) address;
> +  if ((uaddress % alignment) != 0)
> +    {
> +      support_record_failure ();
> +      printf ("error: %s: object at address %p is not aligned to %zu bytes\n",
> +              context, address, alignment);
> +    }
> +}
> +
> +/* Various alignment checking functions.  */
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_int (void)
> +{
> +  int a;
> +  check_align (&a, __alignof__ (a));
> +}
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_long (void)
> +{
> +  long a;
> +  check_align (&a, __alignof__ (a));
> +}
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_long_long (void)
> +{
> +  long long a;
> +  check_align (&a, __alignof__ (a));
> +}
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_double (void)
> +{
> +  double a;
> +  check_align (&a, __alignof__ (a));
> +}
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_4 (void)
> +{
> +  int a __attribute__ ((aligned (4)));
> +  check_align (&a, 4);
> +}
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_8 (void)
> +{
> +  double a __attribute__ ((aligned (8)));
> +  check_align (&a, 8);
> +}
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_16 (void)
> +{
> +  struct aligned
> +  {
> +    double x0  __attribute__ ((aligned (16)));
> +    double x1;
> +  } a;
> +  check_align (&a, 16);
> +}
> +
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_align_32 (void)
> +{
> +  struct aligned
> +  {
> +    double x0  __attribute__ ((aligned (32)));
> +    double x1;
> +    double x2;
> +    double x3;
> +  } a;
> +  check_align (&a, 32);
> +}
> +
> +/* Call all the alignment checking functions.  */
> +__attribute__ ((noinline, noclone, weak))
> +void
> +check_alignments (void)
> +{
> +  check_align_int ();
> +  check_align_long ();
> +  check_align_long_long ();
> +  check_align_double ();
> +  check_align_4 ();
> +  check_align_8 ();
> +  check_align_16 ();
> +  check_align_32 ();
> +}
> +
> +/* Callback functions for makecontext and their invokers (to be used
> +   with support_isolate_in_subprocess).  */
> +
> +static ucontext_t ucp;
> +
> +static void
> +callback_0 (void)
> +{
> +  context = "callback_0";
> +  check_alignments ();
> +  context = "after return from callback_0";
> +}
> +
> +static void
> +invoke_callback_0 (void *closure)
> +{
> +  makecontext (&ucp, (void *) callback_0, 0);
> +  if (setcontext (&ucp) != 0)
> +    FAIL_EXIT1 ("setcontext");
> +  FAIL_EXIT1 ("setcontext returned");
> +}
> +
> +static void
> +callback_1 (int arg1)
> +{
> +  context = "callback_1";
> +  check_alignments ();
> +  TEST_COMPARE (arg1, 101);
> +  context = "after return from callback_1";
> +}
> +
> +static void
> +invoke_callback_1 (void *closure)
> +{
> +  makecontext (&ucp, (void *) callback_1, 1, 101);
> +  if (setcontext (&ucp) != 0)
> +    FAIL_EXIT1 ("setcontext");
> +  FAIL_EXIT1 ("setcontext returned");
> +}
> +
> +static void
> +callback_2 (int arg1, int arg2)
> +{
> +  context = "callback_2";
> +  check_alignments ();
> +  TEST_COMPARE (arg1, 201);
> +  TEST_COMPARE (arg2, 202);
> +  context = "after return from callback_2";
> +}
> +
> +static void
> +invoke_callback_2 (void *closure)
> +{
> +  makecontext (&ucp, (void *) callback_2, 2, 201, 202);
> +  if (setcontext (&ucp) != 0)
> +    FAIL_EXIT1 ("setcontext");
> +  FAIL_EXIT1 ("setcontext returned");
> +}
> +
> +static void
> +callback_3 (int arg1, int arg2, int arg3)
> +{
> +  context = "callback_3";
> +  check_alignments ();
> +  TEST_COMPARE (arg1, 301);
> +  TEST_COMPARE (arg2, 302);
> +  TEST_COMPARE (arg3, 303);
> +  context = "after return from callback_3";
> +}
> +
> +static void
> +invoke_callback_3 (void *closure)
> +{
> +  makecontext (&ucp, (void *) callback_3, 3, 301, 302, 303);
> +  if (setcontext (&ucp) != 0)
> +    FAIL_EXIT1 ("setcontext");
> +  FAIL_EXIT1 ("setcontext returned");
> +}
> +
> +static int
> +do_test (void)
> +{
> +  context = "direct call";
> +  check_alignments ();

OK. Good check of expected alignments before testing context functions.

> +
> +  atexit (check_alignments);
> +
> +  if (getcontext (&ucp) != 0)
> +    FAIL_UNSUPPORTED ("getcontext");
> +
> +  ucp.uc_link = NULL;
> +  ucp.uc_stack.ss_size = 512 * 1024;
> +  ucp.uc_stack.ss_sp = xmmap (NULL, ucp.uc_stack.ss_size,
> +                              PROT_READ | PROT_WRITE,
> +                              MAP_PRIVATE | MAP_ANONYMOUS, -1);
> +
> +  support_isolate_in_subprocess (invoke_callback_0, NULL);
> +  support_isolate_in_subprocess (invoke_callback_1, NULL);
> +  support_isolate_in_subprocess (invoke_callback_2, NULL);
> +  support_isolate_in_subprocess (invoke_callback_3, NULL);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/i386/makecontext.S b/sysdeps/unix/sysv/linux/i386/makecontext.S
> index efa4955033..e3ca3dc0d5 100644
> --- a/sysdeps/unix/sysv/linux/i386/makecontext.S
> +++ b/sysdeps/unix/sysv/linux/i386/makecontext.S
> @@ -108,9 +108,19 @@ L(exitcode):
>  	call	HIDDEN_JUMPTARGET(__setcontext)
>  	/* If this returns (which can happen if the syscall fails) we'll
>  	   exit the program with the return error value (-1).  */
> +	jmp L(call_exit)
>  
> -	movl	%eax, (%esp)
> -2:	call	HIDDEN_JUMPTARGET(exit)
> +2:
> +	/* Exit with status 0.  */
> +	xorl	%eax, %eax
> +
> +L(call_exit):
> +	/* Align the stack and pass the exit code (from %eax).  */
> +	andl	$0xfffffff0, %esp
> +	subl	$12, %esp
> +	pushl	%eax
> +
> +	call	HIDDEN_JUMPTARGET(exit)

OK. Looks good to me.

We even do this already but only for the passed-in stack!

>  	/* The 'exit' call should never return.  In case it does cause
>  	   the process to terminate.  */
>  	hlt
> 


-- 
Cheers,
Carlos.