On 12/04/2013 12:14 PM, 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(a)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(a)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(a)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.
Can we also just in general be more explicit. May be use something like:
"Out of memory trying to allocate X to do Y"
> + }
>
> return ret;
> }
> --
> 1.8.3.1
>
> From 748aa120e07f20b022de29589c3e2179aa2c8840 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <pavel.reichl(a)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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager for IdM portfolio
Red Hat Inc.
-------------------------------
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/