From 2023d56eeae20399a613734889e50df13f8989ce Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Thu, 11 Aug 2016 22:32:42 +0000 Subject: [PATCH] [mod_proxy_]http: follow up to r1750392. Export [ap_]check_pipeline() and use it also for ap_proxy_check_connection(). [Reverted by r1756065] git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1756060 13f79535-47bb-0310-9956-ffa450edef68 --- include/ap_mmn.h | 7 ++-- include/http_request.h | 15 +++++++ modules/http/http_request.c | 75 ++++++++++++++++++++++------------ modules/proxy/mod_proxy.h | 33 ++++++++------- modules/proxy/mod_proxy_ajp.c | 6 ++- modules/proxy/mod_proxy_fcgi.c | 28 ++++++------- modules/proxy/mod_proxy_http.c | 7 ++-- modules/proxy/proxy_util.c | 44 ++++++++------------ 8 files changed, 127 insertions(+), 88 deletions(-) diff --git a/include/ap_mmn.h b/include/ap_mmn.h index d0d8c889eb..19d13d862a 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -532,10 +532,11 @@ * dav_success_proppatch. * 20160608.4 (2.5.0-dev) Add dav_acl_provider, dav_acl_provider_register * dav_get_acl_providers. - * 20160608.5 (2.5.0-dev) Add ap_proxy_check_backend(), and tmp_bb to struct - * proxy_conn_rec. + * 20160608.5 (2.5.0-dev) Add ap_proxy_check_connection(), and tmp_bb to + * struct proxy_conn_rec. * 20160608.6 (2.5.0-dev) Add ap_scan_http_field_content, ap_scan_http_token * and ap_get_http_token + * 20160608.7 (2.5.0-dev) Add ap_check_pipeline(). */ #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */ @@ -543,7 +544,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20160608 #endif -#define MODULE_MAGIC_NUMBER_MINOR 6 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 7 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/include/http_request.h b/include/http_request.h index 4d82c8a3fe..8107a4247f 100644 --- a/include/http_request.h +++ b/include/http_request.h @@ -350,6 +350,21 @@ void ap_process_async_request(request_rec *r); */ AP_DECLARE(void) ap_die(int type, request_rec *r); +/** + * Check whether a connection is still established and has data available, + * optionnaly consuming blank lines ([CR]LF). + * @param c The current connection + * @param bb The brigade to filter + * @param max_blank_lines Max number of blank lines to consume, or zero + * to consider them as data (single read). + * @return APR_SUCCESS: connection ready and empty, + * APR_ENOTEMPTY: connection ready with data available, + * APR_NOTFOUND: too much blank lines, + * APR_E*: connection/general error. + */ +AP_DECLARE(apr_status_t) ap_check_pipeline(conn_rec *c, apr_bucket_brigade *bb, + unsigned int max_blank_lines); + /* Hooks */ /** diff --git a/modules/http/http_request.c b/modules/http/http_request.c index ee50b4232a..564a195d8f 100644 --- a/modules/http/http_request.c +++ b/modules/http/http_request.c @@ -229,41 +229,45 @@ AP_DECLARE(void) ap_die(int type, request_rec *r) ap_die_r(type, r, r->status); } -static void check_pipeline(conn_rec *c, apr_bucket_brigade *bb) +AP_DECLARE(apr_status_t) ap_check_pipeline(conn_rec *c, apr_bucket_brigade *bb, + unsigned int max_blank_lines) { - apr_status_t rv; - int num_blank_lines = DEFAULT_LIMIT_BLANK_LINES; + apr_status_t rv = APR_EOF; ap_input_mode_t mode = AP_MODE_SPECULATIVE; + unsigned int num_blank_lines = 0; apr_size_t cr = 0; char buf[2]; - c->data_in_input_filters = 0; while (c->keepalive != AP_CONN_CLOSE && !c->aborted) { apr_size_t len = cr + 1; apr_brigade_cleanup(bb); rv = ap_get_brigade(c->input_filters, bb, mode, APR_NONBLOCK_READ, len); - if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) { - /* - * Error or empty brigade: There is no data present in the input - * filter - */ + if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb) || !max_blank_lines) { if (mode == AP_MODE_READBYTES) { /* Unexpected error, stop with this connection */ ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO(02967) "Can't consume pipelined empty lines"); c->keepalive = AP_CONN_CLOSE; + rv = APR_EGENERAL; + } + else if (rv == APR_SUCCESS && !APR_BRIGADE_EMPTY(bb)) { + /* Single read asked, data available */ + rv = APR_ENOTEMPTY; + } + else if (APR_STATUS_IS_EAGAIN(rv)) { + /* Pipe is empty and ready */ + rv = APR_SUCCESS; + } + else { + /* Pipe is dead, rv up to date */ + c->keepalive = AP_CONN_CLOSE; } break; } - /* Ignore trailing blank lines (which must not be interpreted as - * pipelined requests) up to the limit, otherwise we would block - * on the next read without flushing data, and hence possibly delay - * pending response(s) until the next/real request comes in or the - * keepalive timeout expires. - */ + /* Lookup and consume blank lines */ rv = apr_brigade_flatten(bb, buf, &len); if (rv != APR_SUCCESS || len != cr + 1) { int log_level; @@ -271,14 +275,17 @@ static void check_pipeline(conn_rec *c, apr_bucket_brigade *bb) /* Unexpected error, stop with this connection */ c->keepalive = AP_CONN_CLOSE; log_level = APLOG_ERR; + if (rv == APR_SUCCESS) { + rv = APR_EGENERAL; + } } else { /* Let outside (non-speculative/blocking) read determine * where this possible failure comes from (metadata, * morphed EOF socket => empty bucket? debug only here). */ - c->data_in_input_filters = 1; log_level = APLOG_DEBUG; + rv = APR_ENOTEMPTY; } ap_log_cerror(APLOG_MARK, log_level, rv, c, APLOGNO(02968) "Can't check pipelined data"); @@ -286,40 +293,48 @@ static void check_pipeline(conn_rec *c, apr_bucket_brigade *bb) } if (mode == AP_MODE_READBYTES) { + /* [CR]LF consumed, try next */ mode = AP_MODE_SPECULATIVE; cr = 0; } else if (cr) { AP_DEBUG_ASSERT(len == 2 && buf[0] == APR_ASCII_CR); if (buf[1] == APR_ASCII_LF) { + /* consume this CRLF */ mode = AP_MODE_READBYTES; - num_blank_lines--; + num_blank_lines++; } else { - c->data_in_input_filters = 1; + /* CR(?!LF) is data */ + rv = APR_ENOTEMPTY; break; } } else { if (buf[0] == APR_ASCII_LF) { - mode = AP_MODE_READBYTES; - num_blank_lines--; + /* consume this LF */ + num_blank_lines++; } else if (buf[0] == APR_ASCII_CR) { cr = 1; } else { - c->data_in_input_filters = 1; + /* Not [CR]LF, some data */ + rv = APR_ENOTEMPTY; break; } } - /* Enough blank lines with this connection? - * Stop and don't recycle it. - */ - if (num_blank_lines < 0) { + if (num_blank_lines > max_blank_lines) { + /* Enough blank lines with this connection, + * stop and don't recycle it. + */ c->keepalive = AP_CONN_CLOSE; + rv = APR_NOTFOUND; + break; } } + + return rv; } @@ -328,6 +343,7 @@ AP_DECLARE(void) ap_process_request_after_handler(request_rec *r) apr_bucket_brigade *bb; apr_bucket *b; conn_rec *c = r->connection; + apr_status_t rv; /* Send an EOR bucket through the output filter chain. When * this bucket is destroyed, the request will be logged and @@ -360,8 +376,15 @@ AP_DECLARE(void) ap_process_request_after_handler(request_rec *r) * already by the EOR bucket's cleanup function. */ - check_pipeline(c, bb); + /* Check pipeline consuming blank lines, they must not be interpreted as + * the next pipelined request, otherwise we would block on the next read + * without flushing data, and hence possibly delay pending response(s) + * until the next/real request comes in or the keepalive timeout expires. + */ + rv = ap_check_pipeline(c, bb, DEFAULT_LIMIT_BLANK_LINES); + c->data_in_input_filters = (rv == APR_ENOTEMPTY); apr_brigade_destroy(bb); + if (c->cs) c->cs->state = (c->aborted) ? CONN_STATE_LINGER : CONN_STATE_WRITE_COMPLETION; diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h index 9a4812a44b..59ae8ecb17 100644 --- a/modules/proxy/mod_proxy.h +++ b/modules/proxy/mod_proxy.h @@ -671,7 +671,7 @@ PROXY_DECLARE(int) ap_proxy_checkproxyblock(request_rec *r, proxy_server_conf *c PROXY_DECLARE(int) ap_proxy_pre_http_request(conn_rec *c, request_rec *r); /* DEPRECATED (will be replaced with ap_proxy_connect_backend */ PROXY_DECLARE(int) ap_proxy_connect_to_backend(apr_socket_t **, const char *, apr_sockaddr_t *, const char *, proxy_server_conf *, request_rec *); -/* DEPRECATED (will be replaced with ap_proxy_check_backend */ +/* DEPRECATED (will be replaced with ap_proxy_check_connection */ PROXY_DECLARE(apr_status_t) ap_proxy_ssl_connection_cleanup(proxy_conn_rec *conn, request_rec *r); PROXY_DECLARE(int) ap_proxy_ssl_enable(conn_rec *c); @@ -977,22 +977,27 @@ PROXY_DECLARE(int) ap_proxy_acquire_connection(const char *proxy_function, PROXY_DECLARE(int) ap_proxy_release_connection(const char *proxy_function, proxy_conn_rec *conn, server_rec *s); + +#define PROXY_CHECK_CONN_EMPTY (1 << 0) /** * Check a connection to the backend - * @param proxy_function calling proxy scheme (http, ajp, ...) - * @param conn acquired connection - * @param s current server record - * @param expect_empty whether to check for empty (no data available) or not - * @return APR_SUCCESS or, - * APR_ENOTSOCK: not connected, - * APR_NOTFOUND: worker in error state (unusable), - * APR_ENOTEMPTY: expect_empty set but the connection has data, - * other: connection closed/aborted (remotely) + * @param scheme calling proxy scheme (http, ajp, ...) + * @param conn acquired connection + * @param server current server record + * @param max_blank_lines how many blank lines to consume, + * or zero for none (considered data) + * @param flags PROXY_CHECK_* bitmask + * @return APR_SUCCESS: connection ready and empty, + * APR_ENOTEMPTY: connection ready and has data, + * APR_ENOSOCKET: not connected, + * APR_EINVAL: worker in error state (unusable), + * other: connection closed/aborted (remotely) */ -PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function, - proxy_conn_rec *conn, - server_rec *s, - int expect_empty); +PROXY_DECLARE(apr_status_t) ap_proxy_check_connection(const char *scheme, + proxy_conn_rec *conn, + server_rec *server, + unsigned max_blank_lines, + int flags); /** * Make a connection to the backend diff --git a/modules/proxy/mod_proxy_ajp.c b/modules/proxy/mod_proxy_ajp.c index b8e818adb2..a31afa9b1a 100644 --- a/modules/proxy/mod_proxy_ajp.c +++ b/modules/proxy/mod_proxy_ajp.c @@ -783,8 +783,10 @@ static int proxy_ajp_handler(request_rec *r, proxy_worker *worker, break; /* Step Two: Make the Connection */ - if (ap_proxy_check_backend(scheme, backend, r->server, 1) && - ap_proxy_connect_backend(scheme, backend, worker, r->server)) { + if (ap_proxy_check_connection(scheme, backend, r->server, 0, + PROXY_CHECK_CONN_EMPTY) + && ap_proxy_connect_backend(scheme, backend, worker, + r->server)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(00896) "failed to make connection to backend: %s", backend->hostname); diff --git a/modules/proxy/mod_proxy_fcgi.c b/modules/proxy/mod_proxy_fcgi.c index 2a760fcb02..467bd7bf66 100644 --- a/modules/proxy/mod_proxy_fcgi.c +++ b/modules/proxy/mod_proxy_fcgi.c @@ -928,16 +928,6 @@ static int proxy_fcgi_handler(request_rec *r, proxy_worker *worker, backend->is_ssl = 0; - /* Step One: Determine Who To Connect To */ - uri = apr_palloc(p, sizeof(*uri)); - status = ap_proxy_determine_connection(p, r, conf, worker, backend, - uri, &url, proxyname, proxyport, - server_portstr, - sizeof(server_portstr)); - if (status != OK) { - goto cleanup; - } - /* This scheme handler does not reuse connections by default, to * avoid tying up a fastcgi that isn't expecting to work on * parallel requests. But if the user went out of their way to @@ -948,11 +938,21 @@ static int proxy_fcgi_handler(request_rec *r, proxy_worker *worker, backend->close = 0; } + /* Step One: Determine Who To Connect To */ + uri = apr_palloc(p, sizeof(*uri)); + status = ap_proxy_determine_connection(p, r, conf, worker, backend, + uri, &url, proxyname, proxyport, + server_portstr, + sizeof(server_portstr)); + if (status != OK) { + goto cleanup; + } + /* Step Two: Make the Connection */ - if ((backend->close || ap_proxy_check_backend(FCGI_SCHEME, backend, - r->server, 1)) && - ap_proxy_connect_backend(FCGI_SCHEME, backend, worker, - r->server)) { + if (ap_proxy_check_connection(FCGI_SCHEME, backend, r->server, 0, + PROXY_CHECK_CONN_EMPTY) + && ap_proxy_connect_backend(FCGI_SCHEME, backend, worker, + r->server)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01079) "failed to make connection to backend: %s", backend->hostname); diff --git a/modules/proxy/mod_proxy_http.c b/modules/proxy/mod_proxy_http.c index f5731652bf..ae6384424c 100644 --- a/modules/proxy/mod_proxy_http.c +++ b/modules/proxy/mod_proxy_http.c @@ -2068,9 +2068,10 @@ static int proxy_http_handler(request_rec *r, proxy_worker *worker, } /* Step Two: Make the Connection */ - if (ap_proxy_check_backend(proxy_function, backend, r->server, 1) && - ap_proxy_connect_backend(proxy_function, backend, worker, - r->server)) { + if (ap_proxy_check_connection(proxy_function, backend, r->server, 1, + PROXY_CHECK_CONN_EMPTY) + && ap_proxy_connect_backend(proxy_function, backend, worker, + r->server)) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01114) "HTTP: failed to make connection to backend: %s", backend->hostname); diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c index 00bac1e307..af8479c1dd 100644 --- a/modules/proxy/proxy_util.c +++ b/modules/proxy/proxy_util.c @@ -2699,10 +2699,11 @@ PROXY_DECLARE(apr_status_t) ap_proxy_connect_uds(apr_socket_t *sock, #endif } -PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function, - proxy_conn_rec *conn, - server_rec *s, - int expect_empty) +PROXY_DECLARE(apr_status_t) ap_proxy_check_connection(const char *scheme, + proxy_conn_rec *conn, + server_rec *server, + unsigned max_blank_lines, + int flags) { apr_status_t rv = APR_SUCCESS; proxy_worker *worker = conn->worker; @@ -2713,25 +2714,17 @@ PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function, * e.g. for a timeout or bad status. We should respect this and should * not continue with a connection via this worker even if we got one. */ - rv = APR_NOTFOUND; + rv = APR_EINVAL; } else if (!conn->sock) { - rv = APR_ENOTSOCK; + rv = APR_ENOSOCKET; } else if (conn->connection) { - conn_rec *c = conn->connection; - rv = ap_get_brigade(c->input_filters, conn->tmp_bb, - AP_MODE_SPECULATIVE, APR_NONBLOCK_READ, 1); - if (APR_STATUS_IS_EAGAIN(rv)) { + rv = ap_check_pipeline(conn->connection, conn->tmp_bb, + max_blank_lines); + if (rv == APR_ENOTEMPTY && !(flags & PROXY_CHECK_CONN_EMPTY)) { rv = APR_SUCCESS; } - else if (rv == APR_SUCCESS && expect_empty) { - apr_off_t len = 0; - apr_brigade_length(conn->tmp_bb, 0, &len); - if (len) { - rv = APR_ENOTEMPTY; - } - } apr_brigade_cleanup(conn->tmp_bb); } else if (!ap_proxy_is_socket_connected(conn->sock)) { @@ -2744,7 +2737,7 @@ PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function, * ap_proxy_determine_connection(). */ char ssl_hostname[PROXY_WORKER_RFC1035_NAME_SIZE]; - if (rv == APR_NOTFOUND + if (rv == APR_EINVAL || !conn->ssl_hostname || PROXY_STRNCPY(ssl_hostname, conn->ssl_hostname)) { ssl_hostname[0] = '\0'; @@ -2752,14 +2745,13 @@ PROXY_DECLARE(apr_status_t) ap_proxy_check_backend(const char *proxy_function, socket_cleanup(conn); if (rv != APR_ENOTEMPTY) { - ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(00951) - "%s: backend socket is disconnected.", - proxy_function); + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, server, APLOGNO(00951) + "%s: backend socket is disconnected.", scheme); } else { - ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(03408) + ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, server, APLOGNO(03408) "%s: reusable backend connection is not empty: " - "forcibly closed", proxy_function); + "forcibly closed", scheme); } if (ssl_hostname[0]) { @@ -2785,8 +2777,8 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function, proxy_server_conf *conf = (proxy_server_conf *) ap_get_module_config(sconf, &proxy_module); - rv = ap_proxy_check_backend(proxy_function, conn, s, 0); - if (rv == APR_NOTFOUND) { + rv = ap_proxy_check_connection(proxy_function, conn, s, 0, 0); + if (rv == APR_EINVAL) { return DECLINED; } @@ -3002,7 +2994,7 @@ PROXY_DECLARE(int) ap_proxy_connect_backend(const char *proxy_function, if (rv == APR_SUCCESS) { socket_cleanup(conn); } - rv = APR_NOTFOUND; + rv = APR_EINVAL; } return rv == APR_SUCCESS ? OK : DECLINED;