Re: Brigade memory lifetime & leaks

[Joe Orton]

On Tue, Jun 05, 2018 at 12:07:31PM +0000, Plüm, Rüdiger, Vodafone Group wrote:
> The usual approach in many locations is to store the created bucket brigade
> and reuse it. How about the following (untested):

Yes, that works too, thanks!  Passes test suite and fixes the leak, 
though I we need to change the _destroy() to be a _cleanup() for safety, 
and add another, as in attached.

I think my long standing hatred for this particular function is because 
every single C API ever created in history with a foo_create() 
foo_destroy() pair has memory allocated by the creator and deallocated 
by the destructor.  But this one is special!

We should remove apr_brigade_destroy() in APR 2, IMO, or switch to 
bucket allocated brigades then.


Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1832932)
+++ modules/http/http_request.c (working copy)
@@ -345,6 +345,16 @@
     return rv;
 }
 
+#define RETRIEVE_BRIGADE_FROM_POOL(bb, key, pool, allocator) do {       \
+    apr_pool_userdata_get((void **)&bb, key, pool);                     \
+    if (bb == NULL) {                                                   \
+        bb = apr_brigade_create(pool, allocator);                       \
+        apr_pool_userdata_setn((const void *)bb, key, NULL, pool);      \
+    }                                                                   \
+    else {                                                              \
+        apr_brigade_cleanup(bb);                                        \
+    }                                                                   \
+} while(0)
 
 AP_DECLARE(void) ap_process_request_after_handler(request_rec *r)
 {
@@ -357,7 +367,8 @@
      * this bucket is destroyed, the request will be logged and
      * its pool will be freed
      */
-    bb = apr_brigade_create(c->pool, c->bucket_alloc);
+    RETRIEVE_BRIGADE_FROM_POOL(bb, "ap_process_request_after_handler_brigade",
+                               c->pool, c->bucket_alloc);
     b = ap_bucket_eor_create(c->bucket_alloc, r);
     APR_BRIGADE_INSERT_HEAD(bb, b);
 
@@ -383,7 +394,7 @@
      */
     rv = ap_check_pipeline(c, bb, DEFAULT_LIMIT_BLANK_LINES);
     c->data_in_input_filters = (rv == APR_SUCCESS);
-    apr_brigade_destroy(bb);
+    apr_brigade_cleanup(bb);
 
     if (c->cs)
         c->cs->state = (c->aborted) ? CONN_STATE_LINGER
@@ -477,7 +488,8 @@
     ap_process_async_request(r);
 
     if (!c->data_in_input_filters) {
-        bb = apr_brigade_create(c->pool, c->bucket_alloc);
+        RETRIEVE_BRIGADE_FROM_POOL(bb, "ap_process_request_brigade", 
+                                   c->pool, c->bucket_alloc);
         b = apr_bucket_flush_create(c->bucket_alloc);
         APR_BRIGADE_INSERT_HEAD(bb, b);
         rv = ap_pass_brigade(c->output_filters, bb);
@@ -490,6 +502,7 @@
             ap_log_cerror(APLOG_MARK, APLOG_INFO, rv, c, APLOGNO(01581)
                           "flushing data to the client");
         }
+        apr_brigade_cleanup(bb);
     }
     if (ap_extended_status) {
         ap_time_process_request(c->sbh, STOP_PREQUEST);