On 09/10/2009 01:56 AM, [email protected] 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
