You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2006/05/25 07:56:24 UTC
PR 39643 revealed a problem between mod_cache and mod_filter
During my analysis of PR39643 (http://issues.apache.org/bugzilla/show_bug.cgi?id=39643)
I found out that mod_cache and mod_filter do not work together correctly. In fact mod_filter
crashes with a segfault if the content is delivered from the cache.
The segfault is caused by line 366 of mod_filter (filter_harness):
if (!ctx->func) {
filter_harness implements the output filter of mod_filter.
It expects that its context has been initialized and does not perform a check if this is true.
Why is the context not initialized, if we deliver content from the cache?
This is because the filters (and thus the contexts of at least some filters) get initialized
in ap_invoke_filter_init which is a static function in config.c. ap_invoke_filter_init gets
only called by ap_invoke_handler (also in config.c). But if we deliver content from the cache we
do this inside the quick handler hook, which is run *before* ap_invoke_handler. Although we call
ap_run_insert_filter in the mod_cache quick handler we do *not* initialize the filters there.
So basicly I see the following approach for a fix:
1. In mod_filter do a sanity check if the filter context has been initialized. If not remove ourselves
from the chain and simply pass the brigade. This could be done by the following simple patch:
--- mod_filter.c (Revision 408729)
+++ mod_filter.c (Arbeitskopie)
@@ -355,7 +355,7 @@
harness_ctx *ctx = f->ctx;
ap_filter_rec_t *filter = f->frec;
- if (f->r->status != 200) {
+ if ((!ctx) || (f->r->status != 200)) {
ap_remove_output_filter(f);
return ap_pass_brigade(f->next, bb);
}
Or does somebody sees a need for an error message here if the context is not initialized?
2. Convert ap_invoke_filter_init from a static function to a public function that is part of the API
and let mod_cache call it after ap_run_insert_filter.
Questions:
1. What kind of DECLARE macro should be used to convert it to a public function ?
2. To which header file should we add the prototype?
3. Any special steps needed to adjust exports.c or is this done automatically during
make / configure?
4. Does this require a minor bump (I assume yes)?
Comments / Thoughts / Answers ?
Regards
Rüdiger
Re: PR 39643 revealed a problem between mod_cache and mod_filter
Posted by Ruediger Pluem <rp...@apache.org>.
On 05/25/2006 07:56 AM, Ruediger Pluem wrote:
> During my analysis of PR39643 (http://issues.apache.org/bugzilla/show_bug.cgi?id=39643)
> I found out that mod_cache and mod_filter do not work together correctly. In fact mod_filter
> crashes with a segfault if the content is delivered from the cache.
>
> The segfault is caused by line 366 of mod_filter (filter_harness):
>
> if (!ctx->func) {
>
> filter_harness implements the output filter of mod_filter.
> It expects that its context has been initialized and does not perform a check if this is true.
>
> Why is the context not initialized, if we deliver content from the cache?
>
> This is because the filters (and thus the contexts of at least some filters) get initialized
> in ap_invoke_filter_init which is a static function in config.c. ap_invoke_filter_init gets
> only called by ap_invoke_handler (also in config.c). But if we deliver content from the cache we
> do this inside the quick handler hook, which is run *before* ap_invoke_handler. Although we call
> ap_run_insert_filter in the mod_cache quick handler we do *not* initialize the filters there.
>
> So basicly I see the following approach for a fix:
>
> 1. In mod_filter do a sanity check if the filter context has been initialized. If not remove ourselves
> from the chain and simply pass the brigade. This could be done by the following simple patch:
>
> --- mod_filter.c (Revision 408729)
> +++ mod_filter.c (Arbeitskopie)
> @@ -355,7 +355,7 @@
> harness_ctx *ctx = f->ctx;
> ap_filter_rec_t *filter = f->frec;
>
> - if (f->r->status != 200) {
> + if ((!ctx) || (f->r->status != 200)) {
> ap_remove_output_filter(f);
> return ap_pass_brigade(f->next, bb);
> }
>
> Or does somebody sees a need for an error message here if the context is not initialized?
>
> 2. Convert ap_invoke_filter_init from a static function to a public function that is part of the API
> and let mod_cache call it after ap_run_insert_filter.
> Questions:
>
> 1. What kind of DECLARE macro should be used to convert it to a public function ?
> 2. To which header file should we add the prototype?
> 3. Any special steps needed to adjust exports.c or is this done automatically during
> make / configure?
> 4. Does this require a minor bump (I assume yes)?
>
> Comments / Thoughts / Answers ?
Ping. Any further comments? Especially any answers to my questions?
With answers to my questions I would be able to prepare a patch for further discussion :-).
Regards
Rüdiger
Re: PR 39643 revealed a problem between mod_cache and mod_filter
Posted by Ruediger Pluem <rp...@apache.org>.
On 05/25/2006 09:02 AM, Nick Kew wrote:
> On Thursday 25 May 2006 06:56, Ruediger Pluem wrote:
>
>
> Which begs the question: why are we attaching output filters to mod_cache?
Because some module might offer a protocol or connection filter that should be
also run on cached content. e.g. core_insert_filter is triggered by ap_run_insert filter
to insert all filters set with SetOutputFilter. But looking at the code that
turns up the question for me if it is correct to start the filter chain with
r->output_filters in the case we are serving cached content. I guess we would need
to bypass all filters below the CACHE_OUT filter and start kicking off the filter chain
with this filter.
>
>
>>1. In mod_filter do a sanity check if the filter context has been
>>initialized. If not remove ourselves from the chain and simply pass the
>>brigade. This could be done by the following simple patch:
>
>
> If we have cached contents, it should presumably come from after
> any content-level filters, anyway. So a cautious +1, with the caveat
> that if anyone is using mod_filter to configure connection-level filters,
> it might not be the ideal fix.
Provided that we do 2. it is just a sanity check.
>
> Possibly a debug message. Sod's law says someone will attach a
> filter to mod_cache output then need to debug why it doesn't work.
Sounds reasonable, but I talked about a debug message inside mod_filter.
So this would not help for arbitrary filters.
>
>
>>2. Convert ap_invoke_filter_init from a static function to a public
>>function that is part of the API and let mod_cache call it after
>>ap_run_insert_filter.
>
>
> Which would imply filtering on cached docs. If we're going to
> do that, let's attach it to the normal handler, not quick_handler.
Not necessarily, because we could have protocol and network filters
that should also run on cached content (see above).
Re: PR 39643 revealed a problem between mod_cache and mod_filter
Posted by Nick Kew <ni...@webthing.com>.
On Thursday 25 May 2006 06:56, Ruediger Pluem wrote:
> This is because the filters (and thus the contexts of at least some
> filters) get initialized in ap_invoke_filter_init which is a static
> function in config.c. ap_invoke_filter_init gets only called by
> ap_invoke_handler (also in config.c)
I think you've just supported my view, that the filter_init step is
a Bad Thing and should be dropped. The offending code in
mod_filter is a grotty - and evidently incomplete -hack to work
around it.
> . But if we deliver content from the
> cache we do this inside the quick handler hook, which is run *before*
> ap_invoke_handler. Although we call ap_run_insert_filter in the mod_cache
> quick handler we do *not* initialize the filters there.
Which begs the question: why are we attaching output filters to mod_cache?
> 1. In mod_filter do a sanity check if the filter context has been
> initialized. If not remove ourselves from the chain and simply pass the
> brigade. This could be done by the following simple patch:
If we have cached contents, it should presumably come from after
any content-level filters, anyway. So a cautious +1, with the caveat
that if anyone is using mod_filter to configure connection-level filters,
it might not be the ideal fix.
> Or does somebody sees a need for an error message here if the context is
> not initialized?
Possibly a debug message. Sod's law says someone will attach a
filter to mod_cache output then need to debug why it doesn't work.
> 2. Convert ap_invoke_filter_init from a static function to a public
> function that is part of the API and let mod_cache call it after
> ap_run_insert_filter.
Which would imply filtering on cached docs. If we're going to
do that, let's attach it to the normal handler, not quick_handler.
--
Nick Kew