Re: svn commit: r1055250 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
On 01/05/2011 01:23 AM, minf...@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
Re: svn commit: r1055250 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
On 05 Jan 2011, at 9:43 AM, Ruediger Pluem wrote:
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?
It does - I've fixed it in r1055367.
-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?
Hmmm... it does yes, I think we need an apr_table_clone here.
/*
* 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.
In this case we shouldn't, as apr_table_add() makes a copy of te from
backend->r->pool. If we'd used apr_table_addn(), then it would have
been yes.
Regards,
Graham
--
RE: svn commit: r1055250 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
> -Original Message-
> From: Graham Leggett
> Sent: Mittwoch, 5. Januar 2011 10:30
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1055250 - in /httpd/httpd/trunk:
> CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
>
> On 05 Jan 2011, at 9:43 AM, Ruediger Pluem wrote:
> >> /*
> >> * 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.
>
> In this case we shouldn't, as apr_table_add() makes a copy of
> te from
> backend->r->pool. If we'd used apr_table_addn(), then it would have
> been yes.
Good catch. I missed that.
Regards
Rüdiger
Re: svn commit: r1055250 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Hello,
minf...@apache.org wrote:
Author: minfrin
Date: Wed Jan 5 00:23:43 2011
New Revision: 1055250
Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1055250&r1=1055249&r2=1055250&view=diff
==
--- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Jan 5 00:23:43 2011
@@ -348,16 +348,20 @@ PROXY_DECLARE(const char *)
PROXY_DECLARE(request_rec *)ap_proxy_make_fake_req(conn_rec *c, request_rec *r)
{
-request_rec *rp = apr_pcalloc(r->pool, sizeof(*r));
+apr_pool_t *pool;
+
+apr_pool_create(&pool, c->pool);
+
+request_rec *rp = apr_pcalloc(pool, sizeof(*r));
MSVC idiosyncrasy, you cannot declare rp after doing something, in this
case "something" is calling apr_pool_create.
.\proxy_util.c(355) : error C2275: 'request_rec' : illegal use of this
type as an expression
c:\build\nresvn\include\httpd.h(730) : see declaration of
'request_rec'
.\proxy_util.c(355) : error C2065: 'rp' : undeclared identifier
to the tune of 36 total errors.
It looks like this apr_pool_create and apr_pcalloc lines were
specifically placed in this order so declare rp after pool.
apr_pool_t *pool;
request_rec *rp;
apr_pool_create(&pool, c->pool);
rp = apr_pcalloc(pool, sizeof(*r));
Thanks,
Gregg
RE: svn commit: r1055250 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
> -Original Message-
> From: Gregg L. Smith
> Sent: Donnerstag, 6. Januar 2011 02:56
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1055250 - in /httpd/httpd/trunk:
> CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
>
> Hello,
>
> wrote:
> > Author: minfrin
> > Date: Wed Jan 5 00:23:43 2011
> > New Revision: 1055250
>
> > Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/p
> roxy_util.c?rev=1055250&r1=1055249&r2=1055250&view=diff
> >
> ==
>
> > --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Wed Jan 5
> 00:23:43 2011
> > @@ -348,16 +348,20 @@ PROXY_DECLARE(const char *)
> >
> > PROXY_DECLARE(request_rec
> *)ap_proxy_make_fake_req(conn_rec *c, request_rec *r)
> > {
> > -request_rec *rp = apr_pcalloc(r->pool, sizeof(*r));
> > +apr_pool_t *pool;
> > +
> > +apr_pool_create(&pool, c->pool);
> > +
> > +request_rec *rp = apr_pcalloc(pool, sizeof(*r));
> >
>
> MSVC idiosyncrasy, you cannot declare rp after doing
> something, in this
> case "something" is calling apr_pool_create.
Fixed in r1055771.
Regards
Rüdiger
Re: svn commit: r1055250 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
On Thursday 06 January 2011, Plüm, Rüdiger, VF-Group wrote:
> > > {
> > >
> > > -request_rec *rp = apr_pcalloc(r->pool, sizeof(*r));
> > > +apr_pool_t *pool;
> > > +
> > > +apr_pool_create(&pool, c->pool);
> > > +
> > > +request_rec *rp = apr_pcalloc(pool, sizeof(*r));
> > >
> > >
> >
> >
> >
> > MSVC idiosyncrasy, you cannot declare rp after doing
> > something, in this
> > case "something" is calling apr_pool_create.
>
> Fixed in r1055771.
BTW, gcc has -Wdeclaration-after-statement to catch these things.
Should we add that to the default warnings enabled in maintainer mode?
Re: svn commit: r1055250 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Am 10.01.2011 21:26, schrieb Stefan Fritsch: BTW, gcc has -Wdeclaration-after-statement to catch these things. Should we add that to the default warnings enabled in maintainer mode? +2 - NetWare compiler cant deal with either; and +1 to treat it even as error rather than warning: -Werror=declarationafterstatement BTW. isnt the option without dashes? My 'man gcc' lists: Wdeclarationafterstatement (C and ObjectiveC only) Warn when a declaration is found after a statement in a block. This construct, known from C++, was introduced with ISO C99 and is by default allowed in GCC. It is not supported by ISO C90 and was not supported by GCC versions before GCC 3.0. Since GCC < 3 didnt support this I assume that then also the above options are not recognized - so adding them should depend on GCC version >= 3. Gün.
