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 2007/06/01 22:55:10 UTC
Re: svn commit: r543515 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_mem_cache.c
On 06/01/2007 05:50 PM, covener@apache.org wrote:
> Author: covener
> Date: Fri Jun 1 08:50:12 2007
> New Revision: 543515
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=543515
> Log:
> SECURITY: CVE-2007-1862 (cve.mitre.org)
> mod_mem_cache: Copy headers into longer lived storage; header names and
> values could previously point to cleaned up storage
>
> PR: 41551
> Submitted by: Davi Arnaut <davi haxent.com.br>
> Reviewed by: covener
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/cache/mod_mem_cache.c
>
>
> Modified: httpd/httpd/trunk/modules/cache/mod_mem_cache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/cache/mod_mem_cache.c?view=diff&rev=543515&r1=543514&r2=543515
> ==============================================================================
> --- httpd/httpd/trunk/modules/cache/mod_mem_cache.c (original)
> +++ httpd/httpd/trunk/modules/cache/mod_mem_cache.c Fri Jun 1 08:50:12 2007
> @@ -539,12 +539,28 @@
> return OK;
> }
>
> +static apr_table_t *deep_table_copy(apr_pool_t *p, const apr_table_t *table)
> +{
> + const apr_array_header_t *array = apr_table_elts(table);
> + apr_table_entry_t *elts = (apr_table_entry_t *) array->elts;
> + apr_table_t *copy = apr_table_make(p, array->nelts);
> + int i;
> +
> + for (i = 0; i < array->nelts; i++) {
> + if (elts[i].key) {
> + apr_table_add(copy, elts[i].key, elts[i].val);
> + }
> + }
> +
> + return copy;
> +}
> +
> static apr_status_t recall_headers(cache_handle_t *h, request_rec *r)
> {
> mem_cache_object_t *mobj = (mem_cache_object_t*) h->cache_obj->vobj;
>
> - h->req_hdrs = apr_table_copy(r->pool, mobj->req_hdrs);
> - h->resp_hdrs = apr_table_copy(r->pool, mobj->header_out);
> + h->req_hdrs = deep_table_copy(r->pool, mobj->req_hdrs);
> + h->resp_hdrs = deep_table_copy(r->pool, mobj->header_out);
Just curious: Why is this needed? The live time of the mobj->pool pool should be longer
then that of the r->pool pool (which should be ensured by the refcount stuff, which would
be broken otherwise).
Of course not doing a deep_table_copy can cause trouble afterwards with APR_POOL_DEBUG
set as mobj->pool does not seem to be an ancestor of r->pool. Is this the reason
for this copy operation?
Regards
Rüdiger
Re: svn commit: r543515 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_mem_cache.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
> On 06/01/2007 11:18 PM, Eric Covener wrote:
>> On 6/1/07, Ruediger Pluem <rp...@apache.org> wrote:
>>
>>> Ahh. Should have read
>>> http://issues.apache.org/bugzilla/show_bug.cgi?id=41551#c2
>>> before which answers my question :-).
>>> Anyway another question: From a first glance your original patch and
>>> this patch basicly seem to do the same thing.
>>> But the original patch was said to be non working by the reporter whereas
>>> this worked. Any idea why?
>>
>> A mystery to me as well, the one-sided endorsement from the reporter
>
> Good. I feared I just missed something obvious :-).
>
>> as well as Davi having contributed the original heap-to-pools patch
>> was enough to sway me to use his patch instead.
>>
>
> The advantage of his patch is that once his proposed apr_table_clone
> is available in apr we can use this and things where we call it remain
> more readable IMHO than with the apr_table_do approach (not saying that
> your patch was unreadable).
Just to be clear, apr_table_clone can't arrive until apr 1.3.0, which would
be adopted in httpd 2.4 at the earliest.
Clearer, more legible to be sure, but httpd-2.2 (2.0?) needs the short-lived
stop gap for now.
Bill
Re: svn commit: r543515 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_mem_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 06/01/2007 11:18 PM, Eric Covener wrote:
> On 6/1/07, Ruediger Pluem <rp...@apache.org> wrote:
>
>> Ahh. Should have read
>> http://issues.apache.org/bugzilla/show_bug.cgi?id=41551#c2
>> before which answers my question :-).
>> Anyway another question: From a first glance your original patch and
>> this patch basicly seem to do the same thing.
>> But the original patch was said to be non working by the reporter whereas
>> this worked. Any idea why?
>
>
> A mystery to me as well, the one-sided endorsement from the reporter
Good. I feared I just missed something obvious :-).
> as well as Davi having contributed the original heap-to-pools patch
> was enough to sway me to use his patch instead.
>
The advantage of his patch is that once his proposed apr_table_clone
is available in apr we can use this and things where we call it remain
more readable IMHO than with the apr_table_do approach (not saying that
your patch was unreadable).
Regards
Rüdiger
Re: svn commit: r543515 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_mem_cache.c
Posted by Eric Covener <co...@gmail.com>.
On 6/1/07, Ruediger Pluem <rp...@apache.org> wrote:
> Ahh. Should have read http://issues.apache.org/bugzilla/show_bug.cgi?id=41551#c2
> before which answers my question :-).
> Anyway another question: From a first glance your original patch and
> this patch basicly seem to do the same thing.
> But the original patch was said to be non working by the reporter whereas
> this worked. Any idea why?
A mystery to me as well, the one-sided endorsement from the reporter
as well as Davi having contributed the original heap-to-pools patch
was enough to sway me to use his patch instead.
--
Eric Covener
covener@gmail.com
Re: svn commit: r543515 - in /httpd/httpd/trunk: CHANGES modules/cache/mod_mem_cache.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 06/01/2007 10:55 PM, Ruediger Pluem wrote:
>
> On 06/01/2007 05:50 PM, covener@apache.org wrote:
>
>>+
>> static apr_status_t recall_headers(cache_handle_t *h, request_rec *r)
>> {
>> mem_cache_object_t *mobj = (mem_cache_object_t*) h->cache_obj->vobj;
>>
>>- h->req_hdrs = apr_table_copy(r->pool, mobj->req_hdrs);
>>- h->resp_hdrs = apr_table_copy(r->pool, mobj->header_out);
>>+ h->req_hdrs = deep_table_copy(r->pool, mobj->req_hdrs);
>>+ h->resp_hdrs = deep_table_copy(r->pool, mobj->header_out);
>
>
> Just curious: Why is this needed? The live time of the mobj->pool pool should be longer
> then that of the r->pool pool (which should be ensured by the refcount stuff, which would
> be broken otherwise).
> Of course not doing a deep_table_copy can cause trouble afterwards with APR_POOL_DEBUG
> set as mobj->pool does not seem to be an ancestor of r->pool. Is this the reason
> for this copy operation?
Ahh. Should have read http://issues.apache.org/bugzilla/show_bug.cgi?id=41551#c2 before which
answers my question :-).
Anyway another question: From a first glance your original patch and this patch basicly seem
to do the same thing. But the original patch was said to be non working by the reporter whereas
this worked. Any idea why?
Regards
Rüdiger