From b07149f7eaf48f30b53c18b9e72ff5ebe0da4e7e Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Mon, 6 Jul 2015 07:13:57 +0000 Subject: [PATCH] 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 --- CHANGES | 4 ++ modules/filters/mod_reqtimeout.c | 94 ++++++++++++++------------------ 2 files changed, 46 insertions(+), 52 deletions(-) diff --git a/CHANGES b/CHANGES index 3a21818c64..915a0e4ac4 100644 --- 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 , Yann Ylavic] diff --git a/modules/filters/mod_reqtimeout.c b/modules/filters/mod_reqtimeout.c index f4b38e22a4..bc7789964a 100644 --- 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; -- 2.50.1