From cee70bafd9b1775bef8bf0ea0d3d96fad862884b Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Fri, 24 Mar 2017 18:00:40 +0000 Subject: [PATCH] Merge r1783842 from trunk: mod_cache: Fix a regression in 2.4.25 for the forward proxy case by computing and using the same entity key according to when the cache checks, loads and saves the request. PR 60577. Submitted by: ylavic git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1788511 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 5 ++ STATUS | 7 --- modules/cache/cache_storage.c | 87 ++++++++++++++++++----------------- modules/cache/cache_util.c | 33 ++++++++++--- modules/cache/cache_util.h | 6 +++ modules/cache/mod_cache.c | 5 +- 6 files changed, 86 insertions(+), 57 deletions(-) diff --git a/CHANGES b/CHANGES index b50928b35d..ca67dd5209 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,11 @@ -*- coding: utf-8 -*- Changes with Apache 2.4.26 + + *) mod_cache: Fix a regression in 2.4.25 for the forward proxy case by + computing and using the same entity key according to when the cache + checks, loads and saves the request. + PR 60577. [Yann Ylavic] *) mod_proxy_hcheck: Ensure thread-safety when concurrent healthchecks are in use (ProxyHCTPsize > 0). PR 60071. [Yann Ylavic, Jim Jagielski] diff --git a/STATUS b/STATUS index 2af49e22b1..cf05b37f92 100644 --- a/STATUS +++ b/STATUS @@ -118,13 +118,6 @@ RELEASE SHOWSTOPPERS: PATCHES ACCEPTED TO BACKPORT FROM TRUNK: [ start all new proposals below, under PATCHES PROPOSED. ] - *) mod_cache: Fix a regression in 2.4.25 for the forward proxy case by - computing and using the same entity key according to when the cache - checks, loads and saves the request. PR 60577. - trunk patch: http://svn.apache.org/r1783842 - 2.4.x patch: trunk works (modulo CHANGES) - +1: ylavic, jim, covener - PATCHES PROPOSED TO BACKPORT FROM TRUNK: [ New proposals should be added at the end of the list ] diff --git a/modules/cache/cache_storage.c b/modules/cache/cache_storage.c index 1a75cc1d81..41f638c025 100644 --- a/modules/cache/cache_storage.c +++ b/modules/cache/cache_storage.c @@ -427,7 +427,7 @@ int cache_select(cache_request_rec *cache, request_rec *r) } static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, - const char *uri, const char *query, + const char *path, const char *query, apr_uri_t *parsed_uri, const char **key) { @@ -435,8 +435,8 @@ static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, char *port_str, *hn, *lcs; const char *hostname, *scheme; int i; - const char *path; - char *querystring; + const char *kpath; + const char *kquery; if (*key) { /* @@ -564,8 +564,8 @@ static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, * Check if we need to ignore session identifiers in the URL and do so * if needed. */ - path = uri; - querystring = apr_pstrdup(p, query ? query : parsed_uri->query); + kpath = path; + kquery = conf->ignorequerystring ? NULL : query; if (conf->ignore_session_id->nelts) { int i; char **identifier; @@ -580,24 +580,23 @@ static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, * Check that we have a parameter separator in the last segment * of the path and that the parameter matches our identifier */ - if ((param = ap_strrchr_c(path, ';')) + if ((param = ap_strrchr_c(kpath, ';')) && !strncmp(param + 1, *identifier, len) && (*(param + len + 1) == '=') && !ap_strchr_c(param + len + 2, '/')) { - path = apr_pstrmemdup(p, path, param - path); + kpath = apr_pstrmemdup(p, kpath, param - kpath); continue; } /* - * Check if the identifier is in the querystring and cut it out. + * Check if the identifier is in the query string and cut it out. */ - if (querystring && *querystring) { + if (kquery && *kquery) { /* * First check if the identifier is at the beginning of the - * querystring and followed by a '=' + * query string and followed by a '=' */ - if (!strncmp(querystring, *identifier, len) - && (*(querystring + len) == '=')) { - param = querystring; + if (!strncmp(kquery, *identifier, len) && kquery[len] == '=') { + param = kquery; } else { char *complete; @@ -607,7 +606,7 @@ static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, * identifier with a '&' and append a '=' */ complete = apr_pstrcat(p, "&", *identifier, "=", NULL); - param = ap_strstr_c(querystring, complete); + param = ap_strstr_c(kquery, complete); /* If we found something we are sitting on the '&' */ if (param) { param++; @@ -615,28 +614,28 @@ static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, } if (param) { const char *amp; + char *dup = NULL; - if (querystring != param) { - querystring = apr_pstrndup(p, querystring, - param - querystring); + if (kquery != param) { + dup = apr_pstrmemdup(p, kquery, param - kquery); + kquery = dup; } else { - querystring = ""; + kquery = ""; } if ((amp = ap_strchr_c(param + len + 1, '&'))) { - querystring = apr_pstrcat(p, querystring, amp + 1, - NULL); + kquery = apr_pstrcat(p, kquery, amp + 1, NULL); } else { /* - * If querystring is not "", then we have the case + * If query string is not "", then we have the case * that the identifier parameter we removed was the - * last one in the original querystring. Hence we have + * last one in the original query string. Hence we have * a trailing '&' which needs to be removed. */ - if (*querystring) { - querystring[strlen(querystring) - 1] = '\0'; + if (dup) { + dup[strlen(dup) - 1] = '\0'; } } } @@ -644,15 +643,11 @@ static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, } } - /* Key format is a URI, optionally without the query-string */ - if (conf->ignorequerystring) { - *key = apr_pstrcat(p, scheme, "://", hostname, port_str, path, "?", - NULL); - } - else { - *key = apr_pstrcat(p, scheme, "://", hostname, port_str, path, "?", - querystring, NULL); - } + /* Key format is a URI, optionally without the query-string (NULL + * per above if conf->ignorequerystring) + */ + *key = apr_pstrcat(p, scheme, "://", hostname, port_str, + kpath, "?", kquery, NULL); /* * Store the key in the request_config for the cache as r->parsed_uri @@ -662,20 +657,26 @@ static apr_status_t cache_canonicalise_key(request_rec *r, apr_pool_t* p, * resource in the cache under a key where it is never found by the quick * handler during following requests. */ - ap_log_rerror( - APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r, APLOGNO(00698) "cache: Key for entity %s?%s is %s", uri, parsed_uri->query, *key); + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r, APLOGNO(00698) + "cache: Key for entity %s?%s is %s", path, query, *key); return APR_SUCCESS; } apr_status_t cache_generate_key_default(request_rec *r, apr_pool_t* p, - const char **key) + const char **key) { - /* We want the actual query-string, which may differ from - * r->parsed_uri.query (immutable), so use "" (not NULL). + /* In early processing (quick-handler, forward proxy), we want the initial + * query-string from r->parsed_uri, since any change before CACHE_SAVE + * shouldn't modify the key. Otherwise we want the actual query-string. */ - const char *args = r->args ? r->args : ""; - return cache_canonicalise_key(r, p, r->uri, args, &r->parsed_uri, key); + const char *path = r->uri; + const char *query = r->args; + if (cache_use_early_url(r)) { + path = r->parsed_uri.path; + query = r->parsed_uri.query; + } + return cache_canonicalise_key(r, p, path, query, &r->parsed_uri, key); } /* @@ -717,7 +718,8 @@ int cache_invalidate(cache_request_rec *cache, request_rec *r) if (location) { if (apr_uri_parse(r->pool, location, &location_uri) || cache_canonicalise_key(r, r->pool, - location, NULL, + location_uri.path, + location_uri.query, &location_uri, &location_key) || !(r->parsed_uri.hostname && location_uri.hostname @@ -732,7 +734,8 @@ int cache_invalidate(cache_request_rec *cache, request_rec *r) if (apr_uri_parse(r->pool, content_location, &content_location_uri) || cache_canonicalise_key(r, r->pool, - content_location, NULL, + content_location_uri.path, + content_location_uri.query, &content_location_uri, &content_location_key) || !(r->parsed_uri.hostname diff --git a/modules/cache/cache_util.c b/modules/cache/cache_util.c index 096058308e..25f900e2d6 100644 --- a/modules/cache/cache_util.c +++ b/modules/cache/cache_util.c @@ -31,10 +31,8 @@ extern module AP_MODULE_DECLARE_DATA cache_module; * in "filter". All but the path comparisons are case-insensitive. */ static int uri_meets_conditions(const apr_uri_t *filter, const int pathlen, - request_rec *r) + const apr_uri_t *url, const char *path) { - const apr_uri_t *url = &r->parsed_uri; - /* Scheme, hostname port and local part. The filter URI and the * URI we test may have the following shapes: * / @@ -114,7 +112,7 @@ static int uri_meets_conditions(const apr_uri_t *filter, const int pathlen, /* For HTTP caching purposes, an empty (NULL) path is equivalent to * a single "/" path. RFCs 3986/2396 */ - if (!r->uri) { + if (!path) { if (*filter->path == '/' && pathlen == 1) { return 1; } @@ -126,7 +124,23 @@ static int uri_meets_conditions(const apr_uri_t *filter, const int pathlen, /* Url has met all of the filter conditions so far, determine * if the paths match. */ - return !strncmp(filter->path, r->uri, pathlen); + return !strncmp(filter->path, path, pathlen); +} + +int cache_use_early_url(request_rec *r) +{ + cache_server_conf *conf; + + if (r->proxyreq == PROXYREQ_PROXY) { + return 1; + } + + conf = ap_get_module_config(r->server->module_config, &cache_module); + if (conf->quick) { + return 1; + } + + return 0; } static cache_provider_list *get_provider(request_rec *r, struct cache_enable *ent, @@ -172,6 +186,7 @@ cache_provider_list *cache_get_providers(request_rec *r, { cache_dir_conf *dconf = ap_get_module_config(r->per_dir_config, &cache_module); cache_provider_list *providers = NULL; + const char *path; int i; /* per directory cache disable */ @@ -179,11 +194,14 @@ cache_provider_list *cache_get_providers(request_rec *r, return NULL; } + path = cache_use_early_url(r) ? r->parsed_uri.path : r->uri; + /* global cache disable */ for (i = 0; i < conf->cachedisable->nelts; i++) { struct cache_disable *ent = (struct cache_disable *)conf->cachedisable->elts; - if (uri_meets_conditions(&ent[i].url, ent[i].pathlen, r)) { + if (uri_meets_conditions(&ent[i].url, ent[i].pathlen, + &r->parsed_uri, path)) { /* Stop searching now. */ return NULL; } @@ -200,7 +218,8 @@ cache_provider_list *cache_get_providers(request_rec *r, for (i = 0; i < conf->cacheenable->nelts; i++) { struct cache_enable *ent = (struct cache_enable *)conf->cacheenable->elts; - if (uri_meets_conditions(&ent[i].url, ent[i].pathlen, r)) { + if (uri_meets_conditions(&ent[i].url, ent[i].pathlen, + &r->parsed_uri, path)) { providers = get_provider(r, &ent[i], providers); } } diff --git a/modules/cache/cache_util.h b/modules/cache/cache_util.h index 6b0174c7bd..6b921510a1 100644 --- a/modules/cache/cache_util.h +++ b/modules/cache/cache_util.h @@ -327,6 +327,12 @@ char *cache_strqtok(char *str, const char *sep, char **last); */ apr_table_t *cache_merge_headers_out(request_rec *r); +/** + * Return whether to use request's path/query from early stage (r->parsed_uri) + * or the current/rewritable ones (r->uri/r->args). + */ +int cache_use_early_url(request_rec *r); + #ifdef __cplusplus } #endif diff --git a/modules/cache/mod_cache.c b/modules/cache/mod_cache.c index b857e2ea09..7982776ba8 100644 --- a/modules/cache/mod_cache.c +++ b/modules/cache/mod_cache.c @@ -823,6 +823,7 @@ static apr_status_t cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in) apr_pool_t *p; apr_bucket *e; apr_table_t *headers; + const char *query; conf = (cache_server_conf *) ap_get_module_config(r->server->module_config, &cache_module); @@ -927,6 +928,8 @@ static apr_status_t cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in) } } + query = cache_use_early_url(r) ? r->parsed_uri.query : r->args; + /* read expiry date; if a bad date, then leave it so the client can * read it */ @@ -1057,7 +1060,7 @@ static apr_status_t cache_save_filter(ap_filter_t *f, apr_bucket_brigade *in) reason = "s-maxage or max-age zero and no Last-Modified or Etag; not cacheable"; } - else if (!conf->ignorequerystring && r->parsed_uri.query && exps == NULL + else if (!conf->ignorequerystring && query && exps == NULL && !control.max_age && !control.s_maxage) { /* if a query string is present but no explicit expiration time, * don't cache it (RFC 2616/13.9 & 13.2.1) -- 2.50.1