httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r813178 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_connect.c
Date Thu, 10 Sep 2009 19:27:53 GMT


On 09/10/2009 01:56 AM, minfrin@apache.org wrote:
> Author: minfrin
> Date: Wed Sep  9 23:56:29 2009
> New Revision: 813178
> 
> URL: http://svn.apache.org/viewvc?rev=813178&view=rev
> Log:
> mod_proxy_connect: The connect method doesn't work if the client is 
> connecting to the apache proxy through an ssl socket. Fixed.
> PR: 29744.
> Submitted by: Brad Boyer, Mark Cave-Ayland, Julian Gilbey, Fabrice Durand,
> David Gence, Tim Dodge, Per Gunnar Hans, Emmanuel Elango, Kevin Croft,
> Rudolf Cardinal
> 
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c
> 

> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c?rev=813178&r1=813177&r2=813178&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c Wed Sep  9 23:56:29 2009

> @@ -69,6 +71,50 @@
>      return OK;
>  }
>  
> +/* read available data (in blocks of CONN_BLKSZ) from c_i and copy to c_o */
> +static int proxy_connect_transfer(request_rec *r, conn_rec *c_i, conn_rec *c_o,
> +				  apr_bucket_brigade *bb, char *name)
> +{
> +    int rv;
> +#ifdef DEBUGGING
> +    apr_off_t len;
> +#endif
> +
> +    do {
> +	apr_brigade_cleanup(bb);
> +	rv = ap_get_brigade(c_i->input_filters, bb, AP_MODE_READBYTES,
> +			    APR_NONBLOCK_READ, CONN_BLKSZ);
> +	if (rv == APR_SUCCESS) {
> +	    if (APR_BRIGADE_EMPTY(bb))
> +		break;
> +#ifdef DEBUGGING
> +	    len = -1;
> +	    apr_brigade_length(bb, 0, &len);
> +	    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> +			  "proxy: CONNECT: read %" APR_OFF_T_FMT
> +			  " bytes from %s", len, name);
> +#endif
> +	    rv = ap_pass_brigade(c_o->output_filters, bb);
> +	    if (rv == APR_SUCCESS) {
> +		ap_fflush(c_o->output_filters, bb);

Why do flushing here? At most I would think that would be needed after the loop.

> +	    } else {
> +		ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> +			      "proxy: CONNECT: error on %s - ap_pass_brigade",
> +			      name);
> +	    }
> +	} else if (!APR_STATUS_IS_EAGAIN(rv)) {
> +	    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, rv, r,
> +			  "proxy: CONNECT: error on %s - ap_get_brigade",
> +			  name);
> +	}
> +    } while (rv == APR_SUCCESS);
> +
> +    if (APR_STATUS_IS_EAGAIN(rv)) {
> +	rv = APR_SUCCESS;
> +    }
> +    return rv;
> +}
> +
>  /* CONNECT handler */
>  static int proxy_connect_handler(request_rec *r, proxy_worker *worker,
>                                   proxy_server_conf *conf,

> @@ -203,18 +251,57 @@
>          }
>      }
>  
> +    /* setup polling for connection */
> +    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> +		  "proxy: CONNECT: setting up poll()");
> +
> +    if ((rv = apr_pollset_create(&pollset, 2, r->pool, 0)) != APR_SUCCESS) {
> +	apr_socket_close(sock);
> +        ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> +            "proxy: CONNECT: error apr_pollset_create()");
> +        return HTTP_INTERNAL_SERVER_ERROR;
> +    }
> +
> +    /* Add client side to the poll */
> +    pollfd.p = r->pool;
> +    pollfd.desc_type = APR_POLL_SOCKET;
> +    pollfd.reqevents = APR_POLLIN;
> +    pollfd.desc.s = client_socket;
> +    pollfd.client_data = NULL;
> +    apr_pollset_add(pollset, &pollfd);
> +
> +    /* Add the server side to the poll */
> +    pollfd.desc.s = sock;
> +    apr_pollset_add(pollset, &pollfd);
> +

Why moving this code block up and not leaving it where it was?

>      /*
>       * Step Three: Send the Request
>       *
>       * Send the HTTP/1.1 CONNECT request to the remote server
>       */
>  
> -    /* we are acting as a tunnel - the output filter stack should
> -     * be completely empty, because when we are done here we are done completely.
> -     * We add the NULL filter to the stack to do this...
> -     */
> -    r->output_filters = NULL;
> -    r->connection->output_filters = NULL;
> +    backconn = ap_run_create_connection(c->pool, r->server, sock,
> +					c->id, c->sbh, c->bucket_alloc);
> +    if (!backconn) {
> +	/* peer reset */
> +	ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
> +		      "proxy: an error occurred creating a new connection "
> +		      "to %pI (%s)", connect_addr, connectname);
> +	apr_socket_close(sock);
> +	return HTTP_INTERNAL_SERVER_ERROR;
> +    }
> +    ap_proxy_ssl_disable(backconn);
> +    rc = ap_run_pre_connection(backconn, sock);
> +    if (rc != OK && rc != DONE) {
> +	backconn->aborted = 1;
> +	ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> +		      "proxy: CONNECT: pre_connection setup failed (%d)", rc);
> +	return HTTP_INTERNAL_SERVER_ERROR;
> +    }
> +
> +    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> +		  "proxy: CONNECT: connection complete to %pI (%s)",
> +		  connect_addr, connectname);

Why do we now use a connection? Why don't we stick to writing to the socket directly
for the backend connection. In this direction we clearly do no want to filter anything.

>  
>  
>      /* If we are connecting through a remote proxy, we need to pass

> @@ -396,7 +418,9 @@
>       * Close the socket and clean up
>       */
>  
> -    apr_socket_close(sock);
> +    ap_lingering_close(backconn);
> +
> +    c->aborted = 1;

This is certainly wrong. Why is this done?

>  
>      return OK;
>  }
> 

In general I would really appreciate to fix the style before committing such patches
especially if a patch contains that much style violations as this one.

Regards

RĂ¼diger

Mime
View raw message