Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: domain of covener@gmail.com designates
 209.85.128.172 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20130528210217.AC7052388847@eris.apache.org>
References: <20130528210217.AC7052388847@eris.apache.org>
Date: Thu, 22 Aug 2013 09:28:40 -0400
Message-ID: 
 <CALK=YjN=Nu89dmEQuc4+_K3yzO8UQ1cx2pmrUdjWzwnwV4Mbqg@mail.gmail.com>
Subject: Re: svn commit: r1487118 - in /httpd/httpd/branches/2.4.x: ./ CHANGES
 STATUS modules/cache/mod_cache.c
From: Eric Covener <covener@gmail.com>
To: dev@httpd.apache.org
Content-Type: text/plain; charset=ISO-8859-1

On Tue, May 28, 2013 at 5:02 PM,  <minfrin@apache.org> wrote:
> Author: minfrin
> Date: Tue May 28 21:02:17 2013
> New Revision: 1487118
>
> URL: http://svn.apache.org/r1487118
> Log:
> mod_cache: Make sure that contradictory entity headers present in a 304
> Not Modified response are caught and cause the entity to be removed.
>
> trunk patch: http://svn.apache.org/r1479117
> 2.4.x patch: http://people.apache.org/~minfrin/httpd-mod_cache-304sanity.patch2.4.patch
>
> Submitted by: minfrin
> Reviewed by: jim, wrowe
>
> Modified:
>     httpd/httpd/branches/2.4.x/   (props changed)
>     httpd/httpd/branches/2.4.x/CHANGES
>     httpd/httpd/branches/2.4.x/STThjeyATUS
>     httpd/httpd/branches/2.4.x/modules/cache/mod_cache.c
>
> Propchange: httpd/httpd/branches/2.4.x/
> ------------------------------------------------------------------------------
>   Merged /httpd/httpd/trunk:r1479117
>
> Modified: httpd/httpd/branches/2.4.x/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/CHANGES?rev=1487118&r1=1487117&r2=1487118&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/CHANGES [utf-8] (original)
> +++ httpd/httpd/branches/2.4.x/CHANGES [utf-8] Tue May 28 21:02:17 2013
> @@ -2,6 +2,10 @@
>
>  Changes with Apache 2.4.5
>
> +  *) mod_cache: Make sure that contradictory entity headers present in a 304
> +     Not Modified response are caught and cause the entity to be removed.
> +     [Graham Leggett]
> +
>    *) mod_cache: Make sure Vary processing handles multivalued Vary headers and
>       multivalued headers referred to via Vary. [Graham Leggett]
>
>
> Modified: httpd/httpd/branches/2.4.x/STATUS
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/STATUS?rev=1487118&r1=1487117&r2=1487118&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/STATUS (original)
> +++ httpd/httpd/branches/2.4.x/STATUS Tue May 28 21:02:17 2013
> @@ -90,12 +90,6 @@ RELEASE SHOWSTOPPERS:
>  PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
>    [ start all new proposals below, under PATCHES PROPOSED. ]
>
> -    * mod_cache: Make sure that contradictory entity headers present in a 304
> -      Not Modified response are caught and cause the entity to be removed.
> -      trunk patch: http://svn.apache.org/r1479117
> -      2.4.x patch: http://people.apache.org/~minfrin/httpd-mod_cache-304sanity.patch2.4.patch
> -      +1: minfrin, jim, wrowe
> -
>      * mod_cache: Honour Cache-Control: no-store in a request.
>        trunk patch: http://svn.apache.org/r1479222
>        2.4.x patch: http://people.apache.org/~minfrin/httpd-mod_cache-nostore2.4.patch
>
> Modified: httpd/httpd/branches/2.4.x/modules/cache/mod_cache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/cache/mod_cache.c?rev=1487118&r1=1487117&r2=1487118&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/modules/cache/mod_cache.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/cache/mod_cache.c Tue May 28 21:02:17 2013
> @@ -743,6 +743,22 @@ static int cache_save_store(ap_filter_t
>      return rv;
>  }
>
> +/**
> + * Sanity check for 304 Not Modified responses, as per RFC2616 Section 10.3.5.
> + */
> +static const char *cache_header_cmp(apr_pool_t *pool, apr_table_t *left,
> +        apr_table_t *right, const char *key)
> +{
> +    const char *h1, *h2;
> +
> +    if ((h1 = cache_table_getm(pool, left, key))
> +            && (h2 = cache_table_getm(pool, right, key)) && (strcmp(h1, h2))) {
> +        return apr_pstrcat(pool, "contradiction: 304 Not Modified, but ", key,
> +                " modified", NULL);
> +    }
> +    return NULL;
> +}
> +
>  /*
>   * CACHE_SAVE filter
>   * ---------------
> @@ -776,7 +792,7 @@ static apr_status_t cache_save_filter(ap
>      apr_time_t exp, date, lastmod, now;
>      apr_off_t size = -1;
>      cache_info *info = NULL;
> -    char *reason;
> +    const char *reason;
>      apr_pool_t *p;
>      apr_bucket *e;
>      apr_table_t *headers;
> @@ -1063,6 +1079,56 @@ static apr_status_t cache_save_filter(ap
>          /* or we've been asked not to cache it above */
>          reason = "r->no_cache present";
>      }
> +    else if (r->status == HTTP_NOT_MODIFIED && cache->stale_handle) {
> +        apr_table_t *left = cache->stale_handle->resp_hdrs;
> +        apr_table_t *right = r->headers_out;
> +
> +        /* and lastly, contradiction checks for revalidated responses
> +         * as per RFC2616 Section 10.3.5
> +         */
> +        if (((reason = cache_header_cmp(r->pool, left, right, "Allow")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Content-Encoding")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Content-Language")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Content-Length")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Content-Location")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Content-MD5")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Content-Range")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Content-Type")))
> +                || ((reason = cache_header_cmp(r->pool, left, right, "Expires")))
> +                || ((reason = cache_header_cmp(r->pool, left, right, "ETag")))
> +                || ((reason = cache_header_cmp(r->pool, left, right,
> +                        "Last-Modified")))) {
> +            /* contradiction: 304 Not Modified, but entity header modified */


I stumbled on this contradiction/revalidating message today in 2.4.

Shouldn't Expires be omitted in this check since it applies to the
original entity?

   The response MUST include the following header fields:
...

      - Expires, Cache-Control, and/or Vary, if the field-value might
        differ from that sent in any previous response for the same
        variant


-- 
Eric Covener
covener@gmail.com