You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ji...@apache.org on 2014/01/23 15:09:41 UTC
svn commit: r1560689 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Author: jim
Date: Thu Jan 23 14:09:40 2014
New Revision: 1560689
URL: http://svn.apache.org/r1560689
Log:
Tuck away UDS path in request-rec, since worker isn't
thread-safe. Protect from NULL refs.
Modified:
httpd/httpd/trunk/modules/proxy/proxy_util.c
Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1560689&r1=1560688&r2=1560689&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Jan 23 14:09:40 2014
@@ -1958,11 +1958,8 @@ PROXY_DECLARE(int) ap_proxy_pre_request(
*ptr = '\0';
rv = apr_uri_parse(r->pool, ptr2, &urisock);
if (rv == APR_SUCCESS) {
- char *sockpath = ap_runtime_dir_relative(r->pool, urisock.path);;
- if (PROXY_STRNCPY((*worker)->s->uds_path, sockpath) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02597)
- "worker uds path (%s) too long", sockpath);
- }
+ char *sockpath = ap_runtime_dir_relative(r->pool, urisock.path);
+ apr_table_setn(r->notes, "uds_path", sockpath);
r->filename = ptr+1; /* so we get the scheme for the uds */
*url = apr_pstrdup(r->pool, r->filename);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
@@ -2130,28 +2127,7 @@ PROXY_DECLARE(int) ap_proxy_acquire_conn
(*conn)->worker = worker;
(*conn)->close = 0;
(*conn)->inreslist = 0;
-
- if (*worker->s->uds_path) {
- if ((*conn)->uds_path == NULL) {
- /* use (*conn)->pool instead of worker->cp->pool to match lifetime */
- (*conn)->uds_path = apr_pstrdup((*conn)->pool, worker->s->uds_path);
- }
- if ((*conn)->uds_path) {
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02545)
- "%s: has determined UDS as %s",
- proxy_function, (*conn)->uds_path);
- }
- else {
- /* should never happen */
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02546)
- "%s: cannot determine UDS (%s)",
- proxy_function, worker->s->uds_path);
-
- }
- }
- else {
- (*conn)->uds_path = NULL;
- }
+ (*conn)->uds_path = NULL;
return OK;
}
@@ -2183,6 +2159,7 @@ ap_proxy_determine_connection(apr_pool_t
int server_port;
apr_status_t err = APR_SUCCESS;
apr_status_t uerr = APR_SUCCESS;
+ const char *uds_path;
/*
* Break up the URL to determine the host to connect to
@@ -2223,7 +2200,26 @@ ap_proxy_determine_connection(apr_pool_t
* to check host and port on the conn and be careful about
* spilling the cached addr from the worker.
*/
- if (!(*worker->s->uds_path) &&
+ uds_path = (*worker->s->uds_path ? worker->s->uds_path : apr_table_get(r->notes, "uds_path"));
+ if (uds_path) {
+ if (conn->uds_path == NULL) {
+ /* use (*conn)->pool instead of worker->cp->pool to match lifetime */
+ conn->uds_path = apr_pstrdup(conn->pool, uds_path);
+ }
+ if (conn->uds_path) {
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02545)
+ "%s: has determined UDS as %s",
+ uri->scheme, conn->uds_path);
+ }
+ else {
+ /* should never happen */
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02546)
+ "%s: cannot determine UDS (%s)",
+ uri->scheme, uds_path);
+
+ }
+ }
+ if (!(uds_path) &&
(!conn->hostname || !worker->s->is_address_reusable ||
worker->s->disablereuse)) {
if (proxyname) {
@@ -2275,7 +2271,17 @@ ap_proxy_determine_connection(apr_pool_t
conn->pool);
}
}
- if (!(*worker->s->uds_path) && worker->s->is_address_reusable && !worker->s->disablereuse) {
+ else {
+ /*
+ * In UDS cases, some structs are NULL. Protect from de-refs
+ * and provide info for logging at the same time.
+ */
+ apr_sockaddr_t *sa;
+ apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
+ conn->hostname = "httpd-UDS";
+ conn->addr = sa;
+ }
+ if (!(uds_path) && worker->s->is_address_reusable && !worker->s->disablereuse) {
/*
* Looking up the backend address for the worker only makes sense if
* we can reuse the address.
Re: svn commit: r1560689 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Jan 23, 2014 at 3:09 PM, <ji...@apache.org> wrote:
> Author: jim
> Date: Thu Jan 23 14:09:40 2014
> New Revision: 1560689
>
> URL: http://svn.apache.org/r1560689
> Log:
> Tuck away UDS path in request-rec, since worker isn't
> thread-safe. Protect from NULL refs.
>
> Modified:
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1560689&r1=1560688&r2=1560689&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Jan 23 14:09:40 2014
> [...]
> @@ -2130,28 +2127,7 @@ PROXY_DECLARE(int) ap_proxy_acquire_conn
> (*conn)->worker = worker;
> (*conn)->close = 0;
> (*conn)->inreslist = 0;
> -
> - if (*worker->s->uds_path) {
> - if ((*conn)->uds_path == NULL) {
> - /* use (*conn)->pool instead of worker->cp->pool to match
> lifetime */
> - (*conn)->uds_path = apr_pstrdup((*conn)->pool,
> worker->s->uds_path);
> - }
> - if ((*conn)->uds_path) {
> - ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02545)
> - "%s: has determined UDS as %s",
> - proxy_function, (*conn)->uds_path);
> - }
> - else {
> - /* should never happen */
> - ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02546)
> - "%s: cannot determine UDS (%s)",
> - proxy_function, worker->s->uds_path);
> -
> - }
> - }
> - else {
> - (*conn)->uds_path = NULL;
> - }
> + (*conn)->uds_path = NULL;
>
> return OK;
> }
>
When the worker's connections are reused (eg. not reverse, those with
conn->pool is not cleared on recycle), NULLing conn->uds_path here will
cause a leak below...
> @@ -2183,6 +2159,7 @@ ap_proxy_determine_connection(apr_pool_t
> int server_port;
> apr_status_t err = APR_SUCCESS;
> apr_status_t uerr = APR_SUCCESS;
> + const char *uds_path;
>
> /*
> * Break up the URL to determine the host to connect to
> @@ -2223,7 +2200,26 @@ ap_proxy_determine_connection(apr_pool_t
> * to check host and port on the conn and be careful about
> * spilling the cached addr from the worker.
> */
> - if (!(*worker->s->uds_path) &&
> + uds_path = (*worker->s->uds_path ? worker->s->uds_path :
> apr_table_get(r->notes, "uds_path"));
> + if (uds_path) {
> + if (conn->uds_path == NULL) {
> + /* use (*conn)->pool instead of worker->cp->pool to match
> lifetime */
> + conn->uds_path = apr_pstrdup(conn->pool, uds_path);
> + }
>
... here: conn->uds_path is therefore allocated for each request.
(My proposed patch about r1560367 was no better, so maybe I put you on the
wrong track).
> + if (conn->uds_path) {
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02545)
> + "%s: has determined UDS as %s",
> + uri->scheme, conn->uds_path);
> + }
> + else {
> + /* should never happen */
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02546)
> + "%s: cannot determine UDS (%s)",
> + uri->scheme, uds_path);
> +
> + }
> + }
>
As the comment says, this check looks overkill (NULL is rarely/never
checked against allocations in httpd).
Moreover if that ever happens, the code below should check for
conn->uds_path instead of (the local) uds_path (which won't be NULL).
> + if (!(uds_path) &&
> (!conn->hostname || !worker->s->is_address_reusable ||
> worker->s->disablereuse)) {
> if (proxyname) {
> @@ -2275,7 +2271,17 @@ ap_proxy_determine_connection(apr_pool_t
> conn->pool);
> }
> }
> - if (!(*worker->s->uds_path) && worker->s->is_address_reusable &&
> !worker->s->disablereuse) {
> + else {
> + /*
> + * In UDS cases, some structs are NULL. Protect from de-refs
> + * and provide info for logging at the same time.
> + */
> + apr_sockaddr_t *sa;
> + apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
> + conn->hostname = "httpd-UDS";
> + conn->addr = sa;
> + }
> + if (!(uds_path) && worker->s->is_address_reusable &&
> !worker->s->disablereuse) {
> /*
> * Looking up the backend address for the worker only makes sense
> if
> * we can reuse the address.
>
Maybe the patch below is appropriate to address the comments above.
Since the conn->uds_path may now be reused (not NULL), it could also
theoritically be different from the one given by the note, so I added a
XXX-comment about the case (which is handled by the patch, though it
shouldn't happen for now).
Regards,
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1560806)
+++ modules/proxy/proxy_util.c (working copy)
@@ -1934,7 +1934,6 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_work
"*: found reverse proxy worker for %s",
*url);
*balancer = NULL;
*worker = conf->reverse;
- *(*worker)->s->uds_path = '\0';
access_status = OK;
/*
* The reverse worker does not keep connections alive, so
@@ -2127,7 +2126,6 @@ PROXY_DECLARE(int) ap_proxy_acquire_connection(con
(*conn)->worker = worker;
(*conn)->close = 0;
(*conn)->inreslist = 0;
- (*conn)->uds_path = NULL;
return OK;
}
@@ -2206,18 +2204,30 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
/* use (*conn)->pool instead of worker->cp->pool to match
lifetime */
conn->uds_path = apr_pstrdup(conn->pool, uds_path);
}
- if (conn->uds_path) {
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02545)
- "%s: has determined UDS as %s",
- uri->scheme, conn->uds_path);
+ /* XXX: This "else" is not really needed for now, since uds_path is
+ * either taken from a defined worker (invariant), or from a
+ * mod_rewrite's rule hitting a generic worker (reverse) whose
+ * connections are currently not reusable. Try to be safe from
+ * future changes at low cost (by checking first that the path
+ * is not invariant).
+ */
+ else if (uds_path != worker->s->uds_path &&
+ strcmp(conn->uds_path, uds_path) != 0) {
+ apr_size_t len = strlen(uds_path);
+ if (strlen(conn->uds_path) < len) {
+ conn->uds_path = apr_pstrmemdup(conn->pool, uds_path, len);
+ }
+ else {
+ /* Avoid a leak by reusing the previous allocation when
+ * possible. Since noone should use the old value now,
+ * the const_cast<> is safe here. +1 is for the '\0'.
+ */
+ memcpy((char*)conn->uds_path, uds_path, len + 1);
+ }
}
- else {
- /* should never happen */
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02546)
- "%s: cannot determine UDS (%s)",
- uri->scheme, uds_path);
-
- }
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02545)
+ "%s: has determined UDS as %s",
+ uri->scheme, conn->uds_path);
}
if (!(uds_path) &&
(!conn->hostname || !worker->s->is_address_reusable ||
[EOS]
Re: svn commit: r1560689 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
The patch proposed here fixes this:
http://mail-archives.apache.org/mod_mbox/httpd-dev/201401.mbox/%3CCAKQ1sVOZ4M9626H-1eAuOPwgQYURntd7k2Oks%2B5HTchg9Y%2BEow%40mail.gmail.com%3E
Regards,
Yann.
On Fri, Jan 24, 2014 at 8:50 AM, Ruediger Pluem <rp...@apache.org> wrote:
>
>
> jim@apache.org wrote:
> > Author: jim
> > Date: Thu Jan 23 14:09:40 2014
> > New Revision: 1560689
> >
> > URL: http://svn.apache.org/r1560689
> > Log:
> > Tuck away UDS path in request-rec, since worker isn't
> > thread-safe. Protect from NULL refs.
> >
> > Modified:
> > httpd/httpd/trunk/modules/proxy/proxy_util.c
> >
> > Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1560689&r1=1560688&r2=1560689&view=diff
> >
> ==============================================================================
> > --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> > +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Jan 23 14:09:40 2014
>
> > @@ -2275,7 +2271,17 @@ ap_proxy_determine_connection(apr_pool_t
> > conn->pool);
> > }
> > }
> > - if (!(*worker->s->uds_path) && worker->s->is_address_reusable &&
> !worker->s->disablereuse) {
> > + else {
> > + /*
> > + * In UDS cases, some structs are NULL. Protect from de-refs
> > + * and provide info for logging at the same time.
> > + */
> > + apr_sockaddr_t *sa;
> > + apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
> > + conn->hostname = "httpd-UDS";
> > + conn->addr = sa;
> > + }
>
> You cannot be sure in this else branch that you are in the UDS case. It
> could that there is an existing hostname or that
> the address is reusable. You should do the stuff above at the end of the
> if block starting in 2204 (if (uds_path) {
> )
>
> > + if (!(uds_path) && worker->s->is_address_reusable &&
> !worker->s->disablereuse) {
> > /*
> > * Looking up the backend address for the worker only makes
> sense if
> > * we can reuse the address.
> >
>
> Regards
>
> Rüdiger
>
Re: svn commit: r1560689 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
jim@apache.org wrote:
> Author: jim
> Date: Thu Jan 23 14:09:40 2014
> New Revision: 1560689
>
> URL: http://svn.apache.org/r1560689
> Log:
> Tuck away UDS path in request-rec, since worker isn't
> thread-safe. Protect from NULL refs.
>
> Modified:
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1560689&r1=1560688&r2=1560689&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Thu Jan 23 14:09:40 2014
> @@ -2275,7 +2271,17 @@ ap_proxy_determine_connection(apr_pool_t
> conn->pool);
> }
> }
> - if (!(*worker->s->uds_path) && worker->s->is_address_reusable && !worker->s->disablereuse) {
> + else {
> + /*
> + * In UDS cases, some structs are NULL. Protect from de-refs
> + * and provide info for logging at the same time.
> + */
> + apr_sockaddr_t *sa;
> + apr_sockaddr_info_get(&sa, NULL, APR_UNSPEC, 0, 0, conn->pool);
> + conn->hostname = "httpd-UDS";
> + conn->addr = sa;
> + }
You cannot be sure in this else branch that you are in the UDS case. It could that there is an existing hostname or that
the address is reusable. You should do the stuff above at the end of the if block starting in 2204 (if (uds_path) {
)
> + if (!(uds_path) && worker->s->is_address_reusable && !worker->s->disablereuse) {
> /*
> * Looking up the backend address for the worker only makes sense if
> * we can reuse the address.
>
Regards
Rüdiger