Re: c-stack: use libsigsegv when possible

[Eric Blake]

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

According to Bruno Haible on 7/16/2008 7:29 PM:
| If you want to save linking with libsigsegv, just do the test
| '#if HAVE_LIBSIGSEGV && ! HAVE_XSI_STACK_OVERFLOW_HEURISTIC' at configure
| time rather than at compile-time, create two new variables LIBCSTACK and
| LTLIBCSTACK, AC_SUBST them, and document in modudes/c-stack that the
| appropriate of these two variables needs to be used while linking.

OK, I'll rework the patch along those lines.

|
|> In writing the patch, I noticed that it is probably not portable to
|> longjmp out of a stack overflow handler
|
| This is not an issue with the current uses of c-stack, which just print
| a message and call _exit in the handler.

Actually, c-stack allows the installation of an arbitrary user handler,
which does not necessarily call _exit.  It is this user handler, if one is
provided, that must be aware of the restrictions on using only async-safe
functions, even after longjmp'ing out.

|
| In general, longjmping out of a stack overflow handler can be done
| provided that you call sigsegv_leave_handler() before.

That only guarantees that the signal mask is back in shape.  But it still
does NOT solve the problem of what happens if the stack overflow occurred
while a non-async-safe system function happened to be interrupted after it
had obtained locks.  The longjmp won't reset those resources, and so the
code at the setjmp site cannot safely do anything that mallocs.

| This is a function
| from libsigsegv that does the necessary magic. It has been tested in GNU
clisp:
| when the user types Ctrl-C and then 'abort' to get to the top-level
interactive
| prompt, this code is exercised.

That's slightly different - that wasn't stack overflow, so the program can
be written in such a manner that invocation of any non-async-signal
library function is guarded by an appropriate signal mask, such that the
Ctrl-C can be guaranteed to never interrupt something where the longjmp
would then trigger undefined behavior if the same function is called again.

|> or we need to add a function in c-stack.h that
|> the user must call before attempting longjmp anyway; this wrapper would be
|> responsible for calling libsigsegv's sigsegv_leave_handler as needed.
|
| There's a misunderstanding here: sigsegv_leave_handler() does only ensure
| that the thread - viewed from the kernel's point of view - is in a
reasonable
| state; it does *not* repair any interrupted stateful library function like
| malloc. (In GNU clisp this is not a problem in practice, because malloc
| is rarely called.)

No, I think I understood that already.  That's why I made the statement
that although the user's handler may call longjmp, it must 1) reset the
signal mask, 2) work around OS deficiencies (by calling
sigsegv_leave_handler), and 3) not rely on any non-async-safe functions to
work at the setjmp site.

|
| Another thing: Could you please add a comment summarizing our
discussions from
| 2008-06-06, namely explaining why - although Linux and Solaris both have
| a working sigaltstack(), SIGINFO, and si_addr -
| HAVE_XSI_STACK_OVERFLOW_HEURISTIC is only defined on Solaris and not on
| Linux?

Sure.  Still no response from the Austin Group, as to whether Linux is
buggy in this regard.  I'll send a ping on that thread, as well.

But first, I'm checking in this - it makes it easier to see at a glance
which platforms have false positives when using solely c-stack and not
libsigsegv, as well as making it easier to see if using libsigsegv
introduces any regressions.  Tested on mingw/cygwin (where both tests skip
with identical perror message), on Linux and OpenBSD4.0 (where
test-c-stack.sh passes, and test-c-stack2.sh skips and mentions the false
positive), and Solaris 8 (where both tests pass).

I also noticed an oddity with Solaris 8 /bin/sh that I had to work around:

(die-with-segv) 2> file

causes the shell to output 'Segmentation fault' to stderr, but not

(die-with-segv; echo $?) 2> file

- --
Don't work too hard, make some time for fun as well!

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

iEYEARECAAYFAkh+uHUACgkQ84KuGfSFAYCbaACfabP3FQEt3TU4no0InFvWxo+P
om0Ani2c6I6YrDmIv0msD8dEL7/cAMzt
=sJsJ
-----END PGP SIGNATURE-----

>From 1eb850a182e6e5aa66d17d314b394e2ded30e2b5 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Wed, 16 Jul 2008 16:28:13 -0600
Subject: [PATCH] c-stack: Expose false positives when not using libsigsegv.

* modules/c-stack-tests (Files): Expand test.
* tests/test-c-stack.c (main): Add means to conditionally trigger
non-overflow SIGSEGV.
* tests/test-c-stack2.sh: New file.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog              |   10 +++++++++-
 modules/c-stack-tests  |    5 +++--
 tests/test-c-stack.c   |   19 +++++++++++++------
 tests/test-c-stack2.sh |   29 +++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 9 deletions(-)
 create mode 100755 tests/test-c-stack2.sh

diff --git a/ChangeLog b/ChangeLog
index 91c45ce..0772f2d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,10 +1,18 @@
+2008-07-16  Eric Blake  <address@hidden>
+
+       c-stack: Expose false positives when not using libsigsegv.
+       * modules/c-stack-tests (Files): Expand test.
+       * tests/test-c-stack.c (main): Add means to conditionally trigger
+       non-overflow SIGSEGV.
+       * tests/test-c-stack2.sh: New file.
+
 2008-07-14  Bruno Haible  <address@hidden>
 
        * m4/libsigsegv.m4: Remove unneeded AC_PREREQ.
        Reported by Eric Blake.
 
 2008-07-14  Sam Steingold  <address@hidden>
-            Bruno Haible  <address@hidden>
+           Bruno Haible  <address@hidden>
 
        New module libsigsegv.
        * modules/libsigsegv: New file.
diff --git a/modules/c-stack-tests b/modules/c-stack-tests
index 291f58d..5d3bc58 100644
--- a/modules/c-stack-tests
+++ b/modules/c-stack-tests
@@ -1,6 +1,7 @@
 Files:
 tests/test-c-stack.c
 tests/test-c-stack.sh
+tests/test-c-stack2.sh
 
 Depends-on:
 exitfail
@@ -8,8 +9,8 @@ exitfail
 configure.ac:
 
 Makefile.am:
-TESTS += test-c-stack.sh
+TESTS += test-c-stack.sh test-c-stack2.sh
 TESTS_ENVIRONMENT += EXEEXT='@EXEEXT@'
 check_PROGRAMS += test-c-stack
 test_c_stack_LDADD = $(LDADD) @LIBINTL@
-MOSTLYCLEANFILES += t-c-stack.tmp
+MOSTLYCLEANFILES += t-c-stack.tmp t-c-stack2.tmp
diff --git a/tests/test-c-stack.c b/tests/test-c-stack.c
index 13c1a12..bcf72ad 100644
--- a/tests/test-c-stack.c
+++ b/tests/test-c-stack.c
@@ -29,11 +29,11 @@
   do                                                                        \
     {                                                                       \
       if (!(expr))                                                          \
-        {                                                                   \
-          fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \
-          fflush (stderr);                                                  \
-          abort ();                                                         \
-        }                                                                   \
+       {                                                                    \
+         fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \
+         fflush (stderr);                                                   \
+         abort ();                                                          \
+       }                                                                    \
     }                                                                       \
   while (0)
 
@@ -62,7 +62,14 @@ main (int argc, char **argv)
 #endif
 
   if (c_stack_action (0) == 0)
-    return recurse ("\1");
+    {
+      if (1 < argc)
+       {
+         exit_failure = 77;
+         ++*argv[argc]; /* Intentionally dereference NULL.  */
+       }
+      return recurse ("\1");
+    }
   perror ("c_stack_action");
   return 77;
 }
diff --git a/tests/test-c-stack2.sh b/tests/test-c-stack2.sh
new file mode 100755
index 0000000..e55ff17
--- /dev/null
+++ b/tests/test-c-stack2.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+tmpfiles=""
+trap 'rm -fr $tmpfiles' 1 2 3 15
+
+tmpfiles="t-c-stack2.tmp"
+
+# Sanitize exit status within a subshell, since some shells fail to
+# redirect stderr on their message about death due to signal.
+(./test-c-stack${EXEEXT} 1; exit $?) 2> t-c-stack2.tmp
+
+case $? in
+  77) if grep 'stack overflow' t-c-stack2.tmp >/dev/null ; then
+       echo 'cannot distinguish stack overflow from crash' >&2
+      else
+       cat t-c-stack2.tmp >&2
+      fi
+      (exit 77); exit 77 ;;
+  0) (exit 1); exit 1 ;;
+esac
+if grep 'program error' t-c-stack2.tmp >/dev/null ; then
+  :
+else
+  (exit 1); exit 1
+fi
+
+rm -fr $tmpfiles
+
+exit 0
-- 
1.5.6