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):

Passes tests and fixes the leak, though I we need to change the 
_destroy() to be a _cleanup() for safety, and add another.

I think my main issue with this API is that it every single C API ever 
created in history with a _create/_destroy() interface pair has memory 
allocated and deallocated by the creator and destructor.  We should 
remove apr_brigade_destroy() in APR 2, IMO, or switch to bucket 
allocated brigades then.

Having stack-allocated apr_brigade struct might be nice though would 
mean using alloca() which we don't do normally, plus two new APR 
functions to both return the size of the struct and to initialize it.

Regards, Joe

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);