[SSSD] Warn to syslog if an unresponsive subprocess is terminated

Pavel Reichl preichl at redhat.com
Thu Dec 5 13:11:46 UTC 2013


Hello, 

patches addressing Jakub's and Dmitri's comments are attached.

I have decided to merge two previously separate patches into one as both
are patching same function (monitor_kill_service). 

On Wed, 2013-12-04 at 18:14 +0100, Jakub Hrozek wrote:
> On Mon, Dec 02, 2013 at 07:41:19PM +0100, Pavel Reichl wrote:
> > Hello,
> > 
> > first four patches address issues of freeing mt_svc structure. Last one
> > adds warns to syslog.
> > 
> > Pavel Reichl
> 
> The patches work well and the code looks good. I have a couple of very
> small nitpicks, see inline.
> 
> > From 1d44284275e9f227d2f39085c6a6661a04e9b69c Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl at redhat.com>
> > Date: Mon, 2 Dec 2013 14:51:59 +0000
> > Subject: [PATCH 1/5] monitor: use-after-free bugfix
> > 
> > *monitor_kill_service* may create timed event which operates on *svc* and
> > therefore *svc* should not be freed right after call of *monitor_kill_sercice*.
> > *svc* is supposed to be freed by *mt_svc_restart*.
> > ---
> >  src/monitor/monitor.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> > index 92bac422a53b450f29a2d5b23217b530030781c2..ce4b1adb12626517d5cfa0a932fb6c6198cd2858 100644
> > --- a/src/monitor/monitor.c
> > +++ b/src/monitor/monitor.c
> > @@ -727,7 +727,6 @@ static int service_signal(struct mt_svc *svc, const char *svc_signal)
> >      if (!msg) {
> >          DEBUG(0,("Out of memory?!\n"));
> 
> We're trying to move away from the old decimal debug levels in the code,
> can you change the number 0 to a SSSDBG_ macro while you're changing the
> code?
> 
> >          monitor_kill_service(svc);
> > -        talloc_free(svc);
> 
> This change is correct, nice catch btw.
> 
> >          return ENOMEM;
> >      }
> >  
> > -- 
> > 1.8.3.1
> > 
> 
> > From 60033e161524a27587d409ae8f4807a9d70c8d02 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl at redhat.com>
> > Date: Mon, 2 Dec 2013 15:20:01 +0000
> > Subject: [PATCH 2/5] monitor: use-after-free bugfix
> 
> ACK
> 
> 
> > From b312127656465a30d7b5e93e661cacea592bdc99 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl at redhat.com>
> > Date: Mon, 2 Dec 2013 15:37:57 +0000
> > Subject: [PATCH 3/5] monitor: check tevent_add_timer & remove sigkill_ev
> > @@ -629,8 +628,13 @@ static int monitor_kill_service (struct mt_svc *svc)
> >       * doesn't exit within sixty seconds
> >       */
> >      tv = tevent_timeval_current_ofs(svc->kill_time, 0);
> > -    svc->sigkill_ev = tevent_add_timer(svc->mt_ctx->ev, svc, tv,
> > -                                       mt_svc_sigkill, svc);
> > +    te = tevent_add_timer(svc->mt_ctx->ev, svc, tv, mt_svc_sigkill, svc);
> > +    if (te == NULL) {
> > +        /* Nothing much we can do */
> > +        ret = ENOMEM;
> > +        DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory?!\n"));
> > +        talloc_free(svc);
> 
> I agree with removing the sigkill_ev, but I would prefer if the function
> either returned ENOMEM here directly or jumped to return. I think the
> fallthrough is potentially dangerous.
> 
> > +    }
> >  
> >      return ret;
> >  }
> > -- 
> > 1.8.3.1
> > 
> 
> > From 748aa120e07f20b022de29589c3e2179aa2c8840 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl at redhat.com>
> > Date: Mon, 2 Dec 2013 15:49:04 +0000
> > Subject: [PATCH 4/5] monitor: memory-leak bug
> 
> ACK
> 
> 
> > From b1bcf5cd7f73735ebe251a692ee989e63c0158bb Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl at redhat.com>
> > Date: Mon, 2 Dec 2013 17:22:55 +0000
> > Subject: [PATCH 5/5] monitor: syslog when process killed by monitor
> 
> ACK
> 
> I tested service restarts (by sending SIGSEGV to sssd_be), service kills
> by stopping the sssd_be in gdb. The services are restarted as
> appropriate and the syslog contains the new message.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-monitor-use-after-free-bugfix.patch
Type: text/x-patch
Size: 1683 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131205/e6fce051/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-monitor-monitor_kill_service-refactor.patch
Type: text/x-patch
Size: 2233 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131205/e6fce051/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-monitor-memory-leak-bug.patch
Type: text/x-patch
Size: 1105 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131205/e6fce051/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-monitor-syslog-when-process-killed-by-monitor.patch
Type: text/x-patch
Size: 1419 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131205/e6fce051/attachment-0003.bin>


More information about the sssd-devel mailing list