AW: Brigade memory lifetime & leaks

[Plüm , Rüdiger , Vodafone Group]

> -----Ursprüngliche Nachricht-----
> Von: Stefan Eissing <stefan.eiss...@greenbytes.de>
> Gesendet: Dienstag, 5. Juni 2018 13:27
> An: dev@httpd.apache.org
> Cc: d...@apr.apache.org
> Betreff: Re: Brigade memory lifetime & leaks
> 
> 
> 
> > Am 05.06.2018 um 10:46 schrieb Joe Orton <jor...@redhat.com>:
> >
> > In 2.4's http_request.c there are two places doing:
> >
> >    bb = apr_brigade_create(c->pool, c->bucket_alloc);
> >
> > to handle sending a FLUSH between requests and an EOR bucket which
> both
> > can't be done off r->pool.  Because the brigade structure itself is
> > allocated out of c->pool this means we leak (~64bytes) per request
> > forever or up to request/conn limits.
> 
> Ok, not good. The first seems only to be triggered when request
> data is still pending, but the second one always happens.
> 
> > We can thread a temp pool through to those functions and fix this,
> > though because these core internal functions are in the public API (ha
> > ha) this is the usual compat PITA, and I haven't worked out if that's
> > more complex for the the async requesta processing call chain.
> > As a PoC for non-async MPMs:
> > http://people.apache.org/~jorton/2.4-use-temp-pool.patch
> >
> > Another choice is to allocate the brigade structure using the bucket
> > allocator and actually free it on _destroy().  Anybody around who can
> > remember why we used a pool allocation for that structure from the
> > beginning?
> 
> How about having the apr_bucket_brigade struct on the stack as an
> option?

The usual approach in many locations is to store the created bucket brigade
and reuse it. How about the following (untested):

Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1832516)
+++ 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);
 
@@ -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);


Regards

Rüdiger