Mailing-List: contact new-httpd-help@apache.org; run by ezmlm
Precedence: bulk
Reply-To: new-httpd@apache.org
Date: Thu, 26 Apr 2001 18:41:14 -0400
Message-Id: <200104262241.SAA04468@adsl-77-241-65.rdu.bellsouth.net>
From: Jeff Trawick <trawickj@bellsouth.net>
To: new-httpd@apache.org, dev@apr.apache.org
Subject: [PATCH] do auto-unregister of other child registration
Reply-to: trawickj@bellsouth.net

This is mostly an APR patch but it fixes a symptom (segfault)
associated with mod_cgid after doing "apachectl graceful" followed by
"apachectl restart".

I turned on Greg Stein's mprotect-based memory debugging to help test
it out.

Before, we were leaving registrations in the other_children list after
the pool from which the list element was allocated was destroyed.

With this patch, we use the normal APR auto-cleanup design ith the
other-child registration so that we don't leave the registration
dangling.

The cleanup is a bit hokey because of the API; because
apr_proc_other_child_unregister() is given a user data ptr instead of
an apr_foo_t, we have to search the private list of registrations to
find the pool with which the cleanup is associated.

With this patch it takes a couple of secs for mod_cgid to go away, so
maybe we should be doing an explicit unregister somewhere, but at
least the auto unregister seems to work.

I have pretty low confidence in my understanding of anything to do
with APR other-child code, so I look forward to being pointed in the
right direction.

Index: modules/generators/mod_cgid.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/generators/mod_cgid.c,v
retrieving revision 1.83
diff -u -r1.83 mod_cgid.c
--- modules/generators/mod_cgid.c	2001/04/03 19:12:14	1.83
+++ modules/generators/mod_cgid.c	2001/04/26 22:34:49
@@ -242,7 +242,7 @@
             apr_proc_other_child_unregister(data);
             break;
         case APR_OC_REASON_UNREGISTER:
-            apr_pool_destroy(pcgi);
+            /* don't apr_pool_destroy(pcgi); its cleanup drives this path */
             kill(*sd, SIGHUP);
             break;
     }
Index: srclib/apr/include/arch/unix/misc.h
===================================================================
RCS file: /home/cvspublic/apr/include/arch/unix/misc.h,v
retrieving revision 1.25
diff -u -r1.25 misc.h
--- srclib/apr/include/arch/unix/misc.h	2001/02/25 20:39:32	1.25
+++ srclib/apr/include/arch/unix/misc.h	2001/04/26 22:35:13
@@ -88,6 +88,7 @@
 #endif
 
 struct apr_other_child_rec_t {
+    apr_pool_t *p;
     struct apr_other_child_rec_t *next;
     int id;  /* This is either a pid or tid depending on the platform */
     void (*maintenance) (int, void *, int);
Index: srclib/apr/misc/unix/otherchild.c
===================================================================
RCS file: /home/cvspublic/apr/misc/unix/otherchild.c,v
retrieving revision 1.20
diff -u -r1.20 otherchild.c
--- srclib/apr/misc/unix/otherchild.c	2001/02/16 04:15:56	1.20
+++ srclib/apr/misc/unix/otherchild.c	2001/04/26 22:35:17
@@ -71,6 +71,22 @@
 
 static apr_other_child_rec_t *other_children = NULL;
 
+static apr_status_t other_child_cleanup(void *data)
+{
+    apr_other_child_rec_t **pocr, *nocr;
+
+    for (pocr = &other_children; *pocr; pocr = &(*pocr)->next) {
+        if ((*pocr)->data == data) {
+            nocr = (*pocr)->next;
+            (*(*pocr)->maintenance) (APR_OC_REASON_UNREGISTER, (*pocr)->data, -1);
+            *pocr = nocr;
+            /* XXX: um, well we've just wasted some space in pconf ? */
+            return APR_SUCCESS;
+        }
+    }
+    return APR_SUCCESS;
+}
+
 APR_DECLARE(void) apr_proc_other_child_register(apr_proc_t *pid,
                      void (*maintenance) (int reason, void *, int status),
                      void *data, apr_file_t *write_fd, apr_pool_t *p)
@@ -78,6 +94,7 @@
     apr_other_child_rec_t *ocr;
 
     ocr = apr_palloc(p, sizeof(*ocr));
+    ocr->p = p;
     ocr->id = pid->pid;
     ocr->maintenance = maintenance;
     ocr->data = data;
@@ -89,21 +106,25 @@
     }
     ocr->next = other_children;
     other_children = ocr;
+    apr_pool_cleanup_register(p, ocr->data, other_child_cleanup, 
+                              apr_pool_cleanup_null);
 }
 
 APR_DECLARE(void) apr_proc_other_child_unregister(void *data)
 {
-    apr_other_child_rec_t **pocr, *nocr;
+    apr_other_child_rec_t *cur;
 
-    for (pocr = &other_children; *pocr; pocr = &(*pocr)->next) {
-        if ((*pocr)->data == data) {
-            nocr = (*pocr)->next;
-            (*(*pocr)->maintenance) (APR_OC_REASON_UNREGISTER, (*pocr)->data, -1);
-            *pocr = nocr;
-            /* XXX: um, well we've just wasted some space in pconf ? */
-            return;
+    cur = other_children;
+    while (cur) {
+        if (cur->data == data) {
+            break;
         }
+        cur = cur->next;
     }
+
+    /* segfault if this function called with invalid parm */
+    apr_pool_cleanup_kill(cur->p, cur->data, other_child_cleanup);
+    other_child_cleanup(data);
 }
 
 /* test to ensure that the write_fds are all still writable, otherwise

-- 
Jeff Trawick | trawickj@bellsouth.net | PGP public key at web site:
       http://www.geocities.com/SiliconValley/Park/9289/
             Born in Roswell... married an alien...