Re: svn commit: r592446 - in /httpd/httpd/trunk: CHANGES modules/ssl/mod_ssl.c modules/ssl/ssl_engin

On 11/06/2007 10:28 PM, Joe Orton wrote:
> On Tue, Nov 06, 2007 at 09:45:42PM +0100, Ruediger
Pluem wrote:
>> On 11/06/2007 04:02 PM, jortonapache.org 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->*_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:

Ahh, now I get it. Thanks for explaining. This was somewhat
hard to
understand without pointers.

 >> 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;

I agree that this should do the trick.

Regards

Rüdiger