On Tue, Jun 15, 2004 at 03:23:42PM +0100, Joe Orton wrote:
> http://issues.apache.org/bugzilla/show_bug.cgi?id=23567
OK, the issues I was having with using the bucket allocator to allocate
the brigade were just my own screwups. So, the patches needed to fix
this issue are attached:
1) re-use pool cleanup structures
2) allocate brigade structures using the bucket allocator
these combine to prevent unbounded memory consumption when using
apr_brigade_split() from an output filter.
(2) means that use of a brigade after it has been destroyed will now
result in a segfault. The test suite catches a particularly nice place,
which leads to:
3) don't destroy the brigade being flushed in ap_filter_flush; pass down
a temporary brigade.
(looks like mod_bucketeer is also screwed like this, still working on
that one)
Comments?
joe
Index: memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v
retrieving revision 1.205
diff -u -r1.205 apr_pools.c
--- memory/unix/apr_pools.c 13 Feb 2004 09:38:32 -0000 1.205
+++ memory/unix/apr_pools.c 15 Jun 2004 18:53:02 -0000
@@ -435,6 +435,7 @@
apr_pool_t *sibling;
apr_pool_t **ref;
cleanup_t *cleanups;
+ cleanup_t *free_cleanups;
apr_allocator_t *allocator;
struct process_chain *subprocesses;
apr_abortfunc_t abort_fn;
@@ -674,6 +675,7 @@
/* Run cleanups */
run_cleanups(&pool->cleanups);
pool->cleanups = NULL;
+ pool->free_cleanups = NULL;
/* Free subprocesses */
free_proc_chain(pool->subprocesses);
@@ -801,6 +803,7 @@
pool->abort_fn = abort_fn;
pool->child = NULL;
pool->cleanups = NULL;
+ pool->free_cleanups = NULL;
pool->subprocesses = NULL;
pool->user_data = NULL;
pool->tag = NULL;
@@ -1340,7 +1343,7 @@
/* Run cleanups */
run_cleanups(&pool->cleanups);
- pool->cleanups = NULL;
+ pool->free_cleanups = pool->cleanups = NULL;
/* If new child pools showed up, this is a reason to raise a flag */
if (pool->child)
@@ -1886,7 +1889,13 @@
#endif /* APR_POOL_DEBUG */
if (p != NULL) {
- c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
+ if (p->free_cleanups) {
+ /* reuse a cleanup structure */
+ c = p->free_cleanups;
+ p->free_cleanups = c->next;
+ } else {
+ c = (cleanup_t *)apr_palloc(p, sizeof(cleanup_t));
+ }
c->data = data;
c->plain_cleanup_fn = plain_cleanup_fn;
c->child_cleanup_fn = child_cleanup_fn;
@@ -1912,6 +1921,9 @@
while (c) {
if (c->data == data && c->plain_cleanup_fn == cleanup_fn) {
*lastp = c->next;
+ /* move to freelist */
+ c->next = p->free_cleanups;
+ p->free_cleanups = c;
break;
}
Index: buckets/apr_brigade.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.60
diff -u -r1.60 apr_brigade.c
--- buckets/apr_brigade.c 13 Feb 2004 09:55:25 -0000 1.60
+++ buckets/apr_brigade.c 15 Jun 2004 15:34:47 -0000
@@ -30,7 +30,9 @@
static apr_status_t brigade_cleanup(void *data)
{
- return apr_brigade_cleanup(data);
+ apr_status_t rv = apr_brigade_cleanup(data);
+ apr_bucket_free(data);
+ return rv;
}
APU_DECLARE(apr_status_t) apr_brigade_cleanup(void *data)
@@ -42,14 +44,12 @@
e = APR_BRIGADE_FIRST(b);
apr_bucket_delete(e);
}
- /* We don't need to free(bb) because it's allocated from a pool. */
return APR_SUCCESS;
}
APU_DECLARE(apr_status_t) apr_brigade_destroy(apr_bucket_brigade *b)
{
- apr_pool_cleanup_kill(b->p, b, brigade_cleanup);
- return apr_brigade_cleanup(b);
+ return apr_pool_cleanup_run(b->p, b, brigade_cleanup);
}
APU_DECLARE(apr_bucket_brigade *) apr_brigade_create(apr_pool_t *p,
@@ -57,7 +57,7 @@
{
apr_bucket_brigade *b;
- b = apr_palloc(p, sizeof(*b));
+ b = apr_bucket_alloc(sizeof *b, list);
b->p = p;
b->bucket_alloc = list;
Index: server/util_filter.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.99
diff -u -r1.99 util_filter.c
--- server/util_filter.c 9 Feb 2004 20:40:49 -0000 1.99
+++ server/util_filter.c 15 Jun 2004 18:51:14 -0000
@@ -547,8 +547,12 @@
void *ctx)
{
ap_filter_t *f = ctx;
+ apr_bucket_brigade *tmp_bb;
- return ap_pass_brigade(f, bb);
+ tmp_bb = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
+ APR_BRIGADE_CONCAT(tmp_bb, bb);
+
+ return ap_pass_brigade(f, tmp_bb);
}
AP_DECLARE(apr_status_t) ap_fflush(ap_filter_t *f, apr_bucket_brigade *bb)
