diff mbox

[RFC] Demangler crash resistance

Message ID 20140410115348.GA1381@blade.nx
State RFC, archived
Headers show

Commit Message

Gary Benson April 10, 2014, 11:53 a.m. UTC 
Hi all,

A number of demangler crashes got filed recently, at least some of
which are likely my fault.  These need fixing, of course, but this
highlighted to me the fact that GDB is probably the most sensitive
user of libiberty's demangler in terms of crashes, etc.  GCC and glibc
only enter the demangler for error conditions, whereas GDB demangles
symbols in general use.  Furthermore, a given GDB session will likely
demangle a lot of symbols.  On the other side of the coin, I don't
think many GDB hackers have the depth of knowledge to fix some of the
more complicated crashes, in terms of understanding what the demangler
is supposed to be producing.  Feel free to correct me here (but be
prepared for some questions if you do!)

Here is a quandry where the people with the expertise to fix demangler
crashes chiefly work on projects where such bugs are low priority.
They're high priority for GDB, but we don't have the expertise to fix
them.  While the bugs are unfixed, our users may not be able to use
GDB to debug their applications.

I started to think maybe we should make GDB more resistant to
demangler crashes, to keep our users in business.  I attached a basic
patch I've worked on as a proof of concept.  I'm aware this needs
portability work, a ChangeLog, and maybe error message throttling.
More importantly, I'm also aware that none of the wrapped code uses
cleanups, so caught failures will leak memory.  Normally I wouldn't
consider this appropriate, but I think here the benefit may outweigh
the problems.

Example sessions with the attached patch applied, using the symbol
from https://sourceware.org/bugzilla/show_bug.cgi?id=16817:

| (gdb) set lang c++
| (gdb) maint demangle _QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z
| warning: internal error: demangler failed with signal 11
| Can't demangle "_QueueNotification_QueueController__$4PPPPPPPM_A_INotice___Z"

and:

| (gdb) add-symbol-file /home/gary/a.out 0
| add symbol table from file "/home/gary/a.out" at
|         .text_addr = 0x0
| (y or n) y
| Reading symbols from /home/gary/a.out...warning: internal error: demangler failed with signal 11
| (no debugging symbols found)...done.

Comments please :)

Thanks,
Gary

Comments

Tom Tromey April 10, 2014, 5:29 p.m. UTC | #1 
>>>>> "Gary" == Gary Benson <gbenson@redhat.com> writes:

Gary> I started to think maybe we should make GDB more resistant to
Gary> demangler crashes, to keep our users in business.  I attached a basic
Gary> patch I've worked on as a proof of concept.

FWIW I did this once too:

    https://sourceware.org/ml/gdb-patches/2013-01/msg00290.html

See the whole thread.  You can also see the work on my branch
"demangler-crash-catcher".

I'm on the fence about this.  On the one hand, it's dangerous.  On the
other hand, I imagine it would make gdb more robust in practice.

Tom
diff mbox

Patch

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 2379b54..1783dd1 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -35,8 +35,8 @@ 
 #include "expression.h"
 #include "value.h"
 #include "cp-abi.h"
-
 #include "safe-ctype.h"
+#include <signal.h>
 
 #define d_left(dc) (dc)->u.s_binary.left
 #define d_right(dc) (dc)->u.s_binary.right
@@ -1482,12 +1482,40 @@  cp_lookup_rtti_type (const char *name, struct block *block)
   return rtti_type;
 }
 
+/* Signal handler for gdb_demangle.  */
+
+static void
+demangle_signal_handler (int signo)
+{
+  throw_error (GENERIC_ERROR, _("demangler failed with signal %d"),
+	       signo);
+}
+
 /* A wrapper for bfd_demangle.  */
 
 char *
 gdb_demangle (const char *name, int options)
 {
-  return bfd_demangle (NULL, name, options);
+  volatile struct gdb_exception except;
+  void (*ofunc) ();
+  char *result = NULL;
+
+  ofunc = (void (*)()) signal (SIGSEGV, demangle_signal_handler);
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    {
+      result = bfd_demangle (NULL, name, options);
+    }
+
+  signal (SIGSEGV, ofunc);
+
+  if (except.reason < 0)
+    {
+      warning ("internal error: %s", except.message);
+      result = NULL;
+    }
+
+  return result;
 }
 
 /* Don't allow just "maintenance cplus".  */