Re: stackovf implementation

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Eric Blake on 6/20/2006 1:16 PM:
>>> If that compiles warning-free, then I will see about changing stackovf to
>>> favor sigaltstack over sigstack; I can find a Linux machine to test on.
> 
> After looking more at POSIX, and testing on a Solaris 8 machine, I propose
> the following patch.  Santiago, can you test it on ia64, and see if it
> gets rid of the warning you were seeing?
> 
> 2006-06-20  Eric Blake  <address@hidden>
> 
>       Avoid obsolete sigstack when POSIX sigaltstack is available:

I'm porting this to head as follows:

2006-07-22  Eric Blake  <address@hidden>

        * src/main.c (stackovf_handler): Document the problems in our
        overflow handler.
        * src/stackovf.c: Forward port changes in branch to use POSIX
        sa_sigaction when available.
        (process_sigsegv): Avoid buffer overrun when error string is
        translated, although the fact that we translate in a signal
        handler is still a bug.
        * ltdl/m4/stackovf.m4 (M4_SYS_STACKOVF): Forward port changes
        from branch to modernize checks.

I also noticed, while testing on a Linux box, where I had rlimit claiming
the stack was 0xa00000 bytes, and setup_stackovf_trap was called with the
stack pointer at 0xbfecd000, but the stack overflow didn't happen until I
crossed 0xbf400000.  This was a large enough difference (0xcd000) that it
fell outside the range of the default STACKOVF_DETECT of 16k, and m4
exited with the normal segv signal rather than detecting the overflow.  I
wish it were easier to detect stack overflow.

One thought I had to make overflow detection more accurate would be to
constantly update our notion of the edge of the stack every time we
encounter a deeper call to expand_macro (where we are already implementing
the -L limit), and on segv, see if the fault is within STACKOVF_DETECT of
where expand_macro last noticed the stack at.  I'm not sure how much this
might slow things down, but it seems simple to implement.

Another thing I wonder about is our use of gettext and error inside the
signal handler (where we are just asking for problems, since these invoke
non-signal-safe functions).  It might be better to use
sigsetjump/siglongjump to get back to a non-signal context, and from there
issue the appropriate error message.  But for now, I just documented the
issue.

- --
Life is short - so eat dessert first!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFEwqhS84KuGfSFAYARAs5GAJ4+Zgl3FWadqu0wn4UnAlQeqRikQACgxx4+
JOmadOrUuIrlf0oP+6tRLyI=
=TK6g
-----END PGP SIGNATURE-----

Index: ltdl/m4/stackovf.m4
===================================================================
RCS file: /sources/m4/m4/ltdl/m4/stackovf.m4,v
retrieving revision 1.2
diff -u -p -r1.2 stackovf.m4
--- ltdl/m4/stackovf.m4 16 Jun 2006 03:51:29 -0000      1.2
+++ ltdl/m4/stackovf.m4 22 Jul 2006 21:51:51 -0000
@@ -18,56 +18,64 @@
 # the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
 # Boston, MA 02110-1301, USA.
 
-# serial 3
+# serial 4
 
 # M4_SYS_STACKOVF
 # ---------------
 AC_DEFUN([M4_SYS_STACKOVF],
-[AC_PREREQ([2.56])dnl We use the new compiler based header checking in 2.56
+[AC_PREREQ([2.60])dnl We use the _ONCE variants
 AC_REQUIRE([AC_TYPE_SIGNAL])dnl
 
-AC_CHECK_HEADERS([siginfo.h], [], [], [AC_INCLUDES_DEFAULT])
-AC_CHECK_FUNCS([sigaction sigaltstack sigstack sigvec])
-AC_MSG_CHECKING([if stack overflow is detectable])
+AC_CHECK_HEADERS_ONCE([siginfo.h])
+AC_CHECK_FUNCS_ONCE([sigaction sigaltstack sigstack sigvec])
 # Code from Jim Avera <address@hidden>.
 # stackovf.c requires:
 #  1. Either sigaction with SA_ONSTACK, or sigvec with SV_ONSTACK
 #  2. Either sigaltstack or sigstack
 #  3. getrlimit, including support for RLIMIT_STACK
-use_stackovf=no
+AC_CACHE_CHECK([if stack overflow is detectable], [M4_cv_use_stackovf],
+[M4_cv_use_stackovf=no
 if test "$ac_cv_func_sigaction" = yes || test "$ac_cv_func_sigvec" = yes; then
   if test "$ac_cv_func_sigaltstack" = yes || test "$ac_cv_func_sigstack" = 
yes; then
     AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include <sys/time.h>
 #include <sys/resource.h>
-#include <signal.h>]],
-      [[struct rlimit r; int i; getrlimit (RLIMIT_STACK, &r)
+#include <signal.h>
+]], [[struct rlimit r; getrlimit (RLIMIT_STACK, &r);
 #if (!defined(HAVE_SIGACTION) || !defined(SA_ONSTACK)) \
     && (!defined(HAVE_SIGVEC) || !defined(SV_ONSTACK))
 choke me               /* SA_ONSTACK and/or SV_ONSTACK are not defined */
-#endif]])],
-      [use_stackovf=yes])
+#endif
+]])], [M4_cv_use_stackovf=yes])
   fi
-fi
-AC_MSG_RESULT([$use_stackovf])
+fi])
 
-AM_CONDITIONAL([STACKOVF], [test "$use_stackovf" = yes])
-if test "$use_stackovf" = yes; then
+AM_CONDITIONAL([STACKOVF], [test "$M4_cv_use_stackovf" = yes])
+if test "$M4_cv_use_stackovf" = yes; then
   AC_DEFINE([USE_STACKOVF], [1],
     [Define to 1 if using stack overflow detection.])
-  AC_EGREP_HEADER([rlim_t], [sys/resource.h], [],
-       [AC_DEFINE([rlim_t], [int],
-       [Define to int if rlim_t is not defined in <sys/resource.h>.])])
-  AC_EGREP_HEADER([stack_t], [signal.h], [],
-       [AC_DEFINE([stack_t], [struct sigaltstack],
-       [Define to struct sigaltstack if stack_t is not defined in 
<sys/signal.h>.])])
-  AC_EGREP_HEADER([sigcontext], [signal.h],
-       [AC_DEFINE([HAVE_SIGCONTEXT], [1],
-       [Define to 1 if <signal.h> declares sigcontext.])])
-  AC_EGREP_HEADER([siginfo_t], [signal.h],
-                 [AC_DEFINE([HAVE_SIGINFO_T], [1],
-       [Define to 1 if <signal.h> declares siginfo_t.])])
+  AC_CHECK_TYPES([rlim_t], [],
+    [AC_DEFINE([rlim_t], [int],
+      [Define to int if rlim_t is not defined in sys/resource.h])],
+    [[#include <sys/resource.h>
+]])
+  AC_CHECK_TYPES([stack_t], [],
+    [AC_DEFINE([stack_t], [struct sigaltstack],
+      [Define to struct sigaltstack if stack_t is not in signal.h])],
+    [[#include <signal.h>
+]])
+  AC_CHECK_TYPES([sigcontext], [], [], [[#include <signal.h>
+]])
+  AC_CHECK_TYPES([siginfo_t], [], [], [[#include <signal.h>
+#if HAVE_SIGINFO_H
+# include <siginfo.h>
+#endif
+]])
+  AC_CHECK_MEMBERS([struct sigaction.sa_sigaction], [], [],
+[[#include <signal.h>
+]])
 
-  AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <signal.h>]],
+  AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <signal.h>
+]],
            [[struct sigaltstack x; x.ss_base = 0;]])],
            [AC_DEFINE([ss_sp], [ss_base],
            [Define to ss_base if stack_t has ss_base instead of ss_sp.])])
Index: src/main.c
===================================================================
RCS file: /sources/m4/m4/src/main.c,v
retrieving revision 1.68
diff -u -p -r1.68 main.c
--- src/main.c  14 Jul 2006 15:27:34 -0000      1.68
+++ src/main.c  22 Jul 2006 21:51:52 -0000
@@ -73,6 +73,10 @@ print_program_name_CB (void)
 static void
 stackovf_handler (void)
 {
+  /* FIXME - calling gettext and error inside a signal handler is dangerous,
+     since these functions invoke functions that are not signal-safe.  We
+     are sort of justified by the fact that we will exit and never return,
+     but this should really be fixed.  */
   M4ERROR ((EXIT_FAILURE, 0,
            _("Stack overflow.  (Infinite define recursion?)")));
 }
Index: src/stackovf.c
===================================================================
RCS file: /sources/m4/m4/src/stackovf.c,v
retrieving revision 1.18
diff -u -p -r1.18 stackovf.c
--- src/stackovf.c      22 Jul 2006 19:23:04 -0000      1.18
+++ src/stackovf.c      22 Jul 2006 21:51:52 -0000
@@ -91,6 +91,13 @@
 # include <siginfo.h>
 #endif
 
+#ifndef SA_RESETHAND
+# define SA_RESETHAND 0
+#endif
+#ifndef SA_SIGINFO
+# define SA_SIGINFO 0
+#endif
+
 #ifndef SIGSTKSZ
 # define SIGSTKSZ 8192
 #endif
@@ -108,7 +115,6 @@
 # define STACKOVF_DETECT 16384
 #endif
 
-/* Giving a hand to ansi2knr...  */
 typedef void (*handler_t) (void);
 
 #if defined(__ultrix) && defined(__vax)
@@ -147,21 +153,22 @@ static handler_t stackovf_handler;
    message and abort with a core dump.  This only occurs on systems which
    provide no information, but is better than nothing.  */
 
-#define PARAM_STACKOVF ((const char *) 1)
-#define PARAM_NOSTACKOVF ((const char *) 2)
+#define PARAM_STACKOVF ((const char *) (1 + STACKOVF_DETECT))
+#define PARAM_NOSTACKOVF ((const char *) (2 + STACKOVF_DETECT))
 
 static void
 process_sigsegv (int signo, const char *p)
 {
-  long diff;
+  ptrdiff_t diff;
   diff = (p - stackend);
 
 #ifdef DEBUG_STKOVF
   {
-    char buf[140];
+    char buf[200];
 
-    sprintf (buf, "process_sigsegv: p=%#lx stackend=%#lx diff=%ld bot=%#lx\n",
-            (long) p, (long) stackend, (long) diff, (long) stackbot);
+    sprintf (buf,
+            "process_sigsegv: p=%p stackend=%p diff=%" PRIdPTR "bot=%p\n",
+            p, stackend, diff, stackbot);
     write (2, buf, strlen (buf));
   }
 #endif
@@ -170,11 +177,18 @@ process_sigsegv (int signo, const char *
     {
       if ((long) sbrk (8192) == (long) -1)
        {
+         const char *cp;
 
          /* sbrk failed.  Assume the RLIMIT_VMEM prevents expansion even
             if the stack limit has not been reached.  */
 
-         write (2, _("VMEM limit exceeded?\n"), 21);
+         /* FIXME - calling gettext inside a signal handler is
+            dangerous, since it can call malloc, which is not signal
+            safe.  We can sort of justify it by the fact that this
+            handler is designed to exit() the program, but it could
+            really use a better fix.  */
+         cp = _("VMEM limit exceeded?\n");
+         write (2, cp, strlen (cp));
          p = PARAM_STACKOVF;
        }
       if (diff >= -STACKOVF_DETECT && diff <= STACKOVF_DETECT)
@@ -196,6 +210,8 @@ process_sigsegv (int signo, const char *
     {
       const char *cp;
 
+      /* FIXME - calling gettext inside a signal handler is dangerous,
+        since it can call malloc, which is not signal safe.  */
       cp = _("\
 Memory bounds violation detected (SIGSEGV).  Either a stack overflow\n\
 occurred, or there is a bug in ");
@@ -211,20 +227,31 @@ occurred, or there is a bug in ");
   signal (signo, SIG_DFL);
 }
 
-#if HAVE_SIGINFO_H || HAVE_SIGINFO_T
+#if HAVE_STRUCT_SIGACTION_SA_SIGACTION
+
+/* POSIX.  */
+
+static void
+sigsegv_handler (int signo, siginfo_t *ip, void *context)
+{
+  process_sigsegv
+    (signo, (ip != NULL
+            && ip->si_signo == SIGSEGV ? (char *) ip->si_addr : NULL));
+}
+
+#elif HAVE_SIGINFO_T
 
 /* SVR4.  */
 
 static void
-sigsegv_handler (int signo, siginfo_t * ip)
+sigsegv_handler (int signo, siginfo_t *ip)
 {
   process_sigsegv
-    (signo, (ip != (siginfo_t *) 0
+    (signo, (ip != NULL
             && ip->si_signo == SIGSEGV ? (char *) ip->si_addr : NULL));
 }
 
-#else /* not HAVE_SIGINFO_H */
-#if HAVE_SIGCONTEXT
+#elif HAVE_SIGCONTEXT
 
 /* SunOS 4.x (and BSD?).  (not tested) */
 
@@ -245,7 +272,6 @@ sigsegv_handler (int signo)
 }
 
 #endif /* not HAVE_SIGCONTEXT */
-#endif /* not HAVE_SIGINFO && not HAVE_SIGINFO_T */
 
 /* Arrange to trap a stack-overflow and call a specified handler.  The
    call is on a dedicated signal stack.
@@ -270,20 +296,23 @@ setup_stackovf_trap (char *const *argv, 
   int grows_upward;
   register char *const *v;
   register char *p;
-#if HAVE_SIGACTION && defined(SA_ONSTACK)
+#if HAVE_SIGACTION && defined SA_ONSTACK
   struct sigaction act;
-#else
+#elif HAVE_SIGVEC && defined SV_ONSTACK
   struct sigvec vec;
+#else
+
+Error - Do not know how to set up stack-ovf trap handler...
+
 #endif
 
-  grows_upward = ((char *) argv < (char *) &stack_len);
   arg0 = argv[0];
   stackovf_handler = handler;
 
   /* Calculate the approximate expected addr for a stack-ovf trap.  */
 
   if (getrlimit (RLIMIT_STACK, &rl) < 0)
-    error (1, errno, "getrlimit");
+    error (EXIT_FAILURE, errno, "getrlimit");
   stack_len = (rl.rlim_cur < rl.rlim_max ? rl.rlim_cur : rl.rlim_max);
   stackbot = (char *) argv;
   grows_upward = ((char *) &stack_len > stackbot);
@@ -292,14 +321,14 @@ setup_stackovf_trap (char *const *argv, 
 
       /* Grows toward increasing addresses.  */
 
-      for (v = argv; (p = (char *) *v) != (char *) 0; v++)
+      for (v = argv; (p = (char *) *v) != NULL; v++)
        {
          if (p < stackbot)
            stackbot = p;
        }
       if ((char *) envp < stackbot)
        stackbot = (char *) envp;
-      for (v = envp; (p = (char *) *v) != (char *) 0; v++)
+      for (v = envp; (p = (char *) *v) != NULL; v++)
        {
          if (p < stackbot)
            stackbot = p;
@@ -311,14 +340,14 @@ setup_stackovf_trap (char *const *argv, 
 
       /* The stack grows "downward" (toward decreasing addresses).  */
 
-      for (v = argv; (p = (char *) *v) != (char *) 0; v++)
+      for (v = argv; (p = (char *) *v) != NULL; v++)
        {
          if (p > stackbot)
            stackbot = p;
        }
       if ((char *) envp > stackbot)
        stackbot = (char *) envp;
-      for (v = envp; (p = (char *) *v) != (char *) 0; v++)
+      for (v = envp; (p = (char *) *v) != NULL; v++)
        {
          if (p > stackbot)
            stackbot = p;
@@ -328,9 +357,9 @@ setup_stackovf_trap (char *const *argv, 
 
   /* Allocate a separate signal-handler stack.  */
 
-#if HAVE_SIGALTSTACK && (HAVE_SIGINFO_H || HAVE_SIGINFO_T || !HAVE_SIGSTACK)
+#if HAVE_SIGALTSTACK && (HAVE_SIGINFO_T || ! HAVE_SIGSTACK)
 
-  /* Use sigaltstack only if siginfo is available, unless there is no
+  /* Use sigaltstack only if siginfo_t is available, unless there is no
      choice.  */
 
   {
@@ -341,15 +370,14 @@ setup_stackovf_trap (char *const *argv, 
     ss.ss_size = SIGSTKSZ;
     ss.ss_sp = (void *) stackbuf;
     ss.ss_flags = 0;
-    if (sigaltstack (&ss, (stack_t *) 0) < 0)
+    if (sigaltstack (&ss, NULL) < 0)
       {
        free ((void *) stackbuf);
-       error (1, errno, "sigaltstack");
+       error (EXIT_FAILURE, errno, "sigaltstack");
       }
   }
 
-#else /* not HAVE_SIGALTSTACK || not HAVE_SIGINFO_H && not HAVE_SIGINFO_T && 
HAVE_SIGSTACK */
-#if HAVE_SIGSTACK
+#elif HAVE_SIGSTACK
 
   {
     struct sigstack ss;
@@ -359,8 +387,8 @@ setup_stackovf_trap (char *const *argv, 
     ss.ss_onstack = 0;
     if (sigstack (&ss, NULL) < 0)
       {
-       free (stackbuf);
-       error (1, errno, "sigstack");
+       free ((void *) stackbuf);
+       error (EXIT_FAILURE, errno, "sigstack");
       }
   }
 
@@ -369,45 +397,31 @@ setup_stackovf_trap (char *const *argv, 
 Error - Do not know how to set up stack-ovf trap handler...
 
 #endif /* not HAVE_SIGSTACK */
-#endif /* not HAVE_SIGALTSTACK || not HAVE_SIGINFO_H && HAVE_SIGSTACK */
 
   /* Arm the SIGSEGV signal handler.  */
 
-#if HAVE_SIGACTION && defined(SA_ONSTACK)
+#if HAVE_SIGACTION && defined SA_ONSTACK
 
   sigaction (SIGSEGV, NULL, &act);
+# if HAVE_STRUCT_SIGACTION_SA_SIGACTION
+  act.sa_sigaction = sigsegv_handler;
+# else /* ! HAVE_STRUCT_SIGACTION_SA_SIGACTION */
   act.sa_handler = (RETSIGTYPE (*) (int)) sigsegv_handler;
+# endif /* ! HAVE_STRUCT_SIGACTION_SA_SIGACTION */
   sigemptyset (&act.sa_mask);
-  act.sa_flags = (SA_ONSTACK
-#ifdef SA_RESETHAND
-                 | SA_RESETHAND
-#endif
-#ifdef SA_SIGINFO
-                 | SA_SIGINFO
-#endif
-                 );
+  act.sa_flags = (SA_ONSTACK | SA_RESETHAND | SA_SIGINFO);
   if (sigaction (SIGSEGV, &act, NULL) < 0)
-    error (1, errno, "sigaction");
+    error (EXIT_FAILURE, errno, "sigaction");
 
-#else /* not HAVE_SIGACTION */
-#if HAVE_SIGVEC && defined(SV_ONSTACK)
+#else /* ! HAVE_SIGACTION */
 
   vec.sv_handler = (RETSIGTYPE (*) (int)) sigsegv_handler;
   vec.sv_mask = 0;
-  vec.sv_flags = (SV_ONSTACK
-#ifdef SV_RESETHAND
-                 | SV_RESETHAND
-#endif
-                );
+  vec.sv_flags = (SV_ONSTACK | SV_RESETHAND);
   if (sigvec (SIGSEGV, &vec, NULL) < 0)
-    error (1, errno, "sigvec");
-
-#else /* not HAVE_SIGVEC && defined(SV_ONSTACK) */
-
-Error - Do not know how to catch signals on an alternate stack...
+    error (EXIT_FAILURE, errno, "sigvec");
 
-#endif /* HAVE_SIGVEC && defined(SV_ONSTACK) */
-#endif /* HAVE_SIGALTSTACK && defined(SA_ONSTACK) */
+#endif /* ! HAVE_SIGACTION */
 
 }