You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2007/11/06 22:28:01 UTC
Re: svn commit: r592446 - in /httpd/httpd/trunk: CHANGES
modules/ssl/mod_ssl.c modules/ssl/ssl_engine_io.c
modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_private.h
On Tue, Nov 06, 2007 at 09:45:42PM +0100, Ruediger Pluem wrote:
> On 11/06/2007 04:02 PM, jorton@apache.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->{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;
Re: svn commit: r592446 - in /httpd/httpd/trunk: CHANGES modules/ssl/mod_ssl.c
modules/ssl/ssl_engine_io.c modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_private.h
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/06/2007 11:06 PM, Ruediger Pluem wrote:
>
> 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, jorton@apache.org wrote:
> >> 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.
Any reason why this did not get committed so far?
Regards
Rüdiger
Re: svn commit: r592446 - in /httpd/httpd/trunk: CHANGES modules/ssl/mod_ssl.c
modules/ssl/ssl_engine_io.c modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_private.h
Posted by Ruediger Pluem <rp...@apache.org>.
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, jorton@apache.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->{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:
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