On Wed, Apr 26, 2017 at 11:26 AM, Stefan Eissing
<stefan.eiss...@greenbytes.de> wrote:
>
>> Am 26.04.2017 um 11:14 schrieb Plüm, Rüdiger, Vodafone Group
>> <ruediger.pl...@vodafone.com>:
>>
>>
>>
>>> -----Ursprüngliche Nachricht-----
>>> Von: Stefan Eissing [mailto:stefan.eiss...@greenbytes.de]
>>> Gesendet: Mittwoch, 26. April 2017 10:55
>>> An: dev@httpd.apache.org
>>> Betreff: Re: svn commit: r1707087 -
>>> /httpd/httpd/trunk/modules/debugging/mod_bucketeer.c
>>>
>>> Eh, not really into mod_bucketeer and its use cases, but some
>>> observations after a quick scan:
>>>
>>> line 99:
>>> rv = ap_pass_brigade(f->next, ctx->bb);
>>> apr_brigade_cleanup(ctx->bb);
>>>
>>> line 146:
>>> rv = ap_pass_brigade(f->next, ctx->bb);
>>> if (rv) {
>>> return rv;
>>> }
>>> apr_brigade_cleanup(ctx->bb);
>>>
>>> such things only work if an EOS always comes *before* an EOR
>>> bucket (case 1) or of no DATA bucket of any kind follows an EOR.
>>
>> Correct, but with the BUCKETEER filter being a resource filter that
>> should not happen.
>
> Because the implementations of everything else in the server do not
> do it? Should we then add a filter that checks exactly that?
I don't think that EOR before EOS can happen in HTTP/1, because
ap_finalize_request_protocol() is always called before
ap_process_request_after_handler().
EOS is the signal for request filters to get out of the way, so this
is probably what ap_request_core_filter() should do too, and the
purpose of the attached patch.
This patch could use EOR instead, but I find EOS more "semantically"
correct for a request filter (we don't need to set aside anything
after it), while EOR would be crash safe only...
Jacob, does it work better?
Regards,
Yann.
Index: server/request.c
===================================================================
--- server/request.c (revision 1791747)
+++ server/request.c (working copy)
@@ -2057,12 +2057,27 @@ AP_CORE_DECLARE_NONSTD(apr_status_t) ap_request_co
tmp_bb = f->ctx = apr_brigade_create(f->r->pool, f->c->bucket_alloc);
}
+ /* Use the given bb downstream (purposedly scoped), hence move the
+ * buckets into tmp_bb to walk through them locally.
+ */
+ 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);
+ int eos = APR_BUCKET_IS_EOS(bucket);
+ if (eos) {
+ /* pass everything down the chain, this request is over (and will
+ * possibly be destroyed before we leave, should the EOR be aside),
+ * not this filter's business anymore.
+ */
+ APR_BRIGADE_CONCAT(bb, tmp_bb);
+ ap_remove_output_filter(f);
+ }
+ else {
/* if the core has set aside data, back off and try later */
if (!flush_upto) {
if (ap_filter_should_yield(f)) {
@@ -2088,20 +2103,19 @@ 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);
- if (!APR_STATUS_IS_EOF(status) && (status != APR_SUCCESS)) {
+ status = ap_pass_brigade(f->next, bb);
+ if (status != APR_SUCCESS || eos) {
return status;
}
-
+ apr_brigade_cleanup(bb);
}
- ap_filter_setaside_brigade(f, bb);
- return status;
+ return ap_filter_setaside_brigade(f, tmp_bb);
}
extern APR_OPTIONAL_FN_TYPE(authz_some_auth_required) *ap__authz_ap_some_auth_required;
