On 01/05/2011 01:23 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Wed Jan 5 00:23:43 2011
> New Revision: 1055250
>
> URL: http://svn.apache.org/viewvc?rev=1055250&view=rev
> Log:
> mod_proxy_http: Allocate the fake backend request from a child pool
> of the backend connection, instead of misusing the pool of the frontend
> request. Fixes a thread safety issue where buckets set aside in the
> backend connection leak into other threads, and then disappear when
> the frontend request is cleaned up, in turn causing corrupted buckets
> to make other threads spin.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1055250&r1=1055249&r2=1055250&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Wed Jan 5 00:23:43 2011
> @@ -1448,21 +1447,21 @@ apr_status_t ap_proxy_http_process_respo
> * filter chain
> */
>
> - rp = ap_proxy_make_fake_req(origin, r);
> + backend->r = ap_proxy_make_fake_req(origin, r);
What about the comment in mod_proxy.h about the r element of proxy_conn_rec?
/* Request record of the frontend request
* which the backend currently answers. */
Doesn't this comment need to be adjusted now?
> /* In case anyone needs to know, this is a fake request that is really a
> * response.
> */
> - rp->proxyreq = PROXYREQ_RESPONSE;
> + backend->r->proxyreq = PROXYREQ_RESPONSE;
> tmp_bb = apr_brigade_create(p, c->bucket_alloc);
> do {
> apr_status_t rc;
>
> apr_brigade_cleanup(bb);
>
> - rc = ap_proxygetline(tmp_bb, buffer, sizeof(buffer), rp, 0, &len);
> + rc = ap_proxygetline(tmp_bb, buffer, sizeof(buffer), backend->r, 0, &len);
> if (len == 0) {
> /* handle one potential stray CRLF */
> - rc = ap_proxygetline(tmp_bb, buffer, sizeof(buffer), rp, 0, &len);
> + rc = ap_proxygetline(tmp_bb, buffer, sizeof(buffer), backend->r, 0, &len);
> }
> if (len <= 0) {
> ap_log_rerror(APLOG_MARK, APLOG_ERR, rc, r,
> @@ -1814,14 +1813,14 @@ apr_status_t ap_proxy_http_process_respo
> * TE, so that they are preserved accordingly for
> * ap_http_filter to know where to end.
> */
> - rp->headers_in = apr_table_copy(r->pool, r->headers_out);
> + backend->r->headers_in = apr_table_copy(backend->r->pool, r->headers_out);
Doesn't that mean that we will get entries in backend->r->headers that have been allocated
from r->pool instead of backend->r->pool?
> /*
> * Restore Transfer-Encoding header from response if we saved
> * one before and there is none left. We need it for the
> * ap_http_filter. See above.
> */
> - if (te && !apr_table_get(rp->headers_in, "Transfer-Encoding"))
{
> - apr_table_add(rp->headers_in, "Transfer-Encoding", te);
> + if (te && !apr_table_get(backend->r->headers_in, "Transfer-Encoding"))
{
> + apr_table_add(backend->r->headers_in, "Transfer-Encoding", te);
Doesn't that mean that we will get entries in backend->r->headers that have been allocated
from r->pool instead of backend->r->pool? te is an element from r->headers_out.
> }
>
> apr_table_unset(r->headers_out,"Transfer-Encoding");
>
Regards
Rüdiger
|