bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
* sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
@ 2021-05-17  0:33 Bruno Haible
  2021-05-17  2:45 ` Paul Eggert
  0 siblings, 1 reply; 11+ messages in thread
From: Bruno Haible @ 2021-05-17  0:33 UTC (permalink / raw)
  To: bug-gnulib

With the glibc header files from a current glibc (post 2.33) and
CC="gcc -I$GLIBC_INSTALL_PREFIX/include -D_GNU_SOURCE", I get this
compilation error in a test of module 'sigsegv':

In file included from ../../gltests/test-sigsegv-catch-stackoverflow1.c:44:0:
../../gltests/altstack-util.h:32:6: error: variably modified ‘mystack_storage’ at file scope
 char mystack_storage[SIGSTKSZ + 2 * MYSTACK_CRUMPLE_ZONE + 31];
      ^

Similarly, the autoconf tests of the modules 'sigsegv' and 'c-stack'
have guessed wrong.

The cause is that in glibc, SIGSTKSZ is no longer a compile-time constant.
The discussion in March 2021
<https://sourceware.org/pipermail/libc-alpha/2021-March/123553.html>
only had the effect that the change was limited to the case that _GNU_SOURCE
is enabled. But we are compiling nearly all programs with _GNU_SOURCE=1.

I don't have time to rewrite all these unit tests and autoconf tests.
(Additionally, if we were to use malloc() as a replacement, watch out
that malloc()ed memory may not be executable, but some programs need
an executable stack.)

Instead, just use a constant. Which constant is appropriate? glibc-2.33
has these definitions:

glibc-2.33/sysdeps/unix/sysv/linux/sparc/bits/sigstack.h:#define SIGSTKSZ       16384
glibc-2.33/sysdeps/unix/sysv/linux/powerpc/bits/sigstack.h:#define SIGSTKSZ     16384
glibc-2.33/sysdeps/unix/sysv/linux/ia64/bits/sigstack.h:#define SIGSTKSZ        262144
glibc-2.33/sysdeps/unix/sysv/linux/bits/sigstack.h:#define SIGSTKSZ     8192
glibc-2.33/sysdeps/unix/sysv/linux/alpha/bits/sigstack.h:#define SIGSTKSZ       16384
glibc-2.33/sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h:#define SIGSTKSZ     16384
glibc-2.33/bits/sigstack.h:#define SIGSTKSZ     (MINSIGSTKSZ + 32768)
glibc-2.33/bits/sigstack.h:#define MINSIGSTKSZ  8192

So, basically, 256 KB on ia64 and 40 KB (or 64 KB) on all other architectures
should be sufficient. Although there never can be a true guarantee regarding
future processors.

This patch fixes the things from the gnulib side.


2021-05-16  Bruno Haible  <bruno@clisp.org>

	sigsegv, c-stack: Avoid compilation error with glibc >= 2.34.
	* lib/sigsegv.in.h (SIGSTKSZ): On glibc systems, redefine to a suitable
	constant.
	* m4/sigaltstack.m4 (SV_SIGALTSTACK): Likewise.
	* m4/c-stack.m4 (AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC): Likewise.

diff --git a/lib/sigsegv.in.h b/lib/sigsegv.in.h
index 1c87acc..e442f4d 100644
--- a/lib/sigsegv.in.h
+++ b/lib/sigsegv.in.h
@@ -37,16 +37,25 @@
 #endif
 
 /* Correct the value of SIGSTKSZ on some systems.
+   glibc >= 2.34: When _GNU_SOURCE is defined, SIGSTKSZ is no longer a
+   compile-time constant.  But most programs need a simple constant.
    AIX 64-bit: original value 4096 is too small.
    HP-UX: original value 8192 is too small.
    Solaris 11/x86_64: original value 8192 is too small.  */
+#include <signal.h>
+#if __GLIBC__ >= 2
+# undef SIGSTKSZ
+# if defined __ia64__
+#  define SIGSTKSZ 262144
+# else
+#  define SIGSTKSZ 65536
+# endif
+#endif
 #if defined _AIX && defined _ARCH_PPC64
-# include <signal.h>
 # undef SIGSTKSZ
 # define SIGSTKSZ 8192
 #endif
 #if defined __hpux || (defined __sun && (defined __x86_64__ || defined __amd64__))
-# include <signal.h>
 # undef SIGSTKSZ
 # define SIGSTKSZ 16384
 #endif
diff --git a/m4/c-stack.m4 b/m4/c-stack.m4
index 06b2594..3131dd5 100644
--- a/m4/c-stack.m4
+++ b/m4/c-stack.m4
@@ -7,7 +7,7 @@
 
 # Written by Paul Eggert.
 
-# serial 21
+# serial 22
 
 AC_DEFUN([AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC],
   [
@@ -44,6 +44,17 @@ AC_DEFUN([AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC],
             # include <sys/time.h>
             # include <sys/resource.h>
             #endif
+            /* In glibc >= 2.34, when _GNU_SOURCE is defined, SIGSTKSZ is
+               no longer a compile-time constant.  But we need a simple
+               constant here.  */
+            #if __GLIBC__ >= 2
+            # undef SIGSTKSZ
+            # if defined __ia64__
+            #  define SIGSTKSZ 262144
+            # else
+            #  define SIGSTKSZ 16384
+            # endif
+            #endif
             #ifndef SIGSTKSZ
             # define SIGSTKSZ 16384
             #endif
@@ -149,6 +160,16 @@ AC_DEFUN([AC_SYS_XSI_STACK_OVERFLOW_HEURISTIC],
 #if HAVE_SYS_SIGNAL_H
 # include <sys/signal.h>
 #endif
+/* In glibc >= 2.34, when _GNU_SOURCE is defined, SIGSTKSZ is no longer a
+   compile-time constant.  But we need a simple constant here.  */
+#if __GLIBC__ >= 2
+# undef SIGSTKSZ
+# if defined __ia64__
+#  define SIGSTKSZ 262144
+# else
+#  define SIGSTKSZ 16384
+# endif
+#endif
 #ifndef SIGSTKSZ
 # define SIGSTKSZ 16384
 #endif
@@ -233,6 +254,17 @@ int main ()
                 # include <sys/time.h>
                 # include <sys/resource.h>
                 #endif
+                /* In glibc >= 2.34, when _GNU_SOURCE is defined, SIGSTKSZ is
+                   no longer a compile-time constant.  But we need a simple
+                   constant here.  */
+                #if __GLIBC__ >= 2
+                # undef SIGSTKSZ
+                # if defined __ia64__
+                #  define SIGSTKSZ 262144
+                # else
+                #  define SIGSTKSZ 16384
+                # endif
+                #endif
                 #ifndef SIGSTKSZ
                 # define SIGSTKSZ 16384
                 #endif
diff --git a/m4/sigaltstack.m4 b/m4/sigaltstack.m4
index 212e9d3..837191e 100644
--- a/m4/sigaltstack.m4
+++ b/m4/sigaltstack.m4
@@ -1,4 +1,4 @@
-# sigaltstack.m4 serial 12
+# sigaltstack.m4 serial 13
 dnl Copyright (C) 2002-2021 Bruno Haible <bruno@clisp.org>
 dnl Copyright (C) 2008 Eric Blake <ebb9@byu.net>
 dnl This file is free software, distributed under the terms of the GNU
@@ -53,6 +53,16 @@ AC_DEFUN([SV_SIGALTSTACK],
 # include <sys/time.h>
 # include <sys/resource.h>
 #endif
+/* In glibc >= 2.34, when _GNU_SOURCE is defined, SIGSTKSZ is no longer a
+   compile-time constant.  But we need a simple constant here.  */
+#if __GLIBC__ >= 2
+# undef SIGSTKSZ
+# if defined __ia64__
+#  define SIGSTKSZ 262144
+# else
+#  define SIGSTKSZ 16384
+# endif
+#endif
 #ifndef SIGSTKSZ
 # define SIGSTKSZ 16384
 #endif
@@ -138,6 +148,16 @@ int main ()
 #if HAVE_SYS_SIGNAL_H
 # include <sys/signal.h>
 #endif
+/* In glibc >= 2.34, when _GNU_SOURCE is defined, SIGSTKSZ is no longer a
+   compile-time constant.  But we need a simple constant here.  */
+#if __GLIBC__ >= 2
+# undef SIGSTKSZ
+# if defined __ia64__
+#  define SIGSTKSZ 262144
+# else
+#  define SIGSTKSZ 16384
+# endif
+#endif
 #ifndef SIGSTKSZ
 # define SIGSTKSZ 16384
 #endif



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-17  0:33 sigsegv, c-stack: Avoid compilation error with glibc >= 2.34 Bruno Haible
@ 2021-05-17  2:45 ` Paul Eggert
  2021-05-18 17:34   ` Bruno Haible
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Eggert @ 2021-05-17  2:45 UTC (permalink / raw)
  To: bug-gnulib

On 5/16/21 7:33 PM, Bruno Haible wrote:
> I don't have time to rewrite all these unit tests and autoconf tests.

Can we assume you wouldn't mind if someone did take the time? (Not that 
I'm volunteering right just now.)


> (Additionally, if we were to use malloc() as a replacement, watch out
> that malloc()ed memory may not be executable, but some programs need
> an executable stack.)

That shouldn't be a problem on GNU platforms, since it's documented that 
you can malloc the alternate signal stack. If it does turn into a 
problem, I suppose we can assume that when SIGSTKSZ is not a constant 
that malloc should work (otherwise how could you ever create an 
alternate signal stack?).



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-17  2:45 ` Paul Eggert
@ 2021-05-18 17:34   ` Bruno Haible
  2021-05-21 22:49     ` Paul Eggert
  0 siblings, 1 reply; 11+ messages in thread
From: Bruno Haible @ 2021-05-18 17:34 UTC (permalink / raw)
  To: bug-gnulib; +Cc: Paul Eggert, Eric Blake

Hi Paul,

> > (Additionally, if we were to use malloc() as a replacement, watch out
> > that malloc()ed memory may not be executable, but some programs need
> > an executable stack.)
> 
> That shouldn't be a problem on GNU platforms, since it's documented that 
> you can malloc the alternate signal stack. If it does turn into a 
> problem, I suppose we can assume that when SIGSTKSZ is not a constant 
> that malloc should work

That's the theory. Here is how it really is:

On
  Linux/alpha,
  FreeBSD/i386,
  NetBSD/i386,
  macOS/i386,
  macOS/powerpc,
  Solaris 10/i386,
  Solaris 10/sparc
  Cygwin/i386,
  Minix/i386,
  Windows/i386 (mingw)
malloc'ed memory is executable.

On
  Linux/arm,
  Linux/arm64,
  Linux/i386,
  Linux/m68k,
  Linux/mips,
  Linux/powerpc,
  Linux/powerpc64le,
  Linux/s390,
  Linux/s390x,
  Linux/sparc,
  Linux/x86_64,
  Hurd/i386,
  FreeBSD/x86_64,
  DragonFly/i386,
  NetBSD/sparc,
  NetBSD/x86_64,
  OpenBSD/i386,
  OpenBSD/x86_64,
  macOS/x86_64,
  IRIX/mips,
  Solaris 10/x86_64,
  Cygwin/x86_64,
  Windows/x86_64 (mingw),
malloc'ed memory is not executable. But GCC's trampolines (used by nested
functions) require an executable stack. When an object file has such nested
functions, the toolchain arranges for the executable to ask the OS to
allocate an executable stack. But that only has an effect on the primary
stack of each thread. It does not have an effect on an alternate stack
(AFAIK).

On
  Linux/hppa,
  Linux/powerpc64,
  Linux/ia64,
  AIX,
  HP-UX,
it is irrelevant whether malloc'ed memory is executable, because GCC's
trampolines are built without executable code.

> (otherwise how could you ever create an alternate signal stack?).

You could call malloc() and then mprotect(rwx) on it. This would work
on some of the platforms above, namely:
  Hurd/i386,
  FreeBSD/x86_64,
  DragonFly/i386,
  NetBSD/sparc,
  NetBSD/x86_64,
  OpenBSD/i386,
  OpenBSD/x86_64,
  macOS/x86_64,
  IRIX/mips,
  Solaris 10/x86_64,
  Cygwin/x86_64,
It would not work on
  Linux (with SELinux),
  FreeBSD (hardened via kern.elf32.allow_wx=0, kern.elf64.allow_wx=0),
  macOS 11.

> > I don't have time to rewrite all these unit tests and autoconf tests.
> 
> Can we assume you wouldn't mind if someone did take the time? (Not that 
> I'm volunteering right just now.)

I wouldn't mind, if it's done properly:
  - The problem with non-executable malloc'ed memory would need to be
    solved.
  - The declaration in sigsegv.h needs to stay, at least for the next
    couple of years, because it is necessary for programs that use SIGSTKSZ
    (such as GNU m4 and GNU clisp) to compile fine.

Bruno



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-18 17:34   ` Bruno Haible
@ 2021-05-21 22:49     ` Paul Eggert
  2021-05-22 22:59       ` Bruno Haible
  2021-05-23  0:00       ` sigsegv.h interface Bruno Haible
  0 siblings, 2 replies; 11+ messages in thread
From: Paul Eggert @ 2021-05-21 22:49 UTC (permalink / raw)
  To: Bruno Haible; +Cc: Gnulib bugs, Eric Blake

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

On 5/18/21 10:34 AM, Bruno Haible wrote:

> malloc'ed memory is not executable. But GCC's trampolines (used by nested
> functions) require an executable stack. When an object file has such nested
> functions, the toolchain arranges for the executable to ask the OS to
> allocate an executable stack. But that only has an effect on the primary
> stack of each thread. It does not have an effect on an alternate stack
> (AFAIK).

That should be OK so long as stack-overflow handlers don't used nested 
functions. Conversely, even programs that assume SIGSTKSZ is a constant 
(and allocate fixed-size arrays on the stack or in static storage) will 
have problems on such platforms, no? Stack and static storage won't be 
executable any more than heap storage is.

> I wouldn't mind, if it's done properly:
>    - The problem with non-executable malloc'ed memory would need to be
>      solved.

Or, we could just document that stack-overflow handers can't use 
trampolines. That is a reasonable restriction for most apps, I would think.

>    - The declaration in sigsegv.h needs to stay, at least for the next
>      couple of years, because it is necessary for programs that use SIGSTKSZ
>      (such as GNU m4 and GNU clisp) to compile fine.

Why is it needed for GNU m4 and GNU clisp? I just looked at the latest 
development source code for these programs, and neither mentions 
SIGSTKSZ. GNU Emacs doesn't either (except in a configure.ac test that 
does not assume SIGSTKSZ is a constant).

The only place I found that assumed that SIGSTKSZ is a constant, was in 
Gnulib test cases. I removed that assumption by installing the first 
attached patch.

And I propose the second attached patch to remove sigsegv.h's guarantee 
that SIGSTKSZ is a constant, as glibc has gone a different way and it's 
better if Gnulib tracks glibc.

[-- Attachment #2: 0001-sigsegv-don-t-assume-SIGSTKSZ-is-a-constant.patch --]
[-- Type: text/x-patch, Size: 7848 bytes --]

From 97b0d8ad7078daafc1911ada2a53bf3d642a6cde Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 21 May 2021 14:41:42 -0700
Subject: [PATCH 1/2] =?UTF-8?q?sigsegv:=20don=E2=80=99t=20assume=20SIGSTKS?=
 =?UTF-8?q?Z=20is=20a=20constant?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* m4/sigaltstack.m4 (SV_SIGALTSTACK): Don’t attempt to override
SIGSTKSZ.  Instead, use an array that is plenty large, while
checking that it’s large enough.  Also, be consistent about
putting that array in static storage rather than on the stack.
* tests/altstack-util.h (SIGSTKSZ): Don’t define.
(MYSTACK_SIZE): New macro, used consistently instead of SIGSTKSZ.
(mystack_storage, mystack): Now static.
(prepare_alternate_stack) [defined SIGSTKSZ]:
Check that MYSTACK_SIZE is large enough.
---
 ChangeLog                                 | 13 +++++++
 m4/sigaltstack.m4                         | 44 ++++++-----------------
 tests/altstack-util.h                     | 18 ++++++----
 tests/test-sigsegv-catch-stackoverflow1.c |  4 +--
 tests/test-sigsegv-catch-stackoverflow2.c |  2 +-
 5 files changed, 39 insertions(+), 42 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index fc67bc69a..50e376176 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2021-05-21  Paul Eggert  <eggert@cs.ucla.edu>
+
+	sigsegv: don’t assume SIGSTKSZ is a constant
+	* m4/sigaltstack.m4 (SV_SIGALTSTACK): Don’t attempt to override
+	SIGSTKSZ.  Instead, use an array that is plenty large, while
+	checking that it’s large enough.  Also, be consistent about
+	putting that array in static storage rather than on the stack.
+	* tests/altstack-util.h (SIGSTKSZ): Don’t define.
+	(MYSTACK_SIZE): New macro, used instead of SIGSTKSZ.
+	(mystack_storage, mystack): Now static.
+	(prepare_alternate_stack) [SIGSTKSZ]:
+	Check that MYSTACK_SIZE is large enough.
+
 2021-05-20  Paul Eggert  <eggert@cs.ucla.edu>
 
 	fstatat: doc improvement
diff --git a/m4/sigaltstack.m4 b/m4/sigaltstack.m4
index 837191e5f..47e900079 100644
--- a/m4/sigaltstack.m4
+++ b/m4/sigaltstack.m4
@@ -1,4 +1,4 @@
-# sigaltstack.m4 serial 13
+# sigaltstack.m4 serial 14
 dnl Copyright (C) 2002-2021 Bruno Haible <bruno@clisp.org>
 dnl Copyright (C) 2008 Eric Blake <ebb9@byu.net>
 dnl This file is free software, distributed under the terms of the GNU
@@ -53,19 +53,6 @@ AC_DEFUN([SV_SIGALTSTACK],
 # include <sys/time.h>
 # include <sys/resource.h>
 #endif
-/* In glibc >= 2.34, when _GNU_SOURCE is defined, SIGSTKSZ is no longer a
-   compile-time constant.  But we need a simple constant here.  */
-#if __GLIBC__ >= 2
-# undef SIGSTKSZ
-# if defined __ia64__
-#  define SIGSTKSZ 262144
-# else
-#  define SIGSTKSZ 16384
-# endif
-#endif
-#ifndef SIGSTKSZ
-# define SIGSTKSZ 16384
-#endif
 void stackoverflow_handler (int sig)
 {
   /* If we get here, the stack overflow was caught.  */
@@ -82,7 +69,7 @@ int recurse (volatile int n)
   int sum = 0;
   return *recurse_1 (n, &sum);
 }
-char mystack[2 * SIGSTKSZ];
+char mystack[2 * (1 << 24)];
 int main ()
 {
   stack_t altstack;
@@ -97,8 +84,12 @@ int main ()
 #endif
   /* Install the alternate stack.  Use the midpoint of mystack, to guard
      against a buggy interpretation of ss_sp on IRIX.  */
-  altstack.ss_sp = mystack + SIGSTKSZ;
-  altstack.ss_size = SIGSTKSZ;
+#ifdef SIGSTKSZ
+  if (sizeof mystack / 2 < SIGSTKSZ)
+    exit (3);
+#endif
+  altstack.ss_sp = mystack + sizeof mystack / 2;
+  altstack.ss_size = sizeof mystack / 2;
   altstack.ss_flags = 0; /* no SS_DISABLE */
   if (sigaltstack (&altstack, NULL) < 0)
     exit (1);
@@ -148,19 +139,6 @@ int main ()
 #if HAVE_SYS_SIGNAL_H
 # include <sys/signal.h>
 #endif
-/* In glibc >= 2.34, when _GNU_SOURCE is defined, SIGSTKSZ is no longer a
-   compile-time constant.  But we need a simple constant here.  */
-#if __GLIBC__ >= 2
-# undef SIGSTKSZ
-# if defined __ia64__
-#  define SIGSTKSZ 262144
-# else
-#  define SIGSTKSZ 16384
-# endif
-#endif
-#ifndef SIGSTKSZ
-# define SIGSTKSZ 16384
-#endif
 volatile char *stack_lower_bound;
 volatile char *stack_upper_bound;
 static void check_stack_location (volatile char *addr)
@@ -175,14 +153,14 @@ static void stackoverflow_handler (int sig)
   char dummy;
   check_stack_location (&dummy);
 }
+char mystack[2 * (1 << 24)];
 int main ()
 {
-  char mystack[2 * SIGSTKSZ];
   stack_t altstack;
   struct sigaction action;
   /* Install the alternate stack.  */
-  altstack.ss_sp = mystack + SIGSTKSZ;
-  altstack.ss_size = SIGSTKSZ;
+  altstack.ss_sp = mystack + sizeof mystack / 2;
+  altstack.ss_size = sizeof mystack / 2;
   stack_lower_bound = (char *) altstack.ss_sp;
   stack_upper_bound = (char *) altstack.ss_sp + altstack.ss_size - 1;
   altstack.ss_flags = 0; /* no SS_DISABLE */
diff --git a/tests/altstack-util.h b/tests/altstack-util.h
index 513064506..f91072611 100644
--- a/tests/altstack-util.h
+++ b/tests/altstack-util.h
@@ -18,9 +18,7 @@
 #include <stdint.h> /* uintptr_t */
 #include <string.h> /* for memset */
 
-#ifndef SIGSTKSZ
-# define SIGSTKSZ 16384
-#endif
+#define MYSTACK_SIZE (1 << 24)
 
 /* glibc says: Users should use SIGSTKSZ as the size of user-supplied
    buffers.  We want to detect stack overflow of the alternate stack
@@ -29,12 +27,20 @@
    an unaligned pointer, to ensure the alternate stack still ends up
    aligned.  */
 #define MYSTACK_CRUMPLE_ZONE 8192
-char mystack_storage[SIGSTKSZ + 2 * MYSTACK_CRUMPLE_ZONE + 31];
-char *mystack; /* SIGSTKSZ bytes in the middle of storage. */
+static char mystack_storage[MYSTACK_SIZE + 2 * MYSTACK_CRUMPLE_ZONE + 31];
+static char *mystack; /* MYSTACK_SIZE bytes in the middle of storage. */
 
 static void
 prepare_alternate_stack (void)
 {
+#ifdef SIGSTKSZ
+  if (MYSTACK_SIZE < SIGSTKSZ)
+    {
+      size_t size = SIGSTKSZ;
+      printf ("SIGSTKSZ=%zu exceeds MYSTACK_SIZE=%d\n", size, MYSTACK_SIZE);
+      exit (1);
+    }
+#endif
   memset (mystack_storage, 's', sizeof mystack_storage);
   mystack = (char *) ((uintptr_t) (mystack_storage + MYSTACK_CRUMPLE_ZONE) | 31);
 }
@@ -51,7 +57,7 @@ check_alternate_stack_no_overflow (void)
         exit (1);
       }
   for (i = MYSTACK_CRUMPLE_ZONE; i > 0; i--)
-    if (*(mystack + SIGSTKSZ - 1 + i) != 's')
+    if (*(mystack + MYSTACK_SIZE - 1 + i) != 's')
       {
         printf ("Alternate stack was exceeded by %u bytes!!\n", i);
         exit (1);
diff --git a/tests/test-sigsegv-catch-stackoverflow1.c b/tests/test-sigsegv-catch-stackoverflow1.c
index 141dfd44d..c828ed282 100644
--- a/tests/test-sigsegv-catch-stackoverflow1.c
+++ b/tests/test-sigsegv-catch-stackoverflow1.c
@@ -105,11 +105,11 @@ main ()
 
   /* Install the stack overflow handler.  */
   if (stackoverflow_install_handler (&stackoverflow_handler,
-                                     mystack, SIGSTKSZ)
+                                     mystack, MYSTACK_SIZE)
       < 0)
     exit (2);
   stack_lower_bound = mystack;
-  stack_upper_bound = mystack + SIGSTKSZ - 1;
+  stack_upper_bound = mystack + MYSTACK_SIZE - 1;
 
   /* Save the current signal mask.  */
   sigemptyset (&emptyset);
diff --git a/tests/test-sigsegv-catch-stackoverflow2.c b/tests/test-sigsegv-catch-stackoverflow2.c
index 1db2d44fc..b94d1310b 100644
--- a/tests/test-sigsegv-catch-stackoverflow2.c
+++ b/tests/test-sigsegv-catch-stackoverflow2.c
@@ -129,7 +129,7 @@ main ()
 
   /* Install the stack overflow handler.  */
   if (stackoverflow_install_handler (&stackoverflow_handler,
-                                     mystack, SIGSTKSZ)
+                                     mystack, MYSTACK_SIZE)
       < 0)
     exit (2);
 
-- 
2.27.0


[-- Attachment #3: 0002-sigsegv-do-not-guarantee-SIGSTKSZ-is-constant.patch --]
[-- Type: text/x-patch, Size: 2462 bytes --]

From 8f14518191893837d93bf723703e2ad207ee817e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 21 May 2021 15:33:19 -0700
Subject: [PATCH 2/2] sigsegv: do not guarantee SIGSTKSZ is constant

glibc no longer guarantees that SIGSTKSZ is a constant,
and it appears that few programs still need it to be a constant.
* NEWS: Mention this.
* lib/sigsegv.in.h (SIGSTKSZ) [__GLIBC__ >= 2]: Do not redefine.
---
 ChangeLog        |  6 ++++++
 NEWS             |  3 +++
 lib/sigsegv.in.h | 11 +----------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 50e376176..843b2e267 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2021-05-21  Paul Eggert  <eggert@cs.ucla.edu>
 
+	sigsegv: do not guarantee SIGSTKSZ is constant
+	glibc no longer guarantees that SIGSTKSZ is a constant,
+	and it appears that few programs still need it to be a constant.
+	* NEWS: Mention this.
+	* lib/sigsegv.in.h (SIGSTKSZ) [__GLIBC__ >= 2]: Do not redefine.
+
 	sigsegv: don’t assume SIGSTKSZ is a constant
 	* m4/sigaltstack.m4 (SV_SIGALTSTACK): Don’t attempt to override
 	SIGSTKSZ.  Instead, use an array that is plenty large, while
diff --git a/NEWS b/NEWS
index 98931a345..e02ebf42e 100644
--- a/NEWS
+++ b/NEWS
@@ -60,6 +60,9 @@ User visible incompatible changes
 
 Date        Modules         Changes
 
+2021-05-21  sigsegv         sigsegv.h no longer guarantees that SIGSTKSZ
+                            is an integer constant expression.
+
 2021-03-21  fatal-signal    The function at_fatal_signal now returns an error
                             indicator.
 
diff --git a/lib/sigsegv.in.h b/lib/sigsegv.in.h
index e442f4dfa..e5fac651b 100644
--- a/lib/sigsegv.in.h
+++ b/lib/sigsegv.in.h
@@ -37,20 +37,11 @@
 #endif
 
 /* Correct the value of SIGSTKSZ on some systems.
-   glibc >= 2.34: When _GNU_SOURCE is defined, SIGSTKSZ is no longer a
-   compile-time constant.  But most programs need a simple constant.
    AIX 64-bit: original value 4096 is too small.
    HP-UX: original value 8192 is too small.
    Solaris 11/x86_64: original value 8192 is too small.  */
+
 #include <signal.h>
-#if __GLIBC__ >= 2
-# undef SIGSTKSZ
-# if defined __ia64__
-#  define SIGSTKSZ 262144
-# else
-#  define SIGSTKSZ 65536
-# endif
-#endif
 #if defined _AIX && defined _ARCH_PPC64
 # undef SIGSTKSZ
 # define SIGSTKSZ 8192
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-21 22:49     ` Paul Eggert
@ 2021-05-22 22:59       ` Bruno Haible
  2021-05-23  5:12         ` Paul Eggert
  2021-05-23  0:00       ` sigsegv.h interface Bruno Haible
  1 sibling, 1 reply; 11+ messages in thread
From: Bruno Haible @ 2021-05-22 22:59 UTC (permalink / raw)
  To: Paul Eggert; +Cc: bug-gnulib, Eric Blake

[-- Attachment #1: Type: text/plain, Size: 4029 bytes --]

Hi Paul,

What you are doing now is to deconstruct
  - GCC extensions, and
  - POSIX.

1) Regarding GCC extensions:

> > malloc'ed memory is not executable. But GCC's trampolines (used by nested
> > functions) require an executable stack. When an object file has such nested
> > functions, the toolchain arranges for the executable to ask the OS to
> > allocate an executable stack. But that only has an effect on the primary
> > stack of each thread. It does not have an effect on an alternate stack
> > (AFAIK).
> 
> That should be OK so long as stack-overflow handlers don't used nested 
> functions.

Wait a moment. GCC's nested functions are a documented GCC feature [1] for
ca. 30 years. When "security" became an important topic and people demanded
non-executable stacks for web browsers, GCC and the binutils were extended
in such a way that
  - on one hand, programs that use GCC nested functions (on most architectures)
    got a bit set in the executable, which has the consequence of an
    executable stack,
  - on the other hand, programs that don't need this — such as web browsers —
    can have a non-executable stack.

Now, you are arguing "let's ignore whether programs use nested functions".
As if that wasn't a documented way, in the GNU system, of writing programs.??

> > I wouldn't mind, if it's done properly:
> >    - The problem with non-executable malloc'ed memory would need to be
> >      solved.
> 
> Or, we could just document that stack-overflow handers can't use 
> trampolines. That is a reasonable restriction for most apps, I would think.

It adds complexity to the calling program. The developer has to check whether
their code uses nested functions.

2) Regarding POSIX:

> The only place I found that assumed that SIGSTKSZ is a constant, was in 
> Gnulib test cases. I removed that assumption by installing the first 
> attached patch.

By doing so, you moved the Gnulib code away from POSIX. In so many occasions,
you voted for making Gnulib closer to POSIX, and that is a significant
reason why Gnulib is successful.

Here,
  - The current POSIX specifies a constant SIGSTKSZ,
  - Eric Blake is proposing that POSIX drops the constant-ness, and that
    SIGSTKSZ becomes a macro that merely expands to an expression. [2]

So, being close to POSIX would mean to use SIGSTKSZ for the allocation
of an alternate stack, most likely through malloc.

Whereas your patch removes all traces of SIGSTKSZ and hardcodes a constant
(1 << 24) in various places instead.

The point of your patch appears to be to proactively react to other systems
(FreeBSD, Solaris, etc.) to follow glibc's move and make SIGSTKSZ non-
constant. But then, still, it would have been more maintainable to write

  /* SIGSTKSZ can no longer be guaranteed to be a constant.  Until POSIX
     clarifies it, use a large constant anyway.  */
  #undef SIGSTKSZ
  #define SIGSTKSZ (1 << 24)

3) The approach you used now, to use a 16 MB space for sigaltstack(),
is not portable to multithreaded situations. For example, I have a test
program (attached) that I consider to add as a unit test to gnulib.
It uses stack space from the thread itself as alternate signal stack.
That is a perfectly natural thing to do, because
  - the permissions of the alternate stack are automatically right,
  - this alternate stack gets deallocated automatically when the thread
    dies (no memory leak).
But allocating 16 MB on a thread's stack is not portable. On Solaris/SPARC,
the main thread's size is only 8 MB.

Really, my goals here are that the unit tests in Gnulib
  - use POSIX the best they can,
  - give a coding pattern that people can use in their applications,
    be they single-threaded or multi-threaded, and interoperable with
    whatever GCC extensions the programmer wants.

Bruno

[1] https://gcc.gnu.org/onlinedocs/gcc-11.1.0/gcc/Nested-Functions.html
[2] https://sourceware.org/pipermail/libc-alpha/2021-March/123553.html

[-- Attachment #2: sigaltstack-per-thread.c --]
[-- Type: text/x-csrc, Size: 4544 bytes --]

/* Test whether sigaltstack works per thread.  */

#include <errno.h>
#include <pthread.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/select.h>
#include <sys/time.h>

/* Correct the value of SIGSTKSZ on some systems.
   AIX 64-bit: original value 4096 is too small.
   HP-UX: original value 8192 is too small.  */
#if defined _AIX && defined _ARCH_PPC64
# undef SIGSTKSZ
# define SIGSTKSZ 8192
#endif
#if defined __hpux || (defined __sun && (defined __x86_64__ || defined __amd64__))
# undef SIGSTKSZ
# define SIGSTKSZ 16384
#endif

static void*
thread2_func (void *ignored)
{
  stack_t current;
  stack_t alt_stack;

  char mystack_storage[SIGSTKSZ + 31];
  char *mystack = (char *)((uintptr_t) mystack_storage | 31);

  fprintf (stderr, "thread2 running\n");

  if (sigaltstack (NULL, &current) < 0)
    {
      perror ("thread2 sigaltstack");
      exit (1);
    }
  fprintf (stderr, "thread2 sigaltstack: enabled=%s onstack=%s sp=0x%lx size=0x%lx\n",
           (current.ss_flags & SS_DISABLE) ? "no" : "yes",
           (current.ss_flags & SS_ONSTACK) ? "yes" : "no",
           (unsigned long) current.ss_sp,
           (unsigned long) current.ss_size);

  if ((current.ss_flags & SS_DISABLE) == 0 && current.ss_size > 0)
    {
      fprintf (stderr, "thread2 sigaltstack already enabled! Per-thread not supported.\n");
      exit (1);
    }

  alt_stack.ss_flags = 0;
  alt_stack.ss_sp = mystack;
  alt_stack.ss_size = SIGSTKSZ;
  if (sigaltstack (&alt_stack, NULL) < 0)
    {
      perror ("thread2 sigaltstack set");
      exit (1);
    }
  fprintf (stderr, "thread2 alternate stack installed\n");

  if (sigaltstack (NULL, &current) < 0)
    {
      perror ("thread2 sigaltstack");
      exit (1);
    }
  fprintf (stderr, "thread2 sigaltstack now: enabled=%s onstack=%s sp=0x%lx size=0x%lx\n",
           (current.ss_flags & SS_DISABLE) ? "no" : "yes",
           (current.ss_flags & SS_ONSTACK) ? "yes" : "no",
           (unsigned long) current.ss_sp,
           (unsigned long) current.ss_size);

  return 0;
}

int
main ()
{
  stack_t alt_stack;
  stack_t current;
  stack_t last;
  pthread_t thread2;
  struct timeval sleeptime;

  char mystack_storage[SIGSTKSZ + 31];
  char *mystack = (char *)((uintptr_t) mystack_storage | 31);

  alt_stack.ss_flags = 0;
  alt_stack.ss_sp = mystack;
  alt_stack.ss_size = SIGSTKSZ;
  if (sigaltstack (&alt_stack, NULL) < 0)
    {
      perror ("main-thread sigaltstack set");
      exit (1);
    }
  fprintf (stderr, "alternate stack installed\n");

  if (sigaltstack (NULL, &current) < 0)
    {
      perror ("main-thread sigaltstack");
      exit (1);
    }
  fprintf (stderr, "main-thread sigaltstack: enabled=%s onstack=%s sp=0x%lx size=0x%lx\n",
           (current.ss_flags & SS_DISABLE) ? "no" : "yes",
           (current.ss_flags & SS_ONSTACK) ? "yes" : "no",
           (unsigned long) current.ss_sp,
           (unsigned long) current.ss_size);
  last = current;

  if (pthread_create (&thread2, NULL, thread2_func, NULL))
    {
      fprintf (stderr, "creating of thread2 failed\n");
      exit (1);
    }

  /* Sleep 2 sec.  */
  sleeptime.tv_sec = 2;
  sleeptime.tv_usec = 0;
  if (select (0, NULL, NULL, NULL, &sleeptime) < 0)
    {
      perror ("main-thread sleep");
      exit (1);
    }

  if (sigaltstack (NULL, &current) < 0)
    {
      perror ("main-thread sigaltstack");
      exit (1);
    }
  fprintf (stderr, "main-thread sigaltstack: enabled=%s onstack=%s sp=0x%lx size=0x%lx\n",
           (current.ss_flags & SS_DISABLE) ? "no" : "yes",
           (current.ss_flags & SS_ONSTACK) ? "yes" : "no",
           (unsigned long) current.ss_sp,
           (unsigned long) current.ss_size);
  if (!(current.ss_flags == last.ss_flags
        && current.ss_sp == last.ss_sp
        && current.ss_size == last.ss_size))
    {
      fprintf (stderr, "main-thread sigaltstack has changed!\n");
      exit (1);
    }

  fprintf (stderr, "SUPPORTED\n");

  exit (0);
}

/* Results:
   Linux          SUPPORTED
   Hurd           SUPPORTED although "enabled=yes onstack=no sp=0x0 size=0x0"
   Mac OS X 10.5  SUPPORTED
   FreeBSD 11     SUPPORTED
   NetBSD 7       thread2 sigaltstack already enabled! Per-thread not supported.
   NetBSD 8       SUPPORTED
   OpenBSD 6.0    SUPPORTED
   AIX 7.1        SUPPORTED
   HP-UX 11.31    SUPPORTED
   IRIX 6.5       thread2 sigaltstack already enabled! Per-thread not supported.
   Solaris 9, 10  SUPPORTED
   Haiku          SUPPORTED
   Cygwin         SUPPORTED
 */

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: sigsegv.h interface
  2021-05-21 22:49     ` Paul Eggert
  2021-05-22 22:59       ` Bruno Haible
@ 2021-05-23  0:00       ` Bruno Haible
  2021-05-23  5:14         ` Paul Eggert
  1 sibling, 1 reply; 11+ messages in thread
From: Bruno Haible @ 2021-05-23  0:00 UTC (permalink / raw)
  To: Paul Eggert, bug-gnulib

Hi Paul,

I wrote:
> >    - The declaration in sigsegv.h needs to stay, at least for the next
> >      couple of years, because it is necessary for programs that use SIGSTKSZ
> >      (such as GNU m4 and GNU clisp) to compile fine.
> 
> Why is it needed for GNU m4 and GNU clisp? I just looked at the latest 
> development source code for these programs, and neither mentions 
> SIGSTKSZ. GNU Emacs doesn't either (except in a configure.ac test that 
> does not assume SIGSTKSZ is a constant).
> ...
> And I propose the second attached patch to remove sigsegv.h's guarantee 
> that SIGSTKSZ is a constant, as glibc has gone a different way and it's 
> better if Gnulib tracks glibc.

sigsegv.h is the public interface of GNU libsigsegv too.

Apparently my memory was wrong regarding GNU m4 and GNU clisp. So, let me look
at all relevant programs, how they allocate their alternate stacks.

$ apt-cache rdepends libsigsegv2
libsigsegv2
Reverse Depends:
  libsigsegv-dev
  scheme2c
  maude
  libgst7
  clisp
  asymptote
  m4
  gawk

scheme2c
--------
Referenced in scheme2c-2012.10.14/scrt/cio.c.
The program is single-threaded.
The alternate stack is statically allocated, of size 16 KB.

maude
-----
Referenced in maude-3.1/src/Mixfix/interact.cc.
The program is single-threaded.
The alternate stack is statically allocated, of size SIGSTKSZ.

GNU smalltalk
-------------
Uses only a SIGSEGV handler, no stack overflow handler.

GNU clisp
---------
Referenced in clisp/src/spvw_sigsegv.d.
The program is single-threaded.
The alternate stack is allocated on the stack of the main thread,
using alloca(), of size 16 KB.

asymptote
---------
Referenced in asymptote-2.70/main.cc.
The program is multi-threaded.
An alternate stack is allocated only for the main thread, on the stack
of the main thread, as a local variable that goes out-of-scope (weird,
but OK), of size 16 KB.

GNU m4
------
Referenced in m4/src/m4.c.
The program is single-threaded.
The alternate stack (in c-stack.c) is statically allocated, of size 64 KB.

GNU gawk
--------
Referenced in gawk/main.c.
The program is single-threaded.
The alternate stack is allocated through malloc(), of size 16 KB.


So, there are indeed few users of SIGSTKSZ among the users of GNU libsigsegv,
namely only 'maude'. And since 'maude' does not use Gnulib, it is possible
to move forward with the API of sigsegv.h faster in Gnulib than it is possible
in libsigsegv.

But please hold off from doing this patch for now. First, I need to document
things properly, both in the Gnulib documentation and in the libsigsegv
documentation. In particular:
  * API differences between gnulib's sigsegv.h and libsigsegv's sigsegv.h
    need to be documented.
  * It needs to be documented from where the alternate stack should be
    allocated, and with which size (since 16 KB is too small on some platforms,
    and 16 MB is too large for multithreaded apps).

Bruno



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-22 22:59       ` Bruno Haible
@ 2021-05-23  5:12         ` Paul Eggert
  2021-05-23 12:14           ` Bruno Haible
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Eggert @ 2021-05-23  5:12 UTC (permalink / raw)
  To: Bruno Haible; +Cc: bug-gnulib, Eric Blake

On 5/22/21 3:59 PM, Bruno Haible wrote:

> Now, you are arguing "let's ignore whether programs use nested functions".

Not at all. All I'm saying is, stack-overflow handlers shouldn't call 
via pointers to nested functions.

There are already several restrictions on stack-overflow handlers; for 
example, you can't call 'exit'. A restriction against calling via 
pointers to nested functions is just another restriction - one that's 
easier to comply with than the 'exit' restriction, in my experience.

If stack-overflow handlers absolutely can't live without calling via 
pointers to nested functions, one can work around the problem by 
compiling with gcc -fno-trampolines. However, I don't recommend this - 
just as there are a lot of other common features that I don't recommend 
in stack-overflow handlers, such as recursion.

> It adds complexity to the calling program. The developer has to check whether
> their code uses nested functions.

Not all code, just stack-overflow handlers. It should be reasonable to 
ask developers to do that, as stack-overflow handlers should be simple 
and easy to audit anyway.


>    - The current POSIX specifies a constant SIGSTKSZ,
>    - Eric Blake is proposing that POSIX drops the constant-ness, and that
>      SIGSTKSZ becomes a macro that merely expands to an expression. [2]

It's more than that. glibc has already changed to drop the 
constant-ness. Gnulib should of course be portable to POSIX, but it's 
even more important for Gnulib to be portable to GNU.

> being close to POSIX would mean to use SIGSTKSZ for the allocation
> of an alternate stack, most likely through malloc.

That would also be a reasonable change to the test cases. I didn't do 
that, partly because I thought it simpler just to allocate an enormous 
alternate signal stack, partly because I wasn't yet entirely clear on 
exactly when malloc is bad for allocating alternate signal stacks so I 
wanted to avoid malloc entirely. But if you'd prefer that the test cases 
use malloc I can rewrite them to do that.

> it would have been more maintainable to write
> 
>    /* SIGSTKSZ can no longer be guaranteed to be a constant.  Until POSIX
>       clarifies it, use a large constant anyway.  */
>    #undef SIGSTKSZ
>    #define SIGSTKSZ (1 << 24)

The code I wrote attempted to do the right thing even on (hypothetical) 
platforms where MINSIGSTKSZ exceeds 1<<24. A simple #undef/#define 
wouldn't do that. Again, using malloc should fix this issue in a less 
hoggish way.

> 3) The approach you used now, to use a 16 MB space for sigaltstack(),
> is not portable to multithreaded situations. For example, I have a test
> program (attached) that I consider to add as a unit test to gnulib.
> It uses stack space from the thread itself as alternate signal stack.
> That is a perfectly natural thing to do, because
>    - the permissions of the alternate stack are automatically right,
>    - this alternate stack gets deallocated automatically when the thread
>      dies (no memory leak).
> But allocating 16 MB on a thread's stack is not portable. On Solaris/SPARC,
> the main thread's size is only 8 MB.

The code I wrote didn't have to (and didn't) attempt to address this 
scenario. For something like that, have you looked at Florian Weimer's 
recent <sys/cstack.h> proposal for glibc 
<https://sourceware.org/pipermail/libc-alpha/2021-May/126605.html>? If 
that API is added to glibc and can be simulated by Gnulib well-enough on 
other platforms, that should be a good way to address the problem that 
you mention.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: sigsegv.h interface
  2021-05-23  0:00       ` sigsegv.h interface Bruno Haible
@ 2021-05-23  5:14         ` Paul Eggert
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Eggert @ 2021-05-23  5:14 UTC (permalink / raw)
  To: Bruno Haible, bug-gnulib

On 5/22/21 5:00 PM, Bruno Haible wrote:
> it is possible
> to move forward with the API of sigsegv.h faster in Gnulib than it is possible
> in libsigsegv.

Yes, that was the path I was assuming.
> But please hold off from doing this patch for now. 

Will do.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-23  5:12         ` Paul Eggert
@ 2021-05-23 12:14           ` Bruno Haible
  2021-05-24  2:40             ` Paul Eggert
  0 siblings, 1 reply; 11+ messages in thread
From: Bruno Haible @ 2021-05-23 12:14 UTC (permalink / raw)
  To: Paul Eggert; +Cc: bug-gnulib, Eric Blake

[-- Attachment #1: Type: text/plain, Size: 1827 bytes --]

Paul Eggert wrote:
> > Now, you are arguing "let's ignore whether programs use nested functions".
> 
> Not at all. All I'm saying is, stack-overflow handlers shouldn't call 
> via pointers to nested functions.
> 
> There are already several restrictions on stack-overflow handlers; for 
> example, you can't call 'exit'. A restriction against calling via 
> pointers to nested functions is just another restriction - one that's 
> easier to comply with than the 'exit' restriction, in my experience.

I agree that these handlers are _usually_ small and don't have nested
functions. But a restriction is a restriction, and should be documented
as such. Find attached a proposed patch.

> If stack-overflow handlers absolutely can't live without calling via 
> pointers to nested functions, one can work around the problem by 
> compiling with gcc -fno-trampolines.

No, "gcc -fno-trampolines" doesn't get rid of the need to have an executable
stack. In the attached sample code gcc-closure.c, GCC 11.1 produces identical
code with "gcc -fno-trampolines" than with "gcc -ftrampolines".

> > being close to POSIX would mean to use SIGSTKSZ for the allocation
> > of an alternate stack, most likely through malloc.
> 
> That would also be a reasonable change to the test cases. I didn't do 
> that, partly because I thought it simpler just to allocate an enormous 
> alternate signal stack, partly because I wasn't yet entirely clear on 
> exactly when malloc is bad for allocating alternate signal stacks so I 
> wanted to avoid malloc entirely. But if you'd prefer that the test cases 
> use malloc I can rewrite them to do that.

Thanks for the offer. I think we should get clarity first, regarding which
of the three approaches (static allocation, malloc, alloca) is preferrable.
I'll try to write documentation for it.

Bruno

[-- Attachment #2: c-stack-doc.diff --]
[-- Type: text/x-patch, Size: 849 bytes --]

diff --git a/lib/c-stack.h b/lib/c-stack.h
index 56d74f1..8bf773c 100644
--- a/lib/c-stack.h
+++ b/lib/c-stack.h
@@ -34,8 +34,10 @@
    A null ACTION acts like an action that does nothing.
 
    ACTION must be async-signal-safe.  ACTION together with its callees
-   must not require more than 64 KiB of stack space.  Also,
-   ACTION should not call longjmp, because this implementation does
+   must not require more than 64 KiB of stack space.  ACTION must not create
+   nested functions <https://gcc.gnu.org/onlinedocs/gcc/Nested-Functions.html>,
+   because this implementation does not guarantee an executable stack.
+   Also, ACTION should not call longjmp, because this implementation does
    not guarantee that it is safe to return to the original stack.
 
    This function may install a handler for the SIGSEGV signal or for the SIGBUS

[-- Attachment #3: gcc-closure.c --]
[-- Type: text/x-csrc, Size: 697 bytes --]

/* Test whether a lexical closure created by GCC requires an executable stack.
   Compile this file with
     cross <target> gcc -O2 -S -fomit-frame-pointer gcc-closure.c
 */

extern int callback (void (*) (void));

int foo (void)
{
  int x = 0;

  void increment_x (void) { x++; }

  callback (increment_x);

  return x;
}

/* Result:
   i386       yes
   x86_64     yes
   m68k       yes
   mips       yes
   mips64     yes
   sparc      yes
   sparc64    yes
   alpha      yes
   hppa       ?
   hppa64     ?
   arm        yes
   arm64      yes
   powerpc    no
   powerpc64  no
   powerpc64-elfv2 no
   ia64       no
   s390       yes
   s390x      yes
   riscv32    yes
   riscv64    yes
 */

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-23 12:14           ` Bruno Haible
@ 2021-05-24  2:40             ` Paul Eggert
  2021-05-24 10:37               ` Bruno Haible
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Eggert @ 2021-05-24  2:40 UTC (permalink / raw)
  To: Bruno Haible; +Cc: bug-gnulib, Eric Blake

On 5/23/21 5:14 AM, Bruno Haible wrote:

> I agree that these handlers are _usually_ small and don't have nested
> functions. But a restriction is a restriction, and should be documented
> as such. Find attached a proposed patch.

Thanks, that looks good except please change "create nested functions" 
to "create and then use nested functions", as it's OK to for a 
stack-overflow handler to create a nested function without using it.

Should a similar restriction be documented for libsigsegv, or is that 
library immune to this problem?

> No, "gcc -fno-trampolines" doesn't get rid of the need to have an executable
> stack. In the attached sample code gcc-closure.c, GCC 11.1 produces identical
> code with "gcc -fno-trampolines" than with "gcc -ftrampolines".

Ouch, I didn't know that -fno-trampolines doesn't work for C or C++. The 
GCC manual implies otherwise. I filed a GCC doc bug report here:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100735

> I think we should get clarity first, regarding which
> of the three approaches (static allocation, malloc, alloca) is preferrable.
> I'll try to write documentation for it.

If I understand you correctly, all three approaches will work if 
stack-overflow handlers don't create and use nested functions, and 
alloca will work even if nested functions are used.

The main problem is: what size to allocate for the alternate stack? If I 
understand recent glibc discussions correctly, the "right" size might 
vary dynamically even within a thread, which makes any approach 
problematic if it doesn't want to allocate an alternate stack large 
enough to be future-proof. This is where I hope Florian Weimer's 
proposed approach will rescue us somehow.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: sigsegv, c-stack: Avoid compilation error with glibc >= 2.34
  2021-05-24  2:40             ` Paul Eggert
@ 2021-05-24 10:37               ` Bruno Haible
  0 siblings, 0 replies; 11+ messages in thread
From: Bruno Haible @ 2021-05-24 10:37 UTC (permalink / raw)
  To: Paul Eggert; +Cc: bug-gnulib, Eric Blake

Paul Eggert wrote:
> Thanks, that looks good except please change "create nested functions" 
> to "create and then use nested functions", as it's OK to for a 
> stack-overflow handler to create a nested function without using it.

Yup. I had a thinko there.

> Should a similar restriction be documented for libsigsegv, or is that 
> library immune to this problem?

Right, it will need to be documented for libsigsegv as well.


2021-05-24  Bruno Haible  <bruno@clisp.org>

	c-stack: Document another restriction.
	Triggered by a discussion with Paul Eggert.
	* lib/c-stack.h: Mention that ACTION should not use nested functions.

diff --git a/lib/c-stack.h b/lib/c-stack.h
index 56d74f1..a9a8b13 100644
--- a/lib/c-stack.h
+++ b/lib/c-stack.h
@@ -33,10 +33,15 @@
 
    A null ACTION acts like an action that does nothing.
 
-   ACTION must be async-signal-safe.  ACTION together with its callees
-   must not require more than 64 KiB of stack space.  Also,
-   ACTION should not call longjmp, because this implementation does
-   not guarantee that it is safe to return to the original stack.
+   Restrictions:
+   - ACTION must be async-signal-safe.
+   - ACTION together with its callees must not require more than 64 KiB of
+     stack space.
+   - ACTION must not create and then invoke nested functions
+     <https://gcc.gnu.org/onlinedocs/gcc/Nested-Functions.html>, because
+     this implementation does not guarantee an executable stack.
+   - ACTION should not call longjmp, because this implementation does not
+     guarantee that it is safe to return to the original stack.
 
    This function may install a handler for the SIGSEGV signal or for the SIGBUS
    signal or exercise other system dependent exception handling APIs.  */



^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-05-24 10:37 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-17  0:33 sigsegv, c-stack: Avoid compilation error with glibc >= 2.34 Bruno Haible
2021-05-17  2:45 ` Paul Eggert
2021-05-18 17:34   ` Bruno Haible
2021-05-21 22:49     ` Paul Eggert
2021-05-22 22:59       ` Bruno Haible
2021-05-23  5:12         ` Paul Eggert
2021-05-23 12:14           ` Bruno Haible
2021-05-24  2:40             ` Paul Eggert
2021-05-24 10:37               ` Bruno Haible
2021-05-23  0:00       ` sigsegv.h interface Bruno Haible
2021-05-23  5:14         ` Paul Eggert

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