Re: [PATCH 0/2] Demangler crash handler

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Fri, 9 May 2014 16:33:06 +0100
> From: 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.

There are entire subsystems in GDB that are a breeding ground for
segfaults.  Should we therefore wrap evrything?

It is obvious that the demangler is a breeding ground for segmentation
faults.  It uses strcpy, strcat and sprintf.  So it's probably full of
buffer overflows.  I bet that if those are fixed, the SIGSEGVs are
gone.

Note that only some of those buffer overflows will generate a SIGSEGV.
Others will corrupt random memory.  And you can't patch those up with
a signal handler.

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

Not acceptable.  Unless you make it the default...