On Tue, Nov 06, 2007 at 09:45:42PM +0100, Ruediger Pluem wrote: > On 11/06/2007 04:02 PM, [EMAIL PROTECTED] wrote: > > Author: jorton > > Date: Tue Nov 6 07:02:32 2007 > > New Revision: 592446 > > > > URL: http://svn.apache.org/viewvc?rev=592446&view=rev > > Log: ... > > * modules/ssl/ssl_engine_io.c (ssl_io_filter_Upgrade): Remove > > function. > > (ssl_io_input_add_filter, ssl_io_filter_init): Take a request_rec > > pointer and pass to ap_add_*_filter to ensure the filter chain > > is modified correctly; remove it from the filter afterwards. > > Can you explain this in more detail please? I currently don't understand > what is going wrong if you call ap_add_input_filter / ap_add_output_filter > with NULL instead of r in the case of an upgrade (where r != NULL). Is it > because INSERT_BEFORE delivers the wrong value because f->r == NULL for all > connection level filters?
Thanks for the review. Sorry, I should really have put a comment about that in there somewhere. The issue is a long-standing "feature" of filters: if you add (or remove) a connection-level filter during a request, without reference to that request_rec, the r->{proto_}*_filters go stale: e.g. simplified: r->output_filters = r->proto_output_filters = HTTP_IN -> CORE_OUT r->connection->output_filters = CORE_OUT then insert the SSL filter before CORE_OUT leaves you with: r->output_filters = r->proto_output_filters = HTTP_IN -> CORE_OUT r->connection->output_filters = SSL -> CORE_OUT ...which is of course broken. Some past discussion of the issue: http://marc.info/?l=apache-httpd-dev&m=105366252619530&w=2 http://marc.info/?l=apache-httpd-dev&m=110243972504409&w=2 > Currently I see the danger that the connection level filter ssl_io_filter > is allocated out of the request pool by add_any_filter_handle (because r != > NULL) > instead of the connection pool. I think that even in the upgrade case the > lifetime of > ssl_io_filter is the same as the (remaining) lifetime of the connection and > that > this lifetime might be longer than that of r->pool. Great catch, I missed that this actually changes the lifetime of the filter, and regret not testing this in my apr-pool-debug build! I would like to say that is an add_any_filter_handle bug, if only because it makes the mod_ssl issue tractable without major surgery on the core filtering design :) If a filter is being added with connection-lifetime it must be allocated out of the c->pool anyway, regardless of whether r is passed in, surely. So, completely untested: Index: server/util_filter.c =================================================================== --- server/util_filter.c (revision 592444) +++ server/util_filter.c (working copy) @@ -279,7 +279,7 @@ ap_filter_t **p_filters, ap_filter_t **c_filters) { - apr_pool_t* p = r ? r->pool : c->pool; + apr_pool_t *p = frec->ftype < AP_FTYPE_CONNECTION && r ? r->pool : c->pool; ap_filter_t *f = apr_palloc(p, sizeof(*f)); ap_filter_t **outf;