You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/01/24 00:32:04 UTC
Re: svn commit: r1560689 - /httpd/httpd/trunk/modules/proxy/proxy_util.c
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]