On 09/10/2009 01:56 AM, minfrin@apac... 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