tevent memory leak?

[Pavel Březina]

On 02/17/2016 10:53 AM, Pavel Březina wrote:
> 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.

Ok, I can confirm that the patch works as expected.