[PATCH] fix cleanups in cleanups (Was Re: New post-log-transaction hook?)

[Aaron Bannert]

On Wed, Sep 19, 2001 at 12:27:35PM -0700, Jon Travis wrote:
> BZzzzt.  The attached code registers a cleanup from within a cleanup, and
> does so 'correctly'.  See the program attached at the bottom, which behaves 
> incorrectly.  It is simple code, but not knowing that a given
> function registers a cleanup can cause major problems (leaking
> file descriptors, etc. eventually).  The file should contain 'Cleanup',
> because the cleanup of the file should flush the buffer -- that
> cleanup is never run, though.
> 
> > when the cleanup is registered, it is gauranteed to be there when the cleanup
> > is run.
> > 
> > Anything else is completely broken.
> 
> 
> #include "apr.h"
> #include "apr_file_io.h"
> 
> static apr_status_t my_cleanup(void *cbdata){
>     apr_pool_t *p = cbdata;
>     apr_file_t *file;
> 
>     apr_file_open(&file, "/tmp/bonk", 
>                 APR_WRITE | APR_CREATE | APR_TRUNCATE | APR_BUFFERED,
>                 APR_OS_DEFAULT, p);
>     apr_file_printf(file, "Cleanup");
>     return APR_SUCCESS;
> }
> 
> int main(int argc, char *argv[]){
>     apr_pool_t *pool;
> 
>     apr_initialize();
>     apr_pool_create(&pool, NULL);
>     apr_pool_cleanup_register(pool, pool, my_cleanup, NULL);
>     apr_pool_destroy(pool);
>     apr_terminate();
>     return 0;
> }



Does this fix it for you? All testmem tests passed for me and your code
above properly flushes "Cleanup" to the file.

(Someone needs to check my work on run_child_cleanups() and make sure
that the popping is necessary. It looked to be in the same situation.)

-aaron


Index: memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvspublic/apr/memory/unix/apr_pools.c,v
retrieving revision 1.111
diff -u -r1.111 apr_pools.c
--- memory/unix/apr_pools.c     2001/09/17 20:12:23     1.111
+++ memory/unix/apr_pools.c     2001/09/20 18:06:46
@@ -564,7 +564,8 @@
 struct process_chain;
 struct cleanup;
 
-static void run_cleanups(struct cleanup *c);
+static struct cleanup *pop_cleanup(apr_pool_t *p);
+static void run_cleanups(apr_pool_t *p);
 static void free_proc_chain(struct process_chain *p);
 
 static apr_pool_t *permanent_pool;
@@ -764,26 +765,35 @@
     return (*cleanup) (data);
 }
 
-static void run_cleanups(struct cleanup *c)
+static struct cleanup *pop_cleanup(apr_pool_t *p)
 {
-    while (c) {
-       (*c->plain_cleanup) ((void *)c->data);
-       c = c->next;
+    struct cleanup *c;
+    if ((c = p->cleanups)) {
+        p->cleanups = c->next;
+        c->next = NULL;
     }
+    return c;
 }
 
-static void run_child_cleanups(struct cleanup *c)
+static void run_cleanups(apr_pool_t *p)
 {
-    while (c) {
+    struct cleanup *c;
+    while ((c = pop_cleanup(p))) {
+        (*c->plain_cleanup) ((void *)c->data);
+    }
+}
+
+static void run_child_cleanups(apr_pool_t *p)
+{
+    struct cleanup *c;
+    while ((c = pop_cleanup(p))) {
        (*c->child_cleanup) ((void *)c->data);
-       c = c->next;
     }
 }
 
 static void cleanup_pool_for_exec(apr_pool_t *p)
 {
-    run_child_cleanups(p->cleanups);
-    p->cleanups = NULL;
+    run_child_cleanups(p);
 
     for (p = p->sub_pools; p; p = p->sub_next) {
        cleanup_pool_for_exec(p);
@@ -863,8 +873,7 @@
     }
 
     /* run cleanups and free any subprocesses. */
-    run_cleanups(a->cleanups);
-    a->cleanups = NULL;
+    run_cleanups(a);
     free_proc_chain(a->subprocesses);
     a->subprocesses = NULL;