From patchwork Fri May 9 15:33:06 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gary Benson X-Patchwork-Id: 861 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx23.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 98208360073 for ; Fri, 9 May 2014 08:33:26 -0700 (PDT) Received: by homiemail-mx23.g.dreamhost.com (Postfix, from userid 14314964) id 525CD63A288D0; Fri, 9 May 2014 08:33:26 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx23.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx23.g.dreamhost.com (Postfix) with ESMTPS id 018B663ACB304 for ; Fri, 9 May 2014 08:33:25 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; q=dns; s=default; b=l+r9 9XEGtRMmsOK84ryskheazGrpqf7K37UCNGtn+JPj6e5FinUzO8s5LUwToV0CNaxQ W0SLvivMBGLcN9jPO8jIzSkeFbOmpQcjs2ILGZFbsRUWZnMfPi9+eG14UGnM1ruJ ujL9twvskapXht4eHnHRaOB+6s7MndANbc2xjmc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; s=default; bh=t17jPsluES ACZBQaOWnMI8vv2iA=; b=nCA1K2kE+0/spQcnYw/BJqZuUWKdqXWQgAETtfkzQs b7KbwdxrztelwplWE3+DO+Qb9PTmtGNCgqdLRhzhRfA9eGa4y6AHt4KSmAbdpIrw 77+UfQ0aursdtk0Ei0OoHCe0kzghj44jGfVw4TwHcsp/oI94zwZfPOraYgATkV2S Y= Received: (qmail 22638 invoked by alias); 9 May 2014 15:33:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 22525 invoked by uid 89); 9 May 2014 15:33:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 09 May 2014 15:33:09 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s49FX79S012847 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 9 May 2014 11:33:08 -0400 Received: from blade.nx (ovpn-116-96.ams2.redhat.com [10.36.116.96]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s49FX65O008509; Fri, 9 May 2014 11:33:07 -0400 Received: by blade.nx (Postfix, from userid 1000) id 19DD726234A; Fri, 9 May 2014 16:33:06 +0100 (BST) Date: Fri, 9 May 2014 16:33:06 +0100 From: Gary Benson To: Mark Kettenis Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 0/2] Demangler crash handler Message-ID: <20140509153305.GA13345@blade.nx> References: <20140509100656.GA4760@blade.nx> <201405091120.s49BKO1f010622@glazunov.sibelius.xs4all.nl> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <201405091120.s49BKO1f010622@glazunov.sibelius.xs4all.nl> X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in 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 #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". */