httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacob Champion <champio...@gmail.com>
Subject Re: svn commit: r1707087 - /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c
Date Tue, 25 Apr 2017 21:58:17 GMT
On 02/15/2017 05:08 AM, Yann Ylavic wrote:
> [with the patch]
>
> On Wed, Feb 15, 2017 at 2:07 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>> Does the attached patch work for you?
>> I don't like it too much (if ever relevent), we could also possibly
>> special case the EOR brigade (looks a bit hacky to me) or create
>> tmp_bb on c->pool (and leak a few bytes per request, like
>> ap_process_request_after_handler() already)...
>>
>> Ideally we'd have c->tmp_bb...

Hi Yann, circling back to this crash at last...

Unfortunately the patch just moves the crash to another location. We 
can't call APR_BRIGADE_EMPTY() on a brigade that's pointing at junk. I 
think a bucket that cleans up the brigade it's a part of is just not a 
good idea. :) So moving to c->pool is an option for a quick fix, I 
suppose...

...but I'm more inclined to look at the whole EOR bucket situation -- 
specifically eor_bucket_destroy() and its helpers. Why is the EOR bucket 
responsible for freeing the request's pool in the first place? It 
doesn't own the request. Surely we should be freeing the pool in the 
same code context in which we've allocated the pool?

One of the comments in eor_bucket.c is "eor_bucket_destroy might be 
called at a point of time when the request pool had been already 
destroyed", which makes me incredibly suspicious of the whole thing. 
Finalizers are not a great place to put business logic.

--Jacob

Mime
View raw message