You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Paul J. Reder" <re...@remulak.net> on 2002/10/12 01:04:53 UTC
[Patch]: ap_cache_check_freshness 64 bit oddities
I have run into a problem where the cache code randomly decides that a
cached entry is stale or that the last modified date is some time in
the future. I tracked it back to the ap_cache_check_freshness code
which does a lot of checking of dates.
Some of this date checking code compares and assigns uncast numeric values
(such as -1) and the output of atoi() to the apr_time_t values (which are
long longs). The debugger showed me that only the bottom half of the
apr_time_t was being updated/compared.
I would like to fix the code in the following ways (if there are no
objections):
1) Replace the assignments/comparisons of 0 with APR_DATE_BAD
2) Have someone create a "#define APR_DATE_UNASSIGNED ((apr_time_t)-1)"
value in apr_date.h.
3) Replace the assignments/comparisons of -1 with APR_DATE_UNASSIGNED
4) Replace the atoi() calls with calls to apr_atoi64().
Please see the attached patch.
With no objections (and a litle help from someone who has APR commit
authority) I would like to commit this this weekend.
Thanks,
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Index: httpd-2.0/modules/experimental/cache_util.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/experimental/cache_util.c,v
retrieving revision 1.18
diff -u -r1.18 cache_util.c
--- httpd-2.0/modules/experimental/cache_util.c 26 Aug 2002 16:41:56 -0000 1.18
+++ httpd-2.0/modules/experimental/cache_util.c 11 Oct 2002 22:56:14 -0000
@@ -162,7 +162,7 @@
const char *cc_cresp, *cc_req, *pragma_cresp;
const char *agestr = NULL;
char *val;
- apr_time_t age_c = 0;
+ apr_time_t age_c = APR_DATE_BAD;
cache_info *info = &(cache->handle->cache_obj->info);
/*
@@ -202,7 +202,7 @@
/* TODO: pragma_cresp not being used? */
pragma_cresp = apr_table_get(r->headers_out, "Pragma");
if ((agestr = apr_table_get(r->headers_out, "Age"))) {
- age_c = atoi(agestr);
+ age_c = apr_atoi64(agestr);
}
/* calculate age of object */
@@ -210,53 +210,53 @@
/* extract s-maxage */
if (cc_cresp && ap_cache_liststr(cc_cresp, "s-maxage", &val))
- smaxage = atoi(val);
+ smaxage = apr_atoi64(val);
else
- smaxage = -1;
+ smaxage = APR_DATE_UNASSIGNED;
/* extract max-age from request */
if (cc_req && ap_cache_liststr(cc_req, "max-age", &val))
- maxage_req = atoi(val);
+ maxage_req = apr_atoi64(val);
else
- maxage_req = -1;
+ maxage_req = APR_DATE_UNASSIGNED;
/* extract max-age from response */
if (cc_cresp && ap_cache_liststr(cc_cresp, "max-age", &val))
- maxage_cresp = atoi(val);
+ maxage_cresp = apr_atoi64(val);
else
- maxage_cresp = -1;
+ maxage_cresp = APR_DATE_UNASSIGNED;
/*
* if both maxage request and response, the smaller one takes priority
*/
- if (-1 == maxage_req)
+ if (APR_DATE_UNASSIGNED == maxage_req)
maxage = maxage_cresp;
- else if (-1 == maxage_cresp)
+ else if (APR_DATE_UNASSIGNED == maxage_cresp)
maxage = maxage_req;
else
maxage = MIN(maxage_req, maxage_cresp);
/* extract max-stale */
if (cc_req && ap_cache_liststr(cc_req, "max-stale", &val))
- maxstale = atoi(val);
+ maxstale = apr_atoi64(val);
else
- maxstale = 0;
+ maxstale = APR_DATE_BAD;
/* extract min-fresh */
if (cc_req && ap_cache_liststr(cc_req, "min-fresh", &val))
- minfresh = atoi(val);
+ minfresh = apr_atoi64(val);
else
- minfresh = 0;
+ minfresh = APR_DATE_BAD;
/* override maxstale if must-revalidate or proxy-revalidate */
- if (maxstale && ((cc_cresp &&
+ if ((maxstale != APR_DATE_BAD) && ((cc_cresp &&
ap_cache_liststr(cc_cresp, "must-revalidate", NULL))
|| (cc_cresp && ap_cache_liststr(cc_cresp,
"proxy-revalidate", NULL))))
- maxstale = 0;
+ maxstale = APR_DATE_BAD;
/* handle expiration */
- if ((-1 < smaxage && age < (smaxage - minfresh)) ||
- (-1 < maxage && age < (maxage + maxstale - minfresh)) ||
+ if ((APR_DATE_UNASSIGNED < smaxage && age < (smaxage - minfresh)) ||
+ (APR_DATE_UNASSIGNED < maxage && age < (maxage + maxstale - minfresh)) ||
(info->expire != APR_DATE_BAD && age < (info->expire - info->date + maxstale - minfresh))) {
/* it's fresh darlings... */
/* set age header on response */
@@ -264,8 +264,8 @@
apr_psprintf(r->pool, "%lu", (unsigned long)age));
/* add warning if maxstale overrode freshness calculation */
- if (!((-1 < smaxage && age < smaxage) ||
- (-1 < maxage && age < maxage) ||
+ if (!((APR_DATE_UNASSIGNED < smaxage && age < smaxage) ||
+ (APR_DATE_UNASSIGNED < maxage && age < maxage) ||
(info->expire != APR_DATE_BAD && (info->expire - info->date) > age))) {
/* make sure we don't stomp on a previous warning */
apr_table_merge(r->headers_out, "Warning", "110 Response is stale");
Index: httpd-2.0/srclib/apr-util/include/apr_date.h
===================================================================
RCS file: /home/cvspublic/apr-util/include/apr_date.h,v
retrieving revision 1.8
diff -u -r1.8 apr_date.h
--- httpd-2.0/srclib/apr-util/include/apr_date.h 24 May 2002 02:35:02 -0000 1.8
+++ httpd-2.0/srclib/apr-util/include/apr_date.h 11 Oct 2002 22:56:16 -0000
@@ -83,6 +83,7 @@
#define APR_DATE_BAD ((apr_time_t)0)
+#define APR_DATE_UNASSIGNED ((apr_time_t)-1)
/**
* Compare a string to a mask
Re: [Patch]: ap_cache_check_freshness 64 bit oddities
Posted by "Roy T. Fielding" <fi...@apache.org>.
> At first glance, I think there's an even more fundamental problem:
> the code in ap_cache_check_freshness() appears to be mixing times
> measured in microseconds (the result of ap_cache_current_age())
> with times measured in seconds (everything that it gets from the
> HTTP header).
And does that surprise you? Probably not. Add one more to the
continuing saga of errors due to flagrant type name abuse.
....Roy
Re: [Patch]: ap_cache_check_freshness 64 bit oddities
Posted by Brian Pane <br...@cnet.com>.
On Fri, 2002-10-11 at 16:04, Paul J. Reder wrote:
> I have run into a problem where the cache code randomly decides that a
> cached entry is stale or that the last modified date is some time in
> the future. I tracked it back to the ap_cache_check_freshness code
> which does a lot of checking of dates.
>
> Some of this date checking code compares and assigns uncast numeric values
> (such as -1) and the output of atoi() to the apr_time_t values (which are
> long longs). The debugger showed me that only the bottom half of the
> apr_time_t was being updated/compared.
>
> I would like to fix the code in the following ways (if there are no
> objections):
> 1) Replace the assignments/comparisons of 0 with APR_DATE_BAD
> 2) Have someone create a "#define APR_DATE_UNASSIGNED ((apr_time_t)-1)"
> value in apr_date.h.
> 3) Replace the assignments/comparisons of -1 with APR_DATE_UNASSIGNED
> 4) Replace the atoi() calls with calls to apr_atoi64().
At first glance, I think there's an even more fundamental problem:
the code in ap_cache_check_freshness() appears to be mixing times
measured in microseconds (the result of ap_cache_current_age())
with times measured in seconds (everything that it gets from the
HTTP header).
Brian