Re: [PATCH 3/3 v4] Demangler crash handler

[Gary Benson <gbenson@redhat.com>]

Andrew Burgess wrote:
> On 09/06/2014 10:01 AM, Gary Benson wrote:
> > Andrew Burgess wrote:
> > > On 05/06/2014 2:03 PM, Gary Benson wrote:
> > > > diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> > > > index 91533e8..f4dde70 100644
> > > > --- a/gdb/cp-support.c
> > > > +++ b/gdb/cp-support.c
> > >
> > > > +
> > > > +/* Signal handler for gdb_demangle.  */
> > > > +
> > > > +static void
> > > > +gdb_demangle_signal_handler (int signo)
> > > > +{
> > > > +  if (gdb_demangle_attempt_core_dump)
> > > > +    {
> > > > +      if (fork () == 0)
> > > > +	dump_core ();
> > >
> > > This worries me a little, when a problem case occurs gdb will
> > > dump core regardless of the users ulimit setting, without first
> > > asking the user, and doesn't tell the user that a core file was
> > > created.
> > >
> > > This feels quite unexpected behaviour to me, especially the bit
> > > about disregarding the ulimit setting without first asking for
> > > permission.
> > >
> > > Catching the crash feels like a good idea, but I'd prefer that
> > > gdb ask before circumventing the ulimit and dumping core.
> > 
> > This part of the same patch:
> > 
> > +  if (core_dump_allowed == -1)
> > +    {
> > +      core_dump_allowed = can_dump_core ();
> > +
> > +      if (!core_dump_allowed)
> > +        gdb_demangle_attempt_core_dump = 0;
> > +    }
> > 
> > calls this:
> > 
> >   int
> >   can_dump_core (void)
> >   {
> >   #ifdef HAVE_GETRLIMIT
> >     struct rlimit rlim;
> >   
> >     /* Be quiet and assume we can dump if an error is returned.  */
> >     if (getrlimit (RLIMIT_CORE, &rlim) != 0)
> >       return 1;
> >   
> >     if (rlim.rlim_max == 0)
> >       return 0;
> >   #endif /* HAVE_GETRLIMIT */
> >   
> >     return 1;
> >   }
> > 		  
> > which inhibits the core dump if the user's ulimit is 0.
> 
> Ahh, yes I see.
> 
> So the problem here is this function is geared towards the /old/ use
> of the function where we are about to ask the user if we should dump
> core.  For that, this function was correct, we check the hard limit
> of the resource.  If the hard limit is high then we ask the user,
> and dump core.
> 
> However, in doing so we circumvent the soft limit rlim.rlim_cur.  So
> I think my point still stands.  The user has said "no core files
> please", and we create one without asking.  If we must go down this
> road then I think we need two functions to check the two different
> limits.

Ah, I didn't realize the code in dump_core was to override the user's
soft limit.  I will update the patch.

> > > Alternatively we could just not dump core from gdb, report the
> > > bad symbol and let the user file a bug.  With the demangler
> > > being so deterministic it should be possible to reproduce, if
> > > not, then we just ask the user to turn off the crash catch,
> > > adjust their ulimit (like we would with any other gdb SEGV
> > > crash), and rerun the test.
> > 
> > That was and is my preferred solution, but Mark Kettenis indicated
> > that he would not accept the patch unless a meaningful core file
> > was created.
> 
> I don't understand that position, but I'd hope he'd agree that we
> should respect the user ulimit over creating a core file...

Yes, this seems reasonable.

> > > If we really want to create the core file by default, but aren't
> > > going to ask, then I'd propose we honour the ulimit setting, and
> > > make sure that the user is told that a core file was just written.
> > 
> > The problem with asking is that you'd have to ask within the signal
> > handler, and no code that prints to the screen is safe to call from
> > within a signal handler.
> 
> Indeed.  I did wonder about some horrible synchronisation scheme
> where the "master" gdb process queries the user then signals the
> fork()ed child to indicate if it should dump core or not .... but
> it felt like huge overkill.

Yeah, I thought down this road too :)

> > Even indicating that a core file was written is probably
> > impossible: you just have to abort and hope for the best.
> > The nearest I could do is set a flag in the signal handler
> > and have the code it returns to print "Attempting to dump
> > core" or some such thing.
> 
> I think an "attempting ..." style message would be enough, the 
> gdb_demangle_attempt_core_dump flag could be used to indicate
> if we've tried to dump core or not.

I will add this to the updated patch.

Thanks,
Gary

-- 
http://gbenson.net/