mod_reqtimeout: follow up to r1621453.

Don't let pipelining checks and keep-alive times interfere
with the timeouts computed for subsequent requests.  PR 56729.

With pipelined requests, the log_transaction hook is called when the request
is destroyed, which may happen after a subsequent request is to be handled on
the same connection.
Move the initialization of the "header" state into pre_read_request() instead,
and get rid of the racy log_trasaction hook.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1689325 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 3a21818c64cf7b7f95038b8fed19f50f72a18e68..915a0e4ac4da8956073fe96b62a8bc9dce5a63e3 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,10 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) mod_reqtimeout: Don't let pipelining checks and keep-alive times interfere
+     with the timeouts computed for subsequent requests.  PR 56729.
+     [Eric Covener, Yann Ylavic]
+
   *) mod_authz_dbd: Avoid a crash when lacking correct DB access permissions.
      PR 57868. [Jose Kahan <jose w3.org>, Yann Ylavic]
 

diff --git a/modules/filters/mod_reqtimeout.c b/modules/filters/mod_reqtimeout.c
index f4b38e22a490ac9dd3c5b54284a0175e2907a8ee..bc7789964ad926e02684f4cb2b92bfbdb339fa67 100644 (file)
--- a/modules/filters/mod_reqtimeout.c
+++ b/modules/filters/mod_reqtimeout.c
@@ -177,22 +177,29 @@ static apr_status_t reqtimeout_filter(ap_filter_t *f,
     apr_interval_time_t saved_sock_timeout = UNSET;
     reqtimeout_con_cfg *ccfg = f->ctx;
 
-    if (ccfg->in_keep_alive) {
-        /* For this read, the normal keep-alive timeout must be used */
-        ccfg->in_keep_alive = 0;
-        return ap_get_brigade(f->next, bb, mode, block, readbytes);
-    }
-
     if (block == APR_NONBLOCK_READ && mode == AP_MODE_SPECULATIVE) { 
         /*  The source of these above us in the core is check_pipeline(), which
          *  is between requests but before this filter knows to reset timeouts 
-         *  during log_transaction().  If they appear elsewhere, just don't 
+         *  during pre_read_request().  If they appear elsewhere, just don't 
          *  check or extend the time since they won't block and we'll see the
          *  bytes again later
          */
         return ap_get_brigade(f->next, bb, mode, block, readbytes);
     }
 
+    if (ccfg->in_keep_alive) {
+        /* For this read[_request line()], wait for the first byte using the
+         * normal keep-alive timeout (hence don't take this expected idle time
+         * into account to setup the connection expiry below).
+         */
+        ccfg->in_keep_alive = 0;
+        rv = ap_get_brigade(f->next, bb, AP_MODE_SPECULATIVE, block, 1);
+        if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
+            return rv;
+        }
+        apr_brigade_cleanup(bb);
+    }
+
     if (ccfg->new_timeout > 0) {
         /* set new timeout */
         now = apr_time_now();
@@ -364,11 +371,32 @@ static int reqtimeout_init(conn_rec *c)
         ap_set_module_config(c->conn_config, &reqtimeout_module, ccfg);
         ap_add_input_filter(reqtimeout_filter_name, ccfg, NULL, c);
     }
-    else {
-        /* subsequent request under event-like MPM */
-        memset(ccfg, 0, sizeof(reqtimeout_con_cfg));
+
+    /* we are not handling the connection, we just do initialization */
+    return DECLINED;
+}
+
+static void reqtimeout_before_header(request_rec *r, conn_rec *c)
+{
+    reqtimeout_srv_cfg *cfg;
+    reqtimeout_con_cfg *ccfg =
+        ap_get_module_config(c->conn_config, &reqtimeout_module);
+
+    if (ccfg == NULL) {
+        /* not configured for this connection */
+        return;
     }
 
+    cfg = ap_get_module_config(c->base_server->module_config,
+                               &reqtimeout_module);
+    AP_DEBUG_ASSERT(cfg != NULL);
+
+    /* (Re)set the state for this new request, but ccfg->socket and
+     * ccfg->tmpbb which have the lifetime of the connection.
+     */
+    ccfg->timeout_at = 0;
+    ccfg->max_timeout_at = 0;
+    ccfg->in_keep_alive = (c->keepalives > 0);
     ccfg->type = "header";
     if (cfg->header_timeout != UNSET) {
         ccfg->new_timeout     = cfg->header_timeout;
@@ -382,12 +410,9 @@ static int reqtimeout_init(conn_rec *c)
         ccfg->min_rate        = MRT_DEFAULT_HEADER_MIN_RATE;
         ccfg->rate_factor     = default_header_rate_factor;
     }
-
-    /* we are not handling the connection, we just do initialization */
-    return DECLINED;
 }
 
-static int reqtimeout_after_headers(request_rec *r)
+static int reqtimeout_before_body(request_rec *r)
 {
     reqtimeout_srv_cfg *cfg;
     reqtimeout_con_cfg *ccfg =
@@ -419,41 +444,6 @@ static int reqtimeout_after_headers(request_rec *r)
     return OK;
 }
 
-static int reqtimeout_after_body(request_rec *r)
-{
-    reqtimeout_srv_cfg *cfg;
-    reqtimeout_con_cfg *ccfg =
-        ap_get_module_config(r->connection->conn_config, &reqtimeout_module);
-
-    if (ccfg == NULL) {
-        /* not configured for this connection */
-        return OK;
-    }
-
-    cfg = ap_get_module_config(r->connection->base_server->module_config,
-                               &reqtimeout_module);
-    AP_DEBUG_ASSERT(cfg != NULL);
-
-    ccfg->timeout_at = 0;
-    ccfg->max_timeout_at = 0;
-    ccfg->in_keep_alive = 1;
-    ccfg->type = "header";
-    if (ccfg->new_timeout != UNSET) {
-        ccfg->new_timeout     = cfg->header_timeout;
-        ccfg->new_max_timeout = cfg->header_max_timeout;
-        ccfg->min_rate        = cfg->header_min_rate;
-        ccfg->rate_factor     = cfg->header_rate_factor;
-    }
-    else {
-        ccfg->new_timeout     = MRT_DEFAULT_HEADER_TIMEOUT;
-        ccfg->new_max_timeout = MRT_DEFAULT_HEADER_MAX_TIMEOUT;
-        ccfg->min_rate        = MRT_DEFAULT_HEADER_MIN_RATE;
-        ccfg->rate_factor     = default_header_rate_factor;
-    }
-
-    return OK;
-}
-
 static void *reqtimeout_create_srv_config(apr_pool_t *p, server_rec *s)
 {
     reqtimeout_srv_cfg *cfg = apr_pcalloc(p, sizeof(reqtimeout_srv_cfg));
@@ -624,10 +614,10 @@ static void reqtimeout_hooks(apr_pool_t *pool)
      */
     ap_hook_process_connection(reqtimeout_init, NULL, NULL, APR_HOOK_LAST);
 
-    ap_hook_post_read_request(reqtimeout_after_headers, NULL, NULL,
+    ap_hook_pre_read_request(reqtimeout_before_header, NULL, NULL,
+                             APR_HOOK_MIDDLE);
+    ap_hook_post_read_request(reqtimeout_before_body, NULL, NULL,
                               APR_HOOK_MIDDLE);
-    ap_hook_log_transaction(reqtimeout_after_body, NULL, NULL,
-                            APR_HOOK_MIDDLE);
 
 #if MRT_DEFAULT_HEADER_MIN_RATE > 0
     default_header_rate_factor = apr_time_from_sec(1) / MRT_DEFAULT_HEADER_MIN_RATE;