[with the patch]
On Wed, Feb 15, 2017 at 2:07 PM, Yann Ylavic <ylavic....@gmail.com> wrote: > On Tue, Feb 14, 2017 at 10:21 PM, Jacob Champion <champio...@gmail.com> 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; }