On 3/26/20 2:00 PM, Yann Ylavic wrote:
>
> Wouldn't that work (patch attached, passes test framework and Joe's repro)?
>
>
Index: server/util_filter.c
===================================================================
--- server/util_filter.c (revision 1875498)
+++ server/util_filter.c (working copy)
@@ -945,34 +956,56 @@ AP_DECLARE(apr_status_t) ap_filter_setaside_brigad
(!fp->bb || APR_BRIGADE_EMPTY(fp->bb)) ? "empty" : "full",
f->frec->name);
+ /* Buckets in fp->bb are leftover from previous call to reinstate, so
+ * they happen after the buckets (re)set aside here.
+ */
+ if (fp->bb) {
+ APR_BRIGADE_CONCAT(bb, fp->bb);
+ }
+
How could it happend that fp-bb is not empty here?
if (!APR_BRIGADE_EMPTY(bb)) {
+ apr_bucket_brigade *tmp_bb = NULL;
+
/*
- * Set aside the brigade bb within fp->bb.
+ * Set aside the brigade bb to fp->bb.
*/
+
ap_filter_prepare_brigade(f);
+ do {
+ apr_bucket *e = APR_BRIGADE_FIRST(bb);
+ APR_BUCKET_REMOVE(e);
- /* decide what pool we setaside to, request pool or deferred pool? */
- if (f->r) {
- apr_bucket *e;
- for (e = APR_BRIGADE_FIRST(bb); e != APR_BRIGADE_SENTINEL(bb); e =
- APR_BUCKET_NEXT(e)) {
- if (APR_BUCKET_IS_TRANSIENT(e)) {
- int rv = apr_bucket_setaside(e, f->r->pool);
+ /* Assume that morphing buckets have the correct lifetime! */
+ if (AP_BUCKET_IS_MORPHING(e)) {
+ if (tmp_bb && !APR_BRIGADE_EMPTY(tmp_bb)) {
+ /* Save non-morphing buckets batched below. */
+ rv = save_aside_brigade(fp, tmp_bb);
if (rv != APR_SUCCESS) {
- return rv;
+ APR_BRIGADE_PREPEND(bb, tmp_bb);
+ break;
}
}
+ APR_BRIGADE_INSERT_TAIL(fp->bb, e);
}
- APR_BRIGADE_CONCAT(fp->bb, bb);
- }
- else {
- if (!fp->deferred_pool) {
- apr_pool_create(&fp->deferred_pool, f->c->pool);
- apr_pool_tag(fp->deferred_pool, "deferred_pool");
+ else {
+ /* Save calls to ap_save_brigade() by batching successive
+ * non-morping buckets into a temporary brigade.
+ */
+ if (!tmp_bb) {
+ tmp_bb = ap_acquire_brigade(f->c);
+ }
+ APR_BRIGADE_INSERT_TAIL(tmp_bb, e);
}
- return ap_save_brigade(f, &fp->bb, &bb, fp->deferred_pool);
+ } while (!APR_BRIGADE_EMPTY(bb));
+
+ if (tmp_bb) {
+ /* Save any remainder. */
+ if (!APR_BRIGADE_EMPTY(tmp_bb)) {
+ rv = save_aside_brigade(fp, tmp_bb);
+ APR_BRIGADE_PREPEND(bb, tmp_bb);
In which case is this needed? tmp_bb should be always empty unless
ap_save_brigade fails.
+ }
+ ap_release_brigade(f->c, tmp_bb);
}
-
}
else if (fp->deferred_pool) {
/*
I think independent of the patch the API is not safe for use by request filters
because once we return from the handler we insert
the EOR bucket at the level of connection filters (c->output_filters) or before
the patch at the level of the request_core_filter:
Index: modules/http/http_request.c
===================================================================
--- modules/http/http_request.c (revision 1875498)
+++ modules/http/http_request.c (working copy)
@@ -350,7 +350,6 @@ AP_DECLARE(void) ap_process_request_after_handler(
apr_bucket_brigade *bb;
apr_bucket *b;
conn_rec *c = r->connection;
- ap_filter_t *f;
bb = ap_acquire_brigade(c);
@@ -371,15 +370,9 @@ AP_DECLARE(void) ap_process_request_after_handler(
/* All the request filters should have bailed out on EOS, and in any
* case they shouldn't have to handle this EOR which will destroy the
- * request underneath them. So go straight to the core request filter
- * which (if any) will take care of the setaside buckets.
+ * request underneath them. So go straight to the connection filters.
*/
- for (f = r->output_filters; f; f = f->next) {
- if (f->frec == ap_request_core_filter_handle) {
- break;
- }
- }
- ap_pass_brigade(f ? f : c->output_filters, bb);
+ ap_pass_brigade(c->output_filters, bb);
/* The EOR bucket has either been handled by an output filter (eg.
* deleted or moved to a buffered_bb => no more in bb), or an error
But if request filters still have setaside brigades and these would be
reactivated by ap_run_output_pending(c); in the
WRITE_COMPLETION phase they would write their response content after the EOR
bucket to the filter chain, which actually mean that
the EOR bucket is in the middle of the response data.
We would need to ensure that EOR is sent after EOS. But I guess we cannot sent
EOR through the remaining request filters as most
of them ignore and swallow what they see after EOS.
Regards
Rüdiger
