samba-technical@lists.samba.org
[Top] [All Lists]

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

Subject: Re: [PATCH] clean the event context after fork in fork_domain_child()
From: boyang
Date: Wed, 07 Jan 2009 21:42:38 +0800
Stefan (metze) Metzmacher wrote:
> boyang schrieb:
>   
>> Jeremy Allison wrote:
>>     
>>> On Tue, Jan 06, 2009 at 01:19:41PM +0800, boyang wrote:
>>>   
>>>       
>>>> Jeremy Allison wrote:
>>>>     
>>>>         
>>> Ok, I've finished doing a lot of cleanup work on the
>>> event code in winbindd. It's all checked into 3.3 and
>>> master. Please review.
>>>
>>> If you feel it's working, feel free to back-port to 3.2
>>> and/or 3.0.x and I'll commit patches for you. I'm not
>>> doing that work yet as I'm not sure if we're doing any
>>> more 3.2.x releases or just moving on to 3.3.0.
>>>   
>>>       
>> Hi, Jeremy:
>>      Your patch is good. I have done some initial test on it. And I
>> think we can do some work more to clean the context.
>> 1*  _client_list or winbindd_client_list() is never used in child,
>> destroy and free it.
>> 2* file descriptors come from listen socket in parent, accepted socket
>> in parent, socket pairs in parent are never used, close it and delete is
>> from fd_events.
>> 3* delete all memory credentials too as newly forked child won't use it.
>> I did some initial test and it works.
>> Patches are in the attachment, please review it. Thanks!
>>     
>>> Jeremy.
>>>
>>>   
>>>       
>
> Why do you use the deprecated talloc_destroy() ?
>   
Hi, Jeremy && metze:
       This is the updated version of the patch.
1* Got rid of deprecated talloc_destroy() and use TALLOC_FREE().
2* do a test to determine if there is  opened child->event.fd.
Pleas review it. Thanks!
> metze
>
>
>   

>From ff7869862ac66ad49d6dff5a5dfd8c3995d297b0 Mon Sep 17 00:00:00 2001
From: Bo Yang <boyang@xxxxxxxxxx>
Date: Wed, 7 Jan 2009 21:34:25 +0800
Subject: [PATCH] more code to clean context after fork

---
 source3/winbindd/winbindd_cred_cache.c |   24 ++++++++++
 source3/winbindd/winbindd_dual.c       |   74 ++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 0 deletions(-)

diff --git a/source3/winbindd/winbindd_cred_cache.c 
b/source3/winbindd/winbindd_cred_cache.c
index ff6d0f3..8de4769 100644
--- a/source3/winbindd/winbindd_cred_cache.c
+++ b/source3/winbindd/winbindd_cred_cache.c
@@ -956,6 +956,30 @@ NTSTATUS winbindd_add_memory_creds(const char *username,
        return status;
 }
 
+void memory_creds_remove_all_after_fork(void)
+{
+       struct WINBINDD_MEMORY_CREDS *memcredp;
+       struct WINBINDD_CCACHE_ENTRY *entry;
+       
+       while (memory_creds_list) {
+               memcredp = memory_creds_list;
+               DLIST_REMOVE(memory_creds_list, memcredp);
+               entry = get_ccache_by_username(memcredp->username);
+               /* all entries are freed after fork, there is no way
+                * that one entry is still pointing to this memory
+                * creds. */
+               if (entry) {
+                       DEBUG(0, ("memory_creds_remove_all_after_fork: "
+                                 "memory creds is still used by entry!\n"));
+                       smb_panic("memory_creds_remove_all_after_fork: "
+                                 "memory credentials is still used after fork "
+                                 "entries are all destroyed!!\n");
+               }
+               delete_memory_creds(memcredp);
+               TALLOC_FREE(memcredp);
+       }       
+}
+
 /*************************************************************
  Decrement the in-memory ref count - delete if zero.
 *************************************************************/
diff --git a/source3/winbindd/winbindd_dual.c b/source3/winbindd/winbindd_dual.c
index c1b1efb..916ef3b 100644
--- a/source3/winbindd/winbindd_dual.c
+++ b/source3/winbindd/winbindd_dual.c
@@ -1142,6 +1142,55 @@ static void child_msg_dump_event_list(struct 
messaging_context *msg,
        dump_event_list(winbind_event_context());
 }
 
+static void winbindd_remove_client_after_fork(struct winbindd_cli_state 
*state);
+static void close_winbindd_socket_after_fork(void);
+static void winbindd_remove_all_clients_after_fork(void);
+void memory_creds_remove_all_after_fork(void);
+
+static void close_winbindd_socket_after_fork(void)
+{
+       close_winbindd_socket();
+}
+
+static void winbindd_remove_client_after_fork(struct winbindd_cli_state *state)
+{
+       if (!state) {
+               return;
+       }
+
+       /* Close socket */
+       close(state->sock);
+
+       /* Free any getent state */
+       free_getent_state(state->getpwent_state);
+       free_getent_state(state->getgrent_state);
+
+       /* We may have some extra data that was not freed if the client was
+          killed unexpectedly */
+       SAFE_FREE(state->response.extra_data.data);
+       /* The request is not finished, free request.extra.data */
+       SAFE_FREE(state->response.extra_data.data);
+
+       TALLOC_FREE(state->mem_ctx);
+
+       remove_fd_event(&state->fd_event);
+
+       /* Remove from list and free */
+       winbindd_remove_client(state);
+       TALLOC_FREE(state);
+}
+
+static void winbindd_remove_all_clients_after_fork(void)
+{
+       struct winbindd_cli_state *state, *next;
+       state = winbindd_client_list();
+       while (state) {
+               next = state->next;
+               winbindd_remove_client_after_fork(state);
+               state = next;
+       }
+}
+
 bool winbindd_reinit_after_fork(const char *logfilename)
 {
        struct winbindd_domain *domain;
@@ -1199,10 +1248,35 @@ bool winbindd_reinit_after_fork(const char *logfilename)
 
                for (request = cl->requests; request; request = request->next) {
                        TALLOC_FREE(request->reply_timeout_event);
+                       /* remove request from the list, it will be freed in
+                        * TALLOC_FREE(state->mem_ctx). We are in child, we 
+                        * won't process any requests. */
+                       DLIST_REMOVE(cl->requests, request);
+                       /* close the socketpair socket on the parent
+                        * side. */
+                       if (cl->event.fd) {
+                               close(cl->event.fd);
+                       }
+                       remove_fd_event(&cl->event);
                }
                TALLOC_FREE(cl->lockout_policy_event);
                TALLOC_FREE(cl->machine_password_change_event);
         }
+       
+       /* why not free all unused memory and close all unused sockets? */
+       /* Look. file descriptor come from listen socket, accepted connections,
+        * socket pairs between child and parent. */
+       /* Clsoing listen socket in child, unused. */
+       close_winbindd_socket_after_fork();
+       /* remove all clients inherited from parent. */
+       /* fd of accepted connection will be closed here. */
+       winbindd_remove_all_clients_after_fork();
+       /* remove memory credentials after fork. */
+       memory_creds_remove_all_after_fork();
+       /* Hmmm, What will we do with domain list and child list?
+        * leave them untact here? or delete any domains other than
+        * the primary domain and the domain this child works for?
+        * */
 
        return true;
 }
-- 
1.5.3

>From 79df151087c543cffba19c6124d874ea46fef0c6 Mon Sep 17 00:00:00 2001
From: Bo Yang <boyang@xxxxxxxxxx>
Date: Wed, 7 Jan 2009 21:38:20 +0800
Subject: [PATCH] more code to clean context after fork

---
 source/winbindd/winbindd_cred_cache.c |   24 +++++++++++
 source/winbindd/winbindd_dual.c       |   74 +++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 0 deletions(-)

diff --git a/source/winbindd/winbindd_cred_cache.c 
b/source/winbindd/winbindd_cred_cache.c
index ff6d0f3..8de4769 100644
--- a/source/winbindd/winbindd_cred_cache.c
+++ b/source/winbindd/winbindd_cred_cache.c
@@ -956,6 +956,30 @@ NTSTATUS winbindd_add_memory_creds(const char *username,
        return status;
 }
 
+void memory_creds_remove_all_after_fork(void)
+{
+       struct WINBINDD_MEMORY_CREDS *memcredp;
+       struct WINBINDD_CCACHE_ENTRY *entry;
+       
+       while (memory_creds_list) {
+               memcredp = memory_creds_list;
+               DLIST_REMOVE(memory_creds_list, memcredp);
+               entry = get_ccache_by_username(memcredp->username);
+               /* all entries are freed after fork, there is no way
+                * that one entry is still pointing to this memory
+                * creds. */
+               if (entry) {
+                       DEBUG(0, ("memory_creds_remove_all_after_fork: "
+                                 "memory creds is still used by entry!\n"));
+                       smb_panic("memory_creds_remove_all_after_fork: "
+                                 "memory credentials is still used after fork "
+                                 "entries are all destroyed!!\n");
+               }
+               delete_memory_creds(memcredp);
+               TALLOC_FREE(memcredp);
+       }       
+}
+
 /*************************************************************
  Decrement the in-memory ref count - delete if zero.
 *************************************************************/
diff --git a/source/winbindd/winbindd_dual.c b/source/winbindd/winbindd_dual.c
index e066944..81bf9d9 100644
--- a/source/winbindd/winbindd_dual.c
+++ b/source/winbindd/winbindd_dual.c
@@ -1142,6 +1142,55 @@ static void child_msg_dump_event_list(struct 
messaging_context *msg,
        dump_event_list(winbind_event_context());
 }
 
+static void winbindd_remove_client_after_fork(struct winbindd_cli_state 
*state);
+static void close_winbindd_socket_after_fork(void);
+static void winbindd_remove_all_clients_after_fork(void);
+void memory_creds_remove_all_after_fork(void);
+
+static void close_winbindd_socket_after_fork(void)
+{
+       close_winbindd_socket();
+}
+
+static void winbindd_remove_client_after_fork(struct winbindd_cli_state *state)
+{
+       if (!state) {
+               return;
+       }
+
+       /* Close socket */
+       close(state->sock);
+
+       /* Free any getent state */
+       free_getent_state(state->getpwent_state);
+       free_getent_state(state->getgrent_state);
+
+       /* We may have some extra data that was not freed if the client was
+          killed unexpectedly */
+       SAFE_FREE(state->response.extra_data.data);
+       /* The request is not finished, free request.extra.data */
+       SAFE_FREE(state->response.extra_data.data);
+
+       TALLOC_FREE(state->mem_ctx);
+
+       remove_fd_event(&state->fd_event);
+
+       /* Remove from list and free */
+       winbindd_remove_client(state);
+       TALLOC_FREE(state);
+}
+
+static void winbindd_remove_all_clients_after_fork(void)
+{
+       struct winbindd_cli_state *state, *next;
+       state = winbindd_client_list();
+       while (state) {
+               next = state->next;
+               winbindd_remove_client_after_fork(state);
+               state = next;
+       }
+}
+
 bool winbindd_reinit_after_fork(const char *logfilename)
 {
        struct winbindd_domain *domain;
@@ -1199,10 +1248,35 @@ bool winbindd_reinit_after_fork(const char *logfilename)
 
                for (request = cl->requests; request; request = request->next) {
                        TALLOC_FREE(request->reply_timeout_event);
+                       /* remove request from the list, it will be freed in
+                        * TALLOC_FREE(state->mem_ctx). We are in child, we 
+                        * won't process any requests. */
+                       DLIST_REMOVE(cl->requests, request);
+                       /* close the socketpair socket on the parent
+                        * side. */
+                       if (cl->event.fd) {
+                               close(cl->event.fd);
+                       }
+                       remove_fd_event(&cl->event);
                }
                TALLOC_FREE(cl->lockout_policy_event);
                TALLOC_FREE(cl->machine_password_change_event);
         }
+       
+       /* why not free all unused memory and close all unused sockets? */
+       /* Look. file descriptor come from listen socket, accepted connections,
+        * socket pairs between child and parent. */
+       /* Clsoing listen socket in child, unused. */
+       close_winbindd_socket_after_fork();
+       /* remove all clients inherited from parent. */
+       /* fd of accepted connection will be closed here. */
+       winbindd_remove_all_clients_after_fork();
+       /* remove memory credentials after fork. */
+       memory_creds_remove_all_after_fork();
+       /* Hmmm, What will we do with domain list and child list?
+        * leave them untact here? or delete any domains other than
+        * the primary domain and the domain this child works for?
+        * */
 
        return true;
 }
-- 
1.5.3

Attachment: boyang.vcf
Description: Vcard

<Prev in Thread] Current Thread [Next in Thread>