httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bart van der Schans <sch...@hippo.nl>
Subject Re: Mod_cache expires check
Date Thu, 18 Jan 2007 00:20:02 GMT
Roy T. Fielding wrote:
> On Jan 17, 2007, at 12:23 PM, Ruediger Pluem wrote:
>> On 01/15/2007 01:56 PM, Bart van der Schans wrote:
>>> In r463496 the following check was added to mod_cache.c :
>>>
>>>     else if (exp != APR_DATE_BAD && exp < r->request_time)
>>>     {
>>>         /* if a Expires header is in the past, don't cache it */
>>>         reason = "Expires header already expired, not cacheable";
>>>     }
>>>
>>> This check fails to correctly identify the expires header "Thu, 01 Jan
>>> 1970 00:00:00 GMT". The apr_date_parse_http function(exps) returns
>>> (apr_time_t)0 which is equal to APR_DATE_BAD, but it should recognize it
>>> as an already expired header. Is there a way to distinct between
>>> APR_DATE_BAD and the unix epoch? Or is that considered a bad date?
>>
>> I would say 0 is not a bad day. But if this is a bug it is an 
>> APR(-util) bug.
>> Thus I forward it to the apr dev list.
> 
> No, it is a bug in the expression.  A date is an unsigned value and any
> error is specifically forced to 0 so that the comparison
> 
>    if (exp < r->request_time)
> 
> will be true if exp == APR_DATE_BAD (as is desired in this case).
That expression would also return true if the expires header is not set, 
which is probably not desired.

As I understand it, there are four cases:
a - Expires not set
b - Expires set to a bad (unparsable) date
c - Expires set to a date in the past (from and including 0)
d - Expires set to the future (up to +1 year)

According to rfc 2616 14.21, b and c should be treated as the same, but 
mod_cache treats a and b as the same:

exps = apr_table_get(r->err_headers_out, "Expires");
if (exps == NULL) {
     exps = apr_table_get(r->headers_out, "Expires");
}
if (exps != NULL) {
    if (APR_DATE_BAD == (exp = apr_date_parse_http(exps))) {
        exps = NULL;
    }
}
else {
    exp = APR_DATE_BAD;
}


If exp would be set to something like NOT_SET when the expires header is 
not set (in the else block) the check later on could be:

     else if (exp != NOT_SET && exp < r->request_time)
     {
         /* if a Expires header is in the past or bad, don't cache it */
         reason = "Expires header already expired or bad, not cacheable";
     }

It would seem obvious to set exp to NULL instead of NOT_SET, but NULL == 
APR_DATE_BAD evaluates to true (at least on my linux boxes) which would 
make it hard to discern later on between case a. and b.

Regards,

Bart

Mime
View raw message