On 11/3/19 4:48 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Sun Nov 3 15:48:53 2019
> New Revision: 1869338
>
> URL: http://svn.apache.org/viewvc?rev=1869338&view=rev
> Log:
> mod_proxy: factorize mod_proxy_{connect,wstunnel} tunneling code in
> proxy_util.
>
> This commit adds struct proxy_tunnel_rec that contains the fields needed for a
> poll() loop through the filters chains, plus functions
> ap_proxy_tunnel_create()
> and ap_proxy_tunnel_run() to respectively initialize a tunnel and (re)start
> it.
>
> Proxy connect and wstunnel modules now make use of this new API to avoid
> duplicating logic and code.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
> httpd/httpd/trunk/modules/proxy/proxy_util.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=1869338&r1=1869337&r2=1869338&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_connect.c Sun Nov 3 15:48:53
> 2019
> @@ -364,83 +333,25 @@ static int proxy_connect_handler(request
> * Handle two way transfer of data over the socket (this is a tunnel).
> */
>
> - /* we are now acting as a tunnel - the input/output filter stacks should
> - * not contain any non-connection filters.
> - */
> - r->output_filters = c->output_filters;
> - r->proto_output_filters = c->output_filters;
> - r->input_filters = c->input_filters;
> - r->proto_input_filters = c->input_filters;
> -/* r->sent_bodyct = 1;*/
> -
> - do { /* Loop until done (one side closes the connection, or an error) */
> - rv = apr_pollset_poll(pollset, -1, &pollcnt, &signalled);
> - if (rv != APR_SUCCESS) {
> - if (APR_STATUS_IS_EINTR(rv)) {
> - continue;
> - }
> - apr_socket_close(sock);
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(01023)
> "error apr_poll()");
> - return HTTP_INTERNAL_SERVER_ERROR;
> - }
> -
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, APLOGNO(01024)
> - "woke from poll(), i=%d", pollcnt);
> + /* r->sent_bodyct = 1; */
>
> - for (pi = 0; pi < pollcnt; pi++) {
> - const apr_pollfd_t *cur = &signalled[pi];
> -
> - if (cur->desc.s == sock) {
> - pollevent = cur->rtnevents;
> - if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> APLOGNO(01025)
> - "backend was readable");
> - done |= ap_proxy_transfer_between_connections(r,
> backconn,
> - c, bb_back,
> - bb_front,
> - "backend",
> NULL,
> -
> CONN_BLKSZ, 1)
> - !=
> APR_SUCCESS;
> - }
> - else if (pollevent & APR_POLLERR) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01026)
> - "err on backend connection");
> - backconn->aborted = 1;
> - done = 1;
> - }
> - }
> - else if (cur->desc.s == client_socket) {
> - pollevent = cur->rtnevents;
> - if (pollevent & (APR_POLLIN | APR_POLLHUP)) {
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> APLOGNO(01027)
> - "client was readable");
> - done |= ap_proxy_transfer_between_connections(r, c,
> - backconn,
> - bb_front,
> - bb_back,
> - "client",
> - NULL,
> -
> CONN_BLKSZ, 1)
> - !=
> APR_SUCCESS;
> - }
> - else if (pollevent & APR_POLLERR) {
> - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02827)
> - "err on client connection");
> - c->aborted = 1;
> - done = 1;
> - }
> - }
> - else {
> - ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(01028)
> - "unknown socket in pollset");
> - done = 1;
> - }
> + rv = ap_proxy_tunnel_create(&tunnel, r, backconn);
> + if (rv != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(10208)
> + "can't create tunnel for %pI (%s)",
> + nexthop, connectname);
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
>
> + rc = ap_proxy_tunnel_run(tunnel);
> + if (ap_is_HTTP_ERROR(rc)) {
> + /* Don't send an error page if we sent data already */
> + if (proxyport && !tunnel->replied) {
> + return rc;
> }
> - } while (!done);
> -
> - ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> - "finished with poll() - cleaning up");
> + /* Custom log may need this, still */
> + r->status = rc;
I have some lively discussions outside this forum on the consequences the above
line has.
In fact this means that even though we replied to the client with a 200 on its
CONNECT request we might log a different status
code if something goes wrong during the tunneling. In contrast to this we don't
do this for other other methods e.g. GET or POST:
Once we sent the status code to the client we do not log a different one should
something go wrong while delivering the response.
Removing r->status = rc is IMHO enough to align the behavior again.
Thoughts?
Regards
Rüdiger
