[PATCH] fixes & cleanups

Hi all,

This patch cleans up the ap_proxy_http_handler() function. In the process it fixes the following bugs:

• headers from the server were not passed to the client.
• the OPTIONS method was not handled properly.
• some residual cache code was removed.

It's a bit convoluted, basically because of the changes of variable names (resphdrs was turfed in favour of a more consistent headers_out), but it's mostly limited to the single routine.

I'll be away for a few days till the end of the week (sun, sea, sand... sigh :)), if there are no objections I'll commit it when I get back.  

Regards,
Graham

-- 
-----------------------------------------
minfrin@sharp.fm		"There's a moon
					over Bourbon Street
						tonight..."

diff -u3 -r --exclude=CVS ../../../../pristine/httpd-proxy/module-2.0/mod_proxy.c proxy/mod_proxy.c --- ../../../../pristine/httpd-proxy/module-2.0/mod_proxy.c Mon Mar 12 00:33:08 2001 +++ proxy/mod_proxy.c Mon Mar 19 16:38:00 2001 @@ -298,17 +298,30 @@ if (!r->proxyreq || strncmp(r->filename, "proxy:", 6) != 0) return DECLINED;
- if (r->method_number == M_TRACE &&
+ if ((r->method_number == M_TRACE || r->method_number == M_OPTIONS) && (maxfwd_str = apr_table_get(r->headers_in, "Max-Forwards")) != NULL) { long maxfwd = strtol(maxfwd_str, NULL, 10); if (maxfwd < 1) { - int access_status; - r->proxyreq = 0; - if ((access_status = ap_send_http_trace(r))) - ap_die(access_status, r); - else - ap_finalize_request_protocol(r); - return OK; + switch (r->method_number) { + case M_TRACE: { + int access_status; + r->proxyreq = 0; + if ((access_status = ap_send_http_trace(r))) + ap_die(access_status, r); + else + ap_finalize_request_protocol(r); + return OK; + } + case M_OPTIONS: { + int access_status; + r->proxyreq = 0; + if ((access_status = ap_send_http_options(r))) + ap_die(access_status, r); + else + ap_finalize_request_protocol(r); + return OK; + } + } } apr_table_setn(r->headers_in, "Max-Forwards", apr_psprintf(r->pool, "%ld", (maxfwd > 0) ? maxfwd-1 : 0)); diff -u3 -r --exclude=CVS ../../../../pristine/httpd-proxy/module-2.0/proxy_http.c proxy/proxy_http.c --- ../../../../pristine/httpd-proxy/module-2.0/proxy_http.c Wed Mar 14 23:27:55 2001 +++ proxy/proxy_http.c Mon Mar 19 21:37:04 2001 @@ -179,67 +179,77 @@ int ap_proxy_http_handler(request_rec *r, char *url, const char *proxyhost, int proxyport) {
- const char *strp;
- char *strp2;
+ apr_pool_t *p = r->pool; char *desthost; + int destport = 0; + char *destportstr = NULL; + char server_portstr[32]; + const char *uri = NULL; apr_socket_t *sock;
- int i, len, backasswards, content_length = -1;
+ int i, len, backasswards; apr_status_t err;
- apr_array_header_t *reqhdrs_arr;
+ apr_array_header_t *headers_in_array; + apr_table_entry_t *headers_in; struct sockaddr_in server; struct in_addr destaddr; char buffer[HUGE_STRING_LEN];
- char *buffer2;
- char portstr[32];
- apr_pool_t *p = r->pool;
- int destport = 0;
- char *destportstr = NULL;
- const char *urlptr = NULL;
- apr_ssize_t cntr;
- apr_file_t *cachefp = NULL;
+ char *response; char *buf; conn_rec *origin; apr_bucket *e;
- apr_bucket_brigade *bb = apr_brigade_create(r->pool);
+ apr_bucket_brigade *bb = apr_brigade_create(p); void *sconf = r->server->module_config; proxy_server_conf *conf = (proxy_server_conf *) ap_get_module_config(sconf, &proxy_module); struct noproxy_entry *npent = (struct noproxy_entry *) conf->noproxies->elts;
- int nocache = 0;
memset(&server, '\0', sizeof(server)); server.sin_family = AF_INET;
- /* We break the URL into host, port, path-search */
-
- urlptr = strstr(url, "://");
- if (urlptr == NULL)
- return HTTP_BAD_REQUEST;
- urlptr += 3;
- destport = DEFAULT_HTTP_PORT;
- strp = ap_strchr_c(urlptr, '/');
- if (strp == NULL) {
- desthost = apr_pstrdup(p, urlptr); - urlptr = "/"; + /* We break the URL into host, port, uri */ + { + const char *buf; + + uri = strstr(url, "://"); + if (uri == NULL) + return HTTP_BAD_REQUEST; + uri += 3; + destport = DEFAULT_HTTP_PORT; + buf = ap_strchr_c(uri, '/'); + if (buf == NULL) { + desthost = apr_pstrdup(p, uri); + uri = "/"; + } + else { + char *q = apr_palloc(p, buf - uri + 1); + memcpy(q, uri, buf - uri); + q[buf - uri] = '\0'; + uri = buf; + desthost = q; + } }
- else {
- char *q = apr_palloc(p, strp - urlptr + 1); - memcpy(q, urlptr, strp - urlptr); - q[strp - urlptr] = '\0'; - urlptr = strp; - desthost = q; + + /* Get the port number - put it in destport and destportstr */ + { + char *buf; + buf = ap_strchr(desthost, ':'); + if (buf != NULL) { + *(buf++) = '\0'; + if (apr_isdigit(*buf)) { + destport = atoi(buf); + destportstr = buf; + } + } }
- strp2 = ap_strchr(desthost, ':');
- if (strp2 != NULL) {
- *(strp2++) = '\0'; - if (apr_isdigit(*strp2)) { - destport = atoi(strp2); - destportstr = strp2; + /* Get the server port for the Via headers */ + { + i = ap_get_server_port(r); + if (ap_is_default_port(i,r)) { + strcpy(server_portstr,""); + } else { + apr_snprintf(server_portstr, sizeof server_portstr, ":%d", i); } } @@ -254,7 +264,7 @@ "Connect to remote machine blocked"); }
- if ((apr_socket_create(&sock, APR_INET, SOCK_STREAM, r->pool)) != APR_SUCCESS) {
+ if ((apr_socket_create(&sock, APR_INET, SOCK_STREAM, p)) != APR_SUCCESS) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, "proxy: error creating socket"); return HTTP_INTERNAL_SERVER_ERROR; @@ -279,18 +289,18 @@ if (proxyhost != NULL) return DECLINED; /* try again another way */ else - return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(r->pool, + return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(p, "Could not connect to remote machine: ", desthost, NULL)); }
- origin = ap_new_connection(r->pool, r->server, sock, 0);
+ origin = ap_new_connection(p, r->server, sock, 0); if (!origin) { /* the peer reset the connection already; ap_new_connection() * closed the socket */ /* XXX somebody that knows what they're doing add an error path */ /* XXX how's this? */ - return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(r->pool, + return ap_proxyerror(r, HTTP_BAD_GATEWAY, apr_pstrcat(p, "Connection reset by peer: ", desthost, NULL)); } @@ -298,20 +308,20 @@ ap_add_output_filter("CORE", NULL, NULL, origin); /* strip connection listed hop-by-hop headers from the request */
- ap_proxy_clear_connection(r->pool, r->headers_in);
+ ap_proxy_clear_connection(p, r->headers_in);
- buf = apr_pstrcat(r->pool, r->method, " ", proxyhost ? url : urlptr,
+ buf = apr_pstrcat(p, r->method, " ", proxyhost ? url : uri, " HTTP/1.1" CRLF, NULL);
- e = apr_bucket_pool_create(buf, strlen(buf), r->pool);
+ e = apr_bucket_pool_create(buf, strlen(buf), p); APR_BRIGADE_INSERT_TAIL(bb, e); if (destportstr != NULL && destport != DEFAULT_HTTP_PORT) {
- buf = apr_pstrcat(r->pool, "Host: ", desthost, ":", destportstr, CRLF, NULL);
- e = apr_bucket_pool_create(buf, strlen(buf), r->pool);
+ buf = apr_pstrcat(p, "Host: ", desthost, ":", destportstr, CRLF, NULL); + e = apr_bucket_pool_create(buf, strlen(buf), p); APR_BRIGADE_INSERT_TAIL(bb, e); } else {
- buf = apr_pstrcat(r->pool, "Host: ", desthost, CRLF, NULL);
- e = apr_bucket_pool_create(buf, strlen(buf), r->pool);
+ buf = apr_pstrcat(p, "Host: ", desthost, CRLF, NULL); + e = apr_bucket_pool_create(buf, strlen(buf), p); APR_BRIGADE_INSERT_TAIL(bb, e); } @@ -321,42 +331,40 @@ apr_table_unset(r->headers_in, "Via"); } else if (conf->viaopt != via_off) { /* Create a "Via:" request header entry and merge it */ - i = ap_get_server_port(r); - if (ap_is_default_port(i,r)) { - strcpy(portstr,""); - } else { - apr_snprintf(portstr, sizeof portstr, ":%d", i); - } /* Generate outgoing Via: header with/without server comment: */ apr_table_mergen(r->headers_in, "Via", (conf->viaopt == via_full) ? apr_psprintf(p, "%d.%d %s%s (%s)", HTTP_VERSION_MAJOR(r->proto_num), HTTP_VERSION_MINOR(r->proto_num), - ap_get_server_name(r), portstr, + ap_get_server_name(r), server_portstr, AP_SERVER_BASEVERSION) : apr_psprintf(p, "%d.%d %s%s", HTTP_VERSION_MAJOR(r->proto_num), HTTP_VERSION_MINOR(r->proto_num), - ap_get_server_name(r), portstr) + ap_get_server_name(r), server_portstr) ); } + /* Add X-Forwarded-For: so that the upstream has a chance to + determine, where the original request came from. */ + apr_table_mergen(r->headers_in, "X-Forwarded-For", r->connection->remote_ip); + /* send request headers */
- reqhdrs_arr = apr_table_elts(r->headers_in);
- reqhdrs = (apr_table_entry_t *) reqhdrs_arr->elts;
- for (i = 0; i < reqhdrs_arr->nelts; i++) {
- if (reqhdrs[i].key == NULL || reqhdrs[i].val == NULL + headers_in_array = apr_table_elts(r->headers_in); + headers_in = (apr_table_entry_t *) headers_in_array->elts; + for (i = 0; i < headers_in_array->nelts; i++) { + if (headers_in[i].key == NULL || headers_in[i].val == NULL /* Clear out hop-by-hop request headers not to send * RFC2616 13.5.1 says we should strip these headers */ - || !strcasecmp(reqhdrs[i].key, "Host") /* Already sent */
- || !strcasecmp(reqhdrs[i].key, "Keep-Alive")
+ || !strcasecmp(headers_in[i].key, "Host") /* Already sent */ + || !strcasecmp(headers_in[i].key, "Keep-Alive") + || !strcasecmp(headers_in[i].key, "TE") + || !strcasecmp(headers_in[i].key, "Trailer") + || !strcasecmp(headers_in[i].key, "Transfer-Encoding") + || !strcasecmp(headers_in[i].key, "Upgrade") /* XXX: @@@ FIXME: "Proxy-Authorization" should *only* be * suppressed if THIS server requested the authentication, @@ -367,23 +375,23 @@ * code itself, not here. This saves us having to signal * somehow whether this request was authenticated or not. */ - || !strcasecmp(reqhdrs[i].key, "Proxy-Authorization") - || !strcasecmp(reqhdrs[i].key, "Proxy-Authenticate")) + || !strcasecmp(headers_in[i].key, "Proxy-Authorization") + || !strcasecmp(headers_in[i].key, "Proxy-Authenticate")) continue;
- buf = apr_pstrcat(r->pool, reqhdrs[i].key, ": ", reqhdrs[i].val, CRLF, NULL);
- e = apr_bucket_pool_create(buf, strlen(buf), r->pool);
+ buf = apr_pstrcat(p, headers_in[i].key, ": ", headers_in[i].val, CRLF, NULL); + e = apr_bucket_pool_create(buf, strlen(buf), p); APR_BRIGADE_INSERT_TAIL(bb, e); } /* we don't yet support keepalives - but we will soon, I promise! */
- buf = apr_pstrcat(r->pool, "Connection: close", CRLF, NULL);
+ buf = apr_pstrcat(p, "Connection: close", CRLF, NULL); + e = apr_bucket_pool_create(buf, strlen(buf), p); APR_BRIGADE_INSERT_TAIL(bb, e); /* add empty line at the end of the headers */
- e = apr_bucket_pool_create(CRLF, strlen(CRLF), r->pool);
+ e = apr_bucket_pool_create(CRLF, strlen(CRLF), p); APR_BRIGADE_INSERT_TAIL(bb, e); e = apr_bucket_flush_create(); APR_BRIGADE_INSERT_TAIL(bb, e); @@ -393,7 +401,7 @@ /* send the request data, if any. */ if (ap_should_client_block(r)) { while ((i = ap_get_client_block(r, buffer, sizeof buffer)) > 0) {
- e = apr_bucket_pool_create(buffer, i, r->pool);
+ e = apr_bucket_pool_create(buffer, i, p); APR_BRIGADE_INSERT_TAIL(bb, e); } } @@ -406,14 +414,14 @@ ap_add_input_filter("CORE_IN", NULL, NULL, origin); apr_brigade_destroy(bb);
- bb = apr_brigade_create(r->pool);
+ bb = apr_brigade_create(p); /* Tell http_filter to grab the data one line at a time. */ origin->remain = 0; ap_get_brigade(origin->input_filters, bb, AP_MODE_BLOCKING); e = APR_BRIGADE_FIRST(bb);
- apr_bucket_read(e, (const char **)&buffer2, &len, APR_BLOCK_READ);
+ apr_bucket_read(e, (const char **)&response, &len, APR_BLOCK_READ); if (len == -1) { apr_socket_close(sock); ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, @@ -430,56 +438,62 @@ apr_bucket_destroy(e); /* Is it an HTTP/1 response? This is buggy if we ever see an HTTP/1.10 */
- if (ap_checkmask(buffer2, "HTTP/#.# ###*")) {
+ if (ap_checkmask(response, "HTTP/#.# ###*")) { int major, minor; - if (2 != sscanf(buffer2, "HTTP/%u.%u", &major, &minor)) { + if (2 != sscanf(response, "HTTP/%u.%u", &major, &minor)) { major = 1; minor = 1; } /* If not an HTTP/1 message or if the status line was > 8192 bytes */ - if (buffer2[5] != '1' || buffer2[len - 1] != '\n') { + if (response[5] != '1' || response[len - 1] != '\n') { apr_socket_close(sock); return HTTP_BAD_GATEWAY; } backasswards = 0; - buffer2[--len] = '\0'; + response[--len] = '\0'; - buffer2[12] = '\0'; - r->status = atoi(&buffer2[9]); + response[12] = '\0'; + r->status = atoi(&response[9]); - buffer2[12] = ' '; - r->status_line = apr_pstrdup(p, &buffer2[9]); + response[12] = ' '; + r->status_line = apr_pstrdup(p, &response[9]); /* read the headers. */ /* N.B. for HTTP/1.0 clients, we have to fold line-wrapped headers */ /* Also, take care with headers with multiple occurences. */ - resp_hdrs = ap_proxy_read_headers(r, buffer, HUGE_STRING_LEN, origin); - if (resp_hdrs == NULL) { + r->headers_out = ap_proxy_read_headers(r, buffer, HUGE_STRING_LEN, origin); + if (r->headers_out == NULL) { ap_log_error(APLOG_MARK, APLOG_WARNING|APLOG_NOERRNO, 0, r->server, "proxy: Bad HTTP/%d.%d header returned by %s (%s)", major, minor, r->uri, r->method); - nocache = 1; /* do not cache this broken file */ } else { /* strip connection listed hop-by-hop headers from response */
- ap_proxy_clear_connection(p, resp_hdrs);
+ const char *buf; + ap_proxy_clear_connection(p, r->headers_out); + if ((buf = apr_table_get(r->headers_out, "Content-type"))) { + r->content_type = apr_pstrdup(p, buf); } } /* handle Via header in response */ if (conf->viaopt != via_off && conf->viaopt != via_block) { /* Create a "Via:" response header entry and merge it */ - i = ap_get_server_port(r); - if (ap_is_default_port(i,r)) { - strcpy(portstr,""); - } else { - apr_snprintf(portstr, sizeof portstr, ":%d", i); - } + ap_table_mergen(r->headers_out, "Via", + (conf->viaopt == via_full) + ? apr_psprintf(p, "%d.%d %s%s (%s)", + HTTP_VERSION_MAJOR(r->proto_num), + HTTP_VERSION_MINOR(r->proto_num), + ap_get_server_name(r), server_portstr, + AP_SERVER_BASEVERSION) + : apr_psprintf(p, "%d.%d %s%s", + HTTP_VERSION_MAJOR(r->proto_num), + HTTP_VERSION_MINOR(r->proto_num), + ap_get_server_name(r), server_portstr) + ); } } else { @@ -490,38 +504,29 @@ } /* munge the Location and URI response headers according to ProxyPassReverse */
- if ((table_buf = apr_table_get(resp_hdrs, "Location")) != NULL)
- -/*
- if (!r->assbackwards)
- ap_rputs(CRLF, r); -*/ + { + const char *buf; + if ((buf = apr_table_get(r->headers_out, "Location")) != NULL) + apr_table_set(r->headers_out, "Location", ap_proxy_location_reverse_map(r, buf)); + if ((buf = apr_table_get(r->headers_out, "Content-Location")) != NULL) + apr_table_set(r->headers_out, "Content-Location", ap_proxy_location_reverse_map(r, buf)); + if ((buf = apr_table_get(r->headers_out, "URI")) != NULL) + apr_table_set(r->headers_out, "URI", ap_proxy_location_reverse_map(r, buf)); + } + r->sent_bodyct = 1; /* Is it an HTTP/0.9 response? If so, send the extra data */ if (backasswards) {
- cntr = len;
+ apr_ssize_t cntr = len; + /* FIXME: what is buffer used for here? It is of limited size */ e = apr_bucket_heap_create(buffer, cntr, 0, NULL); APR_BRIGADE_INSERT_TAIL(bb, e);
- if (cachefp && apr_file_write(cachefp, buffer, &cntr) != APR_SUCCESS) {
- ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, - "proxy: error writing extra data to cache"); - } } /* send body */
- /* if header only, then cache will be NULL */
/* HTTP/1.0 tells us to read to EOF, rather than content-length bytes */ if (!r->header_only) {
- proxy_completion pc;
- pc.content_length = content_length;
- pc.cache_completion = conf->cache_completion;
-
- origin->remain = content_length;
+ origin->remain = -1; while (ap_get_brigade(origin->input_filters, bb, AP_MODE_BLOCKING) == APR_SUCCESS) { if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) { ap_pass_brigade(r->output_filters, bb); @@ -529,7 +534,7 @@ } ap_pass_brigade(r->output_filters, bb); apr_brigade_destroy(bb);
- bb = apr_brigade_create(r->pool);
+ bb = apr_brigade_create(p); } } Received on Mon Mar 19 20:49:35 2001