tevent memory leak?

[Pavel Březina]

On 02/17/2016 12:11 AM, Jeremy Allison wrote:
> 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

Hi,
I don't have an environment handy to build tevent, but the patch LGTM. 
It is basically the same I wanted to send. I'm attaching a simple 
reproducer, you can run:

$ ./a.out | grep sigaction | wc -l

It will print out 100 without the patch, I expect 0 with the patch.


-------------- next part --------------
#include <talloc.h>
#include <tevent.h>
#include <stdio.h>
#include <signal.h>

#define SIGNUM 100

static void handler(struct tevent_context *ev,
                    struct tevent_signal *se,
                    int signum,
                    int count,
                    void *siginfo,
                    void *private_data)
{
    return;
}

int main()
{
    struct tevent_context *ev;
    struct tevent_signal *se;
    int i;

    talloc_enable_null_tracking();

    ev = tevent_context_init(NULL);
    if (ev == NULL) {
        fprintf(stderr, "Unable to setup tevent context\n", i);
        return 1;
    }

    for (i = 0; i < SIGNUM; i++) {
        se = tevent_add_signal(ev, ev, SIGUSR1, SA_SIGINFO, handler, NULL);
        if (se == NULL) {
            fprintf(stderr, "Unable to setup signal %d\n", i);
            return 1;
        }

        talloc_free(se); /* so the handler can be setup again */
    }

    talloc_free(ev);

    talloc_report_full(NULL, stdout);

    return 0;
}