[with the patch]
On Wed, Feb 15, 2017 at 2:07 PM, Yann Ylavic <[email protected]> wrote:
> On Tue, Feb 14, 2017 at 10:21 PM, Jacob Champion <[email protected]> wrote:
>>
>
> , hence the (default_)handler probably returned
>>
>>> Admittedly bucketeer_out_filter() is not very nice because it does not
>>> "consume" its given brigade (nor does default_handler() clear it
>>> afterward), but that shouldn't be an issue since anyway nothing should
>>> use them once the request is destroyed.
>>>
>>> Do you have a backtrace of the crash (and/or a breakpoint in
>>> bucketeer_out_filter() for the test)?
>>
>>
>> Attached.
>
> Thanks, it shows the request being destroyed with the EOR bucket.
> However the brigade containing the EOR is also allocated on r->pool,
> hence remove_empty_buckets()'s loop crashes (AIUI).
>
> Here is the (reverse) backtrace:
>
> #17 0x0000000000488070 in ap_process_request_after_handler
> (r=0x7fa70980d0a0) at modules/http/http_request.c:366
> #16 0x000000000043a4d6 in ap_pass_brigade (next=0x7fa70981b7d0,
> bb=0x7fa7097fe088) at server/util_filter.c:610
> [...]
> #8 0x00000000004554ca in ap_request_core_filter (f=0x7fa70980ea78,
> bb=0x7fa7097fe088) at
> #7 0x000000000043a4d6 in ap_pass_brigade (next=0x7fa709821cc0,
> bb=0x7fa709821c80) at server/util_filter.c:610
> [...]
> #2 0x00000000004579c0 in ap_core_output_filter (f=0x7fa7097fdcc8,
> bb=0x7fa709821c80) at server/core_filters.c:467
> #1 0x0000000000457b32 in send_brigade_nonblocking (s=0x7fa7098250b0,
> bb=0x7fa709821c80, bytes_written=0x7fa7097fe040, c=0x7fa709825348) at
> server/core_filters.c:511
> #0 0x0000000000457f98 in remove_empty_buckets (bb=0x7fa709821c80) at
> server/core_filters.c:604
>
> See how frame #8 changes "bb" to its own "tmp_bb" (allocated on r->pool).
> Since ap_request_core_filter() is trunk only (r1706669), it also
> explains why it does not happen in 2.4.x.
>
> 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...
>
> Graham/others, a better idea?
Index: server/request.c
===================================================================
--- server/request.c (revision 1783088)
+++ server/request.c (working copy)
@@ -2056,12 +2056,13 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
if (!tmp_bb) {
tmp_bb = f->ctx = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
}
+ APR_BRIGADE_CONCAT(tmp_bb, bb);
/* Reinstate any buffered content */
- ap_filter_reinstate_brigade(f, bb, &flush_upto);
+ ap_filter_reinstate_brigade(f, tmp_bb, &flush_upto);
- while (!APR_BRIGADE_EMPTY(bb)) {
- apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
+ while (!APR_BRIGADE_EMPTY(tmp_bb)) {
+ apr_bucket *bucket = APR_BRIGADE_FIRST(tmp_bb);
/* if the core has set aside data, back off and try later */
if (!flush_upto) {
@@ -2091,9 +2092,9 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
/* pass each bucket down the chain */
APR_BUCKET_REMOVE(bucket);
- APR_BRIGADE_INSERT_TAIL(tmp_bb, bucket);
+ APR_BRIGADE_INSERT_TAIL(bb, bucket);
- status = ap_pass_brigade(f->next, tmp_bb);
+ status = ap_pass_brigade(f->next, bb);
if (!APR_STATUS_IS_EOF(status) && (status != APR_SUCCESS)) {
return status;
}
@@ -2100,7 +2101,7 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
}
- ap_filter_setaside_brigade(f, bb);
+ ap_filter_setaside_brigade(f, tmp_bb);
return status;
}
