From: Ruediger Pluem Date: Sat, 15 Dec 2007 16:15:04 +0000 (+0000) Subject: * Fix a SEGFAULT by ensuring that buckets that may have been buffered in the X-Git-Tag: 2.3.0~1143 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=d2a1cf5f8c90b34a05dcf8c5261570e6d6ff7712;p=apache * Fix a SEGFAULT by ensuring that buckets that may have been buffered in the network filters get flushed to the network. This is needed since these buckets have been created with the bucket allocator of the backend connection. This allocator either gets destroyed if conn->close is set or the worker address is not reusable which causes the connection to the backend to be closed or it will be used again by another frontend connection that wants to recycle the backend connection. In this case we could run into nasty race conditions (e.g. if the next user of the backend connection destroys the allocator before we sent the buckets to the network). Remark 1: Doing a setaside does not help here as the buckets remain created by the wrong allocator in this case. Remark 2: Yes, this creates a possible performance penalty in the case of pipelined requests as we may send only a small amount of data over the wire. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@604447 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/include/ap_mmn.h b/include/ap_mmn.h index 2bfe1fc224..ad19c22745 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -145,6 +145,7 @@ * with non-NULL request_rec pointer (ap_add_*_filter*) * 20071108.4 (2.3.0-dev) Add ap_proxy_ssl_connection_cleanup * 20071108.5 (2.3.0-dev) Add *scpool to proxy_conn_rec structure + * 20071108.6 (2.3.0-dev) Add *r and need_flush to proxy_conn_rec structure */ #define MODULE_MAGIC_COOKIE 0x41503234UL /* "AP24" */ @@ -152,7 +153,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20071108 #endif -#define MODULE_MAGIC_NUMBER_MINOR 5 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 6 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h index 84c6b261c7..9a6a6eb10b 100644 --- a/modules/proxy/mod_proxy.h +++ b/modules/proxy/mod_proxy.h @@ -241,6 +241,10 @@ typedef struct { int inreslist; /* connection in apr_reslist? */ #endif apr_pool_t *scpool; /* Subpool used for socket and connection data */ + request_rec *r; /* Request record of the frontend request + * which the backend currently answers. */ + int need_flush;/* Flag to decide whether we need to flush the + * filter chain or not */ } proxy_conn_rec; typedef struct { diff --git a/modules/proxy/mod_proxy_ftp.c b/modules/proxy/mod_proxy_ftp.c index f7e951d280..6d4c63a8c4 100644 --- a/modules/proxy/mod_proxy_ftp.c +++ b/modules/proxy/mod_proxy_ftp.c @@ -959,6 +959,7 @@ static int proxy_ftp_handler(request_rec *r, proxy_worker *worker, } /* TODO: see if ftp could use determine_connection */ backend->addr = connect_addr; + backend->r = r; ap_set_module_config(c->conn_config, &proxy_ftp_module, backend); } diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c index 92eb8f34c0..e6e2e5ef3b 100644 --- a/modules/proxy/proxy_util.c +++ b/modules/proxy/proxy_util.c @@ -1576,6 +1576,9 @@ static apr_status_t connection_cleanup(void *theconn) { proxy_conn_rec *conn = (proxy_conn_rec *)theconn; proxy_worker *worker = conn->worker; + apr_bucket_brigade *bb; + conn_rec *c; + request_rec *r; /* * If the connection pool is NULL the worker @@ -1596,8 +1599,48 @@ static apr_status_t connection_cleanup(void *theconn) } #endif + r = conn->r; + if (conn->need_flush && r) { + /* + * We need to ensure that buckets that may have been buffered in the + * network filters get flushed to the network. This is needed since + * these buckets have been created with the bucket allocator of the + * backend connection. This allocator either gets destroyed if + * conn->close is set or the worker address is not reusable which + * causes the connection to the backend to be closed or it will be used + * again by another frontend connection that wants to recycle the + * backend connection. + * In this case we could run into nasty race conditions (e.g. if the + * next user of the backend connection destroys the allocator before we + * sent the buckets to the network). + * + * Remark 1: Doing a setaside does not help here as the buckets remain + * created by the wrong allocator in this case. + * + * Remark 2: Yes, this creates a possible performance penalty in the case + * of pipelined requests as we may send only a small amount of data over + * the wire. + */ + c = r->connection; + bb = apr_brigade_create(r->pool, c->bucket_alloc); + if (r->eos_sent) { + /* + * If we have already sent an EOS bucket send directly to the + * connection based filters. We just want to flush the buckets + * if something hasn't been sent to the network yet. + */ + ap_fflush(c->output_filters, bb); + } + else { + ap_fflush(r->output_filters, bb); + } + apr_brigade_destroy(bb); + conn->r = NULL; + conn->need_flush = 0; + } + /* determine if the connection need to be closed */ - if (conn->close) { + if (conn->close || !worker->is_address_reusable) { apr_pool_t *p = conn->pool; apr_pool_clear(p); memset(conn, 0, sizeof(proxy_conn_rec)); @@ -1969,6 +2012,8 @@ ap_proxy_determine_connection(apr_pool_t *p, request_rec *r, apr_status_t err = APR_SUCCESS; apr_status_t uerr = APR_SUCCESS; + conn->r = r; + /* * Break up the URL to determine the host to connect to */ @@ -2287,6 +2332,12 @@ PROXY_DECLARE(int) ap_proxy_connection_create(const char *proxy_function, return OK; } + /* + * We need to flush the buckets before we return the connection to the + * connection pool. See comment in connection_cleanup for why this is + * needed. + */ + conn->need_flush = 1; bucket_alloc = apr_bucket_alloc_create(conn->scpool); /* * The socket is now open, create a new backend server connection