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.
 


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

CHANGES

@@ -1,6 +1,11 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.0
 
+  *) 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_http2: support for ProxyPreserverHost directive. [Stefan Eissing]
   
   *) mod_proxy_fcgi: Add ProxyFCGISetEnvIf to fixup CGI environment

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;
+    const char *kpath;
-    char *querystring;
+    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;
+    kpath = path;
-    querystring = apr_pstrdup(p, query ? query : parsed_uri->query);
+    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)
+                if (!strncmp(kquery, *identifier, len) && kquery[len] == '=') {
-                        && (*(querystring + len) == '=')) {
+                    param = kquery;
-                    param = querystring;
                 }
                 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) {
+                    if (kquery != param) {
-                        querystring = apr_pstrndup(p, querystring,
+                        dup = apr_pstrmemdup(p, kquery, param - kquery);
-                                param - querystring);
+                        kquery = dup;
                     }
                     else {
-                        querystring = "";
+                        kquery = "";
                     }
 
                     if ((amp = ap_strchr_c(param + len + 1, '&'))) {
-                        querystring = apr_pstrcat(p, querystring, amp + 1,
+                        kquery = apr_pstrcat(p, kquery, amp + 1, NULL);
-                                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) {
+                        if (dup) {
-                            querystring[strlen(querystring) - 1] = '\0';
+                            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 */
+    /* Key format is a URI, optionally without the query-string (NULL
-    if (conf->ignorequerystring) {
+     * per above if conf->ignorequerystring)
-        *key = apr_pstrcat(p, scheme, "://", hostname, port_str, path, "?",
+     */
-                NULL);
+    *key = apr_pstrcat(p, scheme, "://", hostname, port_str,
-    }
+                       kpath, "?", kquery, NULL);
-    else {
-        *key = apr_pstrcat(p, scheme, "://", hostname, port_str, path, "?",
-                querystring, 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(
+    ap_log_rerror(APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r, APLOGNO(00698)
-            APLOG_MARK, APLOG_DEBUG, APR_SUCCESS, r, APLOGNO(00698) "cache: Key for entity %s?%s is %s", uri, parsed_uri->query, *key);
+                  "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
+    /* In early processing (quick-handler, forward proxy), we want the initial
-     * r->parsed_uri.query (immutable), so use "" (not NULL).
+     * 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 : "";
+    const char *path = r->uri;
-    return cache_canonicalise_key(r, p, r->uri, args, &r->parsed_uri, key);
+    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

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:
      *   /<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);
         }
     }

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

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)