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
|