Re: AddOutputFilterByType in Apache 2.4 inserts filters as AP_FTYPE_RESOURCE
Hi Nick, if the patch looks good, as you wrote, what is needed to get it applied to trunk and backported to 2.4.x? Have you seen my follow-up questions in the other mail? Best regards, Micha Am 13.01.2016 22:44, schrieb Nick Kew: On Wed, 2016-01-13 at 17:59 +0100, Micha Lenk wrote: Hi, The directive AddOutputFilterByType can be used to insert filters to the output filter chain depending on the content type of the HTTP response. So far so good. PROBLEM DESCRIPTION This is probably worth a bugzilla entry. I think I can clarify a little of what's happened. AddOutputFilterByType was something of a hacked afterthought to filtering back in the days of httpd 2.0. On the one hand, it met a need. On the other hand, it worked only in a very limited range of situations where the content type was known at the time the filter was to be added. It had no capacity to respond to other aspects of the content, or indeed the request/response. And there were other issues. Then came mod_filter and a generalised framework. AddOutputFilterByType was now obsolete, but too widely-used to dispense with entirely. As a compromise it was re-implemented within mod_filter, where it could co-exist with other dynamic filter configuration. Your observation tells us the semantics aren't quite compatible. And your patch looks good - thanks.
Re: AddOutputFilterByType in Apache 2.4 inserts filters as AP_FTYPE_RESOURCE
Hi Nick, Am 13.01.2016 22:44, schrieb Nick Kew: PROBLEM DESCRIPTION This is probably worth a bugzilla entry. Done. https://bz.apache.org/bugzilla/show_bug.cgi?id=58856 Nick, would you mind to provide some insights on these comments from my initial mail: For setups with both, FilterDeclare and AddOutputFilterByType (as described above as fix), I observed some issues with properly merging the two filter harnesses. However, I have no clue what semantics the original author wanted to have in this situation. Assumed that my patch gets applied, the filter type should be correctly set in the filter harness. But what if a user wants to override it? I got a few questions in this context: 1.) Should "FilterDeclare" with filter-name "BYTYPE:DEFLATE" (i.e. colliding with the implicit filter-name created by AddOutputFilterByName) be supported at all? 1a.) If yes, the handling of AddOutputFilterByType needs to be fixed so that: - a globally configured FilterDeclare is also effective for an AddOutputFilterByType within a (location) sub-container. - a filter type set by "FilterDeclare BYTYPE: type>" does not get overwritten by a subsequent "AddOutputFilterByType ". 1b.) If no, the code should detect and reject such configurations. 2.) On a related note to 1., Should FilterDeclare allow a filter-name of existing filter providers at all? If yes, behavior would we expect? Nick, I would be really glad if you could share your thoughts. Best regards, Micha
Re: AddOutputFilterByType in Apache 2.4 inserts filters as AP_FTYPE_RESOURCE
On Wed, 2016-01-13 at 17:59 +0100, Micha Lenk wrote: > Hi, > > The directive AddOutputFilterByType can be used to insert filters to the > output filter chain depending on the content type of the HTTP response. > So far so good. > > PROBLEM DESCRIPTION This is probably worth a bugzilla entry. I think I can clarify a little of what's happened. AddOutputFilterByType was something of a hacked afterthought to filtering back in the days of httpd 2.0. On the one hand, it met a need. On the other hand, it worked only in a very limited range of situations where the content type was known at the time the filter was to be added. It had no capacity to respond to other aspects of the content, or indeed the request/response. And there were other issues. Then came mod_filter and a generalised framework. AddOutputFilterByType was now obsolete, but too widely-used to dispense with entirely. As a compromise it was re-implemented within mod_filter, where it could co-exist with other dynamic filter configuration. Your observation tells us the semantics aren't quite compatible. And your patch looks good - thanks. -- Nick Kew
AddOutputFilterByType in Apache 2.4 inserts filters as AP_FTYPE_RESOURCE
Hi, The directive AddOutputFilterByType can be used to insert filters to the output filter chain depending on the content type of the HTTP response. So far so good. PROBLEM DESCRIPTION I observed that the behavior of this directive changed in Apache 2.4 for filters. Starting with Apache 2.4 filters are inserted at an earlier place in the filter chain than they were inserted with Apache 2.2. For example, if you use the directive AddOutputFilterByType DEFLATE text/html The filter is inserted with AP_FTYPE_RESOURCE, even though it was registered in mod_deflate.c as AP_FTYPE_SET_CONTENT. The effect is that using "AddOutputFilterByType DEFLATE text/html" the resulting filter chain is ordered diferrently compared to using "SetOutputFilter DEFLATE". This can be fixed in configuration by adding the following directive right after AddOutputFilterByType: FilterDeclare BYTYPE:DEFLATE CONTENT_SET Unfortunately the order and placement of FilterDeclare seems to be relevant, i.e. this fix only works if FilterDeclare comes *after* AddOutputFilterByType within the same container. I wonder whether this is an intentional behavior change of AddOutputFilterByType or not. ANALYSIS Apparently the filter type (ap_filter_rec_t struct member ftype) of the filter provider isn't respected anymore. The intended filter type is provided when registering the output filter by calling ap_register_output_filter(). In branch 2.2.x this was sufficient, because the conditional filter (based on the MIME type) was added directly by name (i.e. by calling ap_add_output_filter() in ap_add_output_filters_by_type). In branches 2.4.x and trunk the implementation of AddOutputFilterByType apparently moved to mod_filter and a layer of indirection (the filter harness) is introduced. But the filter harness is apparently created unconditionally with AP_FTYPE_RESOURCE. FIX APPROACH When implicitly creating a filter harness by calling the function add_filter(), we have access to the provider's ap_filter_rec_t anyways. So I recommend to just copy it from the filter provider (e.g. DEFLATE) to the filter harness (e.g. BYTYPE:DEFLATE). I've tested this approach with the patch below for a setup without any FilterDeclare directive, and it fixed the regression; the DEFLATE filter was ordered correctly at AP_FTYPE_CONTENT_SET again. - 8>< -- diff --git a/modules/filters/mod_filter.c b/modules/filters/mod_filter.c index 7b69223..5b5ecf6 100644 --- a/modules/filters/mod_filter.c +++ b/modules/filters/mod_filter.c @@ -444,6 +444,12 @@ static const char *add_filter(cmd_parms *cmd, void *CFG, ap_expr_info_t *node; const char *err = NULL; +/* if provider has been registered, we can look it up */ +provider_frec = ap_get_output_filter_handle(pname); +if (!provider_frec) { +return apr_psprintf(cmd->pool, "Unknown filter provider %s", pname); +} + /* fname has been declared with DeclareFilter, so we can look it up */ frec = apr_hash_get(cfg->live_filters, fname, APR_HASH_KEY_STRING); @@ -454,17 +460,13 @@ static const char *add_filter(cmd_parms *cmd, void *CFG, return c; } frec = apr_hash_get(cfg->live_filters, fname, APR_HASH_KEY_STRING); +frec->ftype = provider_frec->ftype; } if (!frec) { return apr_psprintf(cmd->pool, "Undeclared smart filter %s", fname); } -/* if provider has been registered, we can look it up */ -provider_frec = ap_get_output_filter_handle(pname); -if (!provider_frec) { -return apr_psprintf(cmd->pool, "Unknown filter provider %s", pname); -} provider = apr_palloc(cmd->pool, sizeof(ap_filter_provider_t)); if (expr) { node = ap_expr_parse_cmd(cmd, expr, 0, &err, NULL); - 8>< -- For setups with both, FilterDeclare and AddOutputFilterByType (as described above as fix), I observed some issues with properly merging the two filter harnesses. However, I have no clue what semantics the original author wanted to have in this situation. I hope my explanations are clear enough for others to follows. If not, please don't hesitate to ask. Best regards, Micha