On 02/15/2017 05:08 AM, Yann Ylavic wrote:
[with the patch]
On Wed, Feb 15, 2017 at 2:07 PM, Yann Ylavic <[email protected]> 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
