Re: [PATCH 0/2] Demangler crash handler

[Gary Benson <gbenson@redhat.com>]

Mark Kettenis wrote:
> > A number of bugs have been filed recently because of segmentation
> > faults in the demangler.  While such crashes are a problem for all
> > demangler consumers, they are particularly nasty for GDB because
> > they prevent the user from debugging their program at all.
> > 
> > This patch series arranges for GDB to catch segmentation faults
> > in the demangler and recover from them gracefully.  A warning is
> > printed the first time a fault occurs.  Example sessions with and
> > without these patches are included below.
> > 
> > None of the wrapped code uses cleanups, so each caught failure
> > will leak a small amount of memory.  This is undesirable but I
> > think the benefits here outweigh this drawback.
> > 
> > Ok to commit?
> 
> No.  It's this skind of duct-tape that will make sure that bugs in
> the demangler won't get fixed.  Apart from removing the incentive to
> fix the bugs, these SIGSEGV signal handlers make actually fixing the
> bugs harder as you won't have core dumps.

I would normally agree with you 100% on this issue Mark, but in this
case I think a handler is justified.  If the demangler crashes because
of a symbol in the users program then the user cannot debug their
program at all.  If the demangler were simple and well understood then
that would be fine but it's not: the demangler is complex, the
specification it's following is complex, and everything's complicated
further because you can't allocate heap and you have to roll your own
data structures.  The reality is that the libiberty demangler is a
breeding ground for segfaults, and GDB needs to be able to deal with
this.

It's true that you don't get core dumps with this patch, but what you
do get in return is a printed warning that includes the symbol that
caused the crash.  That's all you need in most cases.  The five recent
demangler crashes (14963, 16593, 16752, 16817 and 16845) all required
digging by either the reporter or a GDB developer to uncover the
failing symbol.  Printing the offending symbol means this work is
already done.

If the lack of core dumps is a showstopper for you then I can
update the patch to allow disabling the handler with
"maint set handle-demangler-crashes 0" or some similar thing.

> Besides, any signal handler that does more than just setting a flag
> is probably broken.  Did you verify that you only call async-signal-
> safe functions in the signal handler code path?

I didn't think this was necessary as to my knowledge SIGSEGV is only
ever emitted synchronously.  If it is an issue then the patch could
be reworked to use (sig)longjmp as included below.

Thanks,
Gary


diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 91533e8..5e79fb4 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -36,6 +36,7 @@
 #include "value.h"
 #include "cp-abi.h"
 #include "language.h"
+#include <signal.h>
 
 #include "safe-ctype.h"
 
@@ -1505,12 +1506,89 @@ cp_lookup_rtti_type (const char *name, struct block *block)
   return rtti_type;
 }
 
+#ifdef SIGSEGV
+
+/* PortabiWrap set/long jmp so that it's more portable.  */
+
+#if defined(HAVE_SIGSETJMP)
+#define SIGJMP_BUF		sigjmp_buf
+#define SIGSETJMP(buf)		sigsetjmp((buf), 1)
+#define SIGLONGJMP(buf,val)	siglongjmp((buf), (val))
+#else
+#define SIGJMP_BUF		jmp_buf
+#define SIGSETJMP(buf)		setjmp(buf)
+#define SIGLONGJMP(buf,val)	longjmp((buf), (val))
+#endif
+
+/* Stack context and environment for demangler crash recovery.  */
+
+static SIGJMP_BUF gdb_demangle_jmp_buf;
+
+/* Signal handler for gdb_demangle.  */
+
+static void
+gdb_demangle_signal_handler (int signo)
+{
+  SIGLONGJMP (gdb_demangle_jmp_buf, signo);
+}
+
+#endif
+
 /* A wrapper for bfd_demangle.  */
 
 char *
 gdb_demangle (const char *name, int options)
 {
-  return bfd_demangle (NULL, name, options);
+  char *result = NULL;
+  int crash_signal = 0;
+
+#ifdef SIGSEGV
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+  struct sigaction sa, old_sa;
+
+  sa.sa_handler = gdb_demangle_signal_handler;
+  sigemptyset (&sa.sa_mask);
+  sa.sa_flags = 0;
+  sigaction (SIGSEGV, &sa, &old_sa);
+#else
+  void (*ofunc) ();
+
+  ofunc = (void (*)()) signal (SIGSEGV, gdb_demangle_signal_handler);
+#endif
+
+  crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
+#endif
+
+  if (crash_signal == 0)
+    result = bfd_demangle (NULL, name, options);
+
+#ifdef SIGSEGV
+#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
+  sigaction (SIGSEGV, &old_sa, NULL);
+#else
+  signal (SIGSEGV, ofunc);
+#endif
+#endif
+
+  if (crash_signal != 0)
+    {
+      static int warning_printed = 0;
+
+      if (!warning_printed)
+	{
+	  warning ("internal error: demangler failed with signal %d\n"
+		   "Unable to demangle '%s'\n"
+		   "This is a bug, "
+		   "please report it to the GDB maintainers.",
+		   crash_signal, name);
+
+	  warning_printed = 1;
+	}
+
+      result = NULL;
+    }
+
+  return result;
 }
 
 /* Don't allow just "maintenance cplus".  */