]> granicus.if.org Git - apache/commitdiff
Merge r1783842 from trunk:
authorJacob Champion <jchampion@apache.org>
Fri, 24 Mar 2017 18:00:40 +0000 (18:00 +0000)
committerJacob Champion <jchampion@apache.org>
Fri, 24 Mar 2017 18:00:40 +0000 (18:00 +0000)
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
STATUS
modules/cache/cache_storage.c
modules/cache/cache_util.c
modules/cache/cache_util.h
modules/cache/mod_cache.c

diff --git a/CHANGES b/CHANGES
index b50928b35dedd26a4fc930af80ced42409b4d999..ca67dd5209e7eaa30e528f21a2b512eedaeb969b 100644 (file)
--- 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 2af49e22b1aca29a0faed397a46e3667b886eeb6..cf05b37f921d4e5a72f5699f446b66fb9dc4d752 100644 (file)
--- 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 ]
index 1a75cc1d814e3b895f27e1dda09ed9f8e9c3c6e9..41f638c025ccd8d612b67fe27cded4159706c33b 100644 (file)
@@ -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
index 096058308e037fa8f92be1ed31acdfa971383ae7..25f900e2d6e6e30798c0b4ff4ff90e54bae57704 100644 (file)
@@ -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:
      *   /<path>
@@ -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);
         }
     }
index 6b0174c7bde1b07622b37f54f125cd9b98291084..6b921510a1e300e57c63b9530e8a158461b639ee 100644 (file)
@@ -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
index b857e2ea0959972474faddcb440f07824e85598c..7982776ba8bf5e56369217832b0b714dd79b62f3 100644 (file)
@@ -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)