[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Brigade memory lifetime & leaks

> Am 05.06.2018 um 10:46 schrieb Joe Orton <jorton@xxxxxxxxxx>:
> 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?

> http://people.apache.org/~jorton/apr-util-brigade-bucket-alloc.patch
> plus http://people.apache.org/~jorton/2.4-missing-brigade-destroy.patch
> This seems like a simple change but it could have some nasty regressions 
> if there places where a brigade is reused after calling 
> apr_brigade_destroy().  Currently that itself can create leaks because 
> there is no pool cleanup but will otherwise work.  I'm also not sure if 
> there are performance implications.
> Also I'd previously written that it was a bad idea to ever call 
> apr_brigade_destroy() so this is kind of reversing that advice... 
> https://httpd.apache.org/docs/trunk/developer/output-filters.html#brigade
> Whether or not this is a good idea in apr-util 1.x I'm not at all sure.
> Any thoughts?
> Regards, Joe