Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Message-ID: <48B5AF90.5020409@apache.org>
Date: Wed, 27 Aug 2008 21:48:32 +0200
From: Ruediger Pluem <rpluem@apache.org>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.1.16) Gecko/20080702 SeaMonkey/1.1.11
MIME-Version: 1.0
To: dev@httpd.apache.org
Subject: Fix mod_expires to set correct expires date (PR41391)
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

While reviewing the patch for PR41391 (https://issues.apache.org/bugzilla/show_bug.cgi?id=41391),
I noticed that besides a possible overflow, it could happen that the expires date is in the past
(just take an old file and say modification plus 1 minute). What we really want to say in this
situation is that the resource is already expired. To state this RF2616 14.21 says that the
Expires header should be set to the same value as the Date header.
The following patch fixes this and ensures that max-age will be zero in this situation:

Index: modules/metadata/mod_expires.c
===================================================================
--- modules/metadata/mod_expires.c      (Revision 689502)
+++ modules/metadata/mod_expires.c      (Arbeitskopie)
@@ -386,6 +386,8 @@
      return new;
  }

+#define SECS_PER_YEAR 86400 * 365
+
  /*
   * Handle the setting of the expiration response header fields according
   * to our criteria.
@@ -395,7 +397,6 @@
                                   apr_table_t *t)
  {
      apr_time_t base;
-    apr_time_t additional;
      apr_time_t expires;
      int additional_sec;
      char *timestr;
@@ -409,16 +410,12 @@
              return DECLINED;
          }
          base = r->finfo.mtime;
-        additional_sec = atoi(&code[1]);
-        additional = apr_time_from_sec(additional_sec);
          break;
      case 'A':
          /* there's been some discussion and it's possible that
           * 'access time' will be stored in request structure
           */
          base = r->request_time;
-        additional_sec = atoi(&code[1]);
-        additional = apr_time_from_sec(additional_sec);
          break;
      default:
          /* expecting the add_* routines to be case-hardened this
@@ -429,7 +426,31 @@
          return HTTP_INTERNAL_SERVER_ERROR;
      }

-    expires = base + additional;
+    additional_sec = atoi(&code[1]);
+    expires = base + apr_time_from_sec(additional_sec);
+    /*
+     * Check if we have an overflow, so the resource should not expire.
+     * According to RFC2616 14.21 we should set a date one year in the future
+     * then, but not more.
+     */
+    if (expires < 0) {
+        expires = r->request_time + apr_time_from_sec(SECS_PER_YEAR);
+    }
+    /*
+     * We had an overflow again. So we seem to be less then a year from
+     * APR_INT64_MAX
+     */
+    if (expires < 0) {
+        expires = APR_INT64_MAX;
+    }
+    /*
+     * Our expires date is in the past. According to RFC2616 14.21 we should
+     * set expires to the same value as the Date header to show that the
+     * resource is already expired.
+     */
+    if (expires < r->request_time) {
+        expires = r->request_time;
+    }
      apr_table_mergen(t, "Cache-Control",
                       apr_psprintf(r->pool, "max-age=%" APR_TIME_T_FMT,
                                    apr_time_sec(expires - r->request_time)));


The downside is that this patch breaks several tests. But I believe that this is caused by a wrong
test. The following patch would fix this:

Index: t/modules/expires.t
===================================================================
--- t/modules/expires.t (Revision 689502)
+++ t/modules/expires.t (Arbeitskopie)
@@ -243,6 +243,17 @@
          $actual = $headers{expires} - $headers{access};
      }

+    ## if we use the modification date as base the calculated expiration date
+    ## might be past and the message that should be sent is that this resource
+    ## is already expired. According to RFC2616 14.21 the Date header and the
+    ## Expires header should be equal in this case.
+    ##
+    if (($exp_type eq 'M')
+        && ($headers{modified} + $expected < $headers{access})) {
+        $expected = 0;
+        $actual = $headers{expires} - $headers{access};
+    }
+
      print "# debug: expected: $expected\n";
      print "# debug: actual  : $actual\n";
      return ($actual == $expected);


Comments?

Regards

R�diger