tevent memory leak?

[Jeremy Allison]

On Tue, Feb 16, 2016 at 02:25:50PM -0800, Jeremy Allison wrote:
> On Tue, Feb 16, 2016 at 12:26:13PM +0100, Pavel Březina wrote:
> > Hi,
> > we are dealing with user-reported memory leak in sssd and I can see
> > in talloc report that there are ~140k if struct sigaction:
> > 
> >     struct tevent_sig_state        contains 21149312 bytes in 139123
> > blocks (ref 6) 0x7f0b97781990
> >         struct sigaction               contains    152 bytes in   1
> > blocks (ref 0) 0x7f0b9c1d9f10
> >         struct sigaction               contains    152 bytes in   1
> > blocks (ref 0) 0x7f0b9c195f80
> >         struct sigaction               contains    152 bytes in   1
> > blocks (ref 0) 0x7f0b9c195e80
> >         struct sigaction               contains    152 bytes in   1
> > blocks (ref 0) 0x7f0b9c195d80
> >         ...
> > 
> > Looking into tevent code, it comes from tevent_common_add_signal():
> > 
> > sig_state->oldact[signum] = talloc(sig_state, struct sigaction);
> > if (sig_state->oldact[signum] == NULL) {
> >     talloc_free(se);
> >     return NULL;
> > }
> > 
> > But it is nowhere freed. I would expect it to be freed in signal
> > destructor after restoring the original sigaction. Is it intentional
> > or a bug?
> 
> I think this is a bug. Can you try the following (untested, but looks
> right :-) patch ? If you can confirm I'll get a second review for
> mater and we can log a bug to get a back-port.

Just a quick follow-up. The second hunk:

@@ -342,6 +343,8 @@ struct tevent_signal *tevent_common_add_signal(struct tevent_context *ev,
 			return NULL;
 		}
 		if (sigaction(signum, &act, sig_state->oldact[signum]) == -1) {
+			talloc_free(sig_state->oldact[se->signum]);
+			sig_state->oldact[se->signum] = NULL;
 			talloc_free(se);
 			return NULL;
 		}

should use 'signum', not se->signum for consistency. The value
of signum and se->signum are the same at this point, but the
code looks strange there (cut-and-paste error).

If you confirm it works, I'll propose the corrected fix for
master.

Jeremy.

> From 8154eb3a825d0541a2fbafa1844c0c88fe14d263 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Tue, 16 Feb 2016 14:23:53 -0800
> Subject: [PATCH] =?UTF-8?q?lib:=20tevent:=20Fix=20memory=20leak=20reported?=
>  =?UTF-8?q?=20by=20Pavel=20B=C5=99ezina=20<pbrezina at redhat.com>=20when=20o?=
>  =?UTF-8?q?ld=20signal=20action=20restored.?=
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  lib/tevent/tevent_signal.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/tevent/tevent_signal.c b/lib/tevent/tevent_signal.c
> index 924dc05..1c1e60e 100644
> --- a/lib/tevent/tevent_signal.c
> +++ b/lib/tevent/tevent_signal.c
> @@ -212,6 +212,7 @@ static int tevent_signal_destructor(struct tevent_signal *se)
>  		/* restore old handler, if any */
>  		if (sig_state->oldact[se->signum]) {
>  			sigaction(se->signum, sig_state->oldact[se->signum], NULL);
> +			talloc_free(sig_state->oldact[se->signum]);
>  			sig_state->oldact[se->signum] = NULL;
>  		}
>  #ifdef SA_SIGINFO
> @@ -342,6 +343,8 @@ struct tevent_signal *tevent_common_add_signal(struct tevent_context *ev,
>  			return NULL;
>  		}
>  		if (sigaction(signum, &act, sig_state->oldact[signum]) == -1) {
> +			talloc_free(sig_state->oldact[se->signum]);
> +			sig_state->oldact[se->signum] = NULL;
>  			talloc_free(se);
>  			return NULL;
>  		}
> @@ -505,6 +508,7 @@ void tevent_cleanup_pending_signal_handlers(struct tevent_signal *se)
>  	if (sig_state->sig_handlers[se->signum] == NULL) {
>  		if (sig_state->oldact[se->signum]) {
>  			sigaction(se->signum, sig_state->oldact[se->signum], NULL);
> +			talloc_free(sig_state->oldact[se->signum]);
>  			sig_state->oldact[se->signum] = NULL;
>  		}
>  	}
> -- 
> 2.7.0.rc3.207.g0ac5344
>