[PATCH] clean the event context after fork in fork_domain_child()

[boyang]

Jeremy Allison wrote:
> On Mon, Jan 05, 2009 at 02:46:07PM +0100, Stefan (metze) Metzmacher wrote:
>   
>> boyang schrieb:
>>     
>>> Stefan (metze) Metzmacher wrote:
>>>       
>>>> Hi BoYang,
>>>>
>>>>   
>>>>         
>>>>>      The point here is that there maybe outstanding requests in parent,
>>>>> and async_request_timeout is set, which cause this child try to kill
>>>>> some other child, probably be primary domain's child. Look, After
>>>>> winbindd started, child for primary domain is forked, then other domains
>>>>> such as BUILTIN. If there are outstanding requests sent to child for
>>>>> primary domain, async_request_timeout handler is set, which is
>>>>> inheritted in child for domain BUILTIN. Later, time expired and child
>>>>> for BUILTIN sends SIGTERM to child for primary domain.
>>>>>      Patches are in the attachment, Thanks!
>>>>>     
>>>>>           
>>>> I think the real solution is to create a new event context in the child
>>>> process (and free the parents context). In the end we should do that
>>>> also within smbd...
>>>>
>>>> metze
>>>>   
>>>>         
>>> Hi, Metze:
>>>      I've finished the patch to clean event context after fork. Please
>>> review it. In event_context_reinit(ev), I freed all timed_event and
>>> fd_event to save some memory. Initial test and 'make test' looks fine.
>>>       
>> Thanks!
>>
>> I think freeing the events is dangerous, as it leaves broken pointers
>> around.
>>     
>
> Yes, I'm planning to add code to go through and clean up
> any events left registered after each fork, and change
> reinit_after_fork() to smbd_reinit_after_fork(), nmbd_reinit_after_fork()
> and winbindd_reinit_after_fork() to make sure each cleans
> up correctly.
>
> I agree with your assessment that leaving the broken pointers
> around is dangerous.
>   
This can lead to double free. That is why I set
domain->check_online_event to NULL(entry->event have to be set too).
Since we didn't free it now, we can not barely set check_online_event to
NULL, which causes memory leak. We must free it after
reinit_after_fork() is called. I'll post the patch later.
>   
>> I integrated it without freeing the events and your krb5 ticket patch
>> in this branch:
>> http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master3-tevent3
>>
>> It would be nice if you could test if that branch fixes still fixes all
>> the problems you wanted to resolve.
>>     
Looks like ccache_regain_all_now relies on entry->refresh_time to judge
whether the handler is krb5_ticket_refresh_handler or
krb5_ticket_gain_handler. Therefore, we must set entry->refresh_time
before event_add_timed to make ccache_regain_all_now work correctly.
I'll post a separate patch later.
>
> These fixes are important enough to delay 3.3.0 for, IMHO.
>
> Thanks Metze and Boyang, really nice work here !
>
> Jeremy.
>
>   

-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 187 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20090106/9d4e306e/boyang.vcf