Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
On Oct 24, 2007, at 9:37 PM, Jeff Trawick wrote: I vote for the small patch, and canl also throw in a spell check for that last comment above. Both look good and pass tests, but I also prefer, and vote +1 for, the small patch :)
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
Should have review cycles over the weekend...
On Oct 24, 2007, at 9:37 PM, Jeff Trawick wrote:
On 10/22/07, Jeff Trawick <[EMAIL PROTECTED]> wrote:
like 2.0/2.2/trunk
attached is an updated patch for the boil-the-ocean flavor; at the
bottom is a tiny alternative
some ways to slice through the big patch:
1. my BIG 1.3 patch vs. the 2.0 commit
essentially same changes except for
s/apr_date_parse_http(date)/ap_parseHTTPdate(date)/
s/apr_rfc822_date(ndate, time)/ap_gm_timestr_822(p, t)/
and the fact that 2.0.x previously made a copy of the input date
string before parsing
The old 1.3.x version, without the logic to make a copy of the input,
temporarily modified the input date string, but it always restored the
original contents before returning.
2. use of ap_proxy_date_canon() in 1.3.x vs. 2.0.x
proxy_http.c callers in 2.0.x are the same as proxy_http.c callers
in 1.3.x but
1.3.x also has callers in proxy_cache.c:
/* get the If-Modified-Since date of the request, if it exists */
c->ims = BAD_DATE;
datestr = ap_table_get(r->headers_in, "If-Modified-Since");
if (datestr != NULL) {
/* this may modify the value in the original table */
datestr = ap_proxy_date_canon(r->pool, datestr);
c->ims = ap_parseHTTPdate(datestr);
if (c->ims == BAD_DATE) /* bad or out of range date; remove
it */
ap_table_unset(r->headers_in, "If-Modified-Since");
}
/* get the If-Unmodified-Since date of the request, if it exists */
c->ius = BAD_DATE;
datestr = ap_table_get(r->headers_in, "If-Unmodified-Since");
if (datestr != NULL) {
/* this may modify the value in the original table */
datestr = ap_proxy_date_canon(r->pool, datestr);
c->ius = ap_parseHTTPdate(datestr);
if (c->ius == BAD_DATE) /* bad or out of range date; remove
it */
ap_table_unset(r->headers_in, "If-Unmodified-Since");
}
The comment "this may modify the value in the original table" is
perhaps alarming, but since ap_proxy_date_canon() restored the
original value, proxy_cache.c isn't relying on the modification as a
useful side-effect.
The call to ap_parseHTTPdate() right after ap_proxy_date_canon() is
more obviously wasteful now, since ap_parseHTTPdate() is the first
thing that ap_proxy_date_canon() calls, and the new string is
discarded.Thus the block for handling each date header can be
simplified to:
/* get the If-Modified-Since date of the request, if it exists and
is valid */
datestr = ap_table_get(r->headers_in, "If-Modified-Since");
if (datestr != NULL) {
c->ims = ap_parseHTTPdate(datestr);
if (c->ims == BAD_DATE) /* bad or out of range date; remove
it */
ap_table_unset(r->headers_in, "If-Modified-Since");
}
else {
c->ims = BAD_DATE;
}
The proxy_cache.c code could be left alone with its extra parsing and
formatting, but as it deserves testing anyway it is best to test the
simpler path than the more complex one.
3. apr_date_parse_http(date) vs. ap_parseHTTPdate(date)
apr_date_parse_http supports an additional RFC 1123 variation with
only one digit for day of month ("6 Oct 2007" instead of "06 Oct
2007"). The old ap_proxy_date_canon() didn't support either, so it
doesn't matter.
4. apr_rfc822_date(ndate, time) vs. ap_gm_timestr_822(p, t)
same format created
5. other comments
ap_parseHTTPdate() ignores the name of the day but old
ap_proxy_date_canon() verified it. ap_parseHTTPdate() doesn't
actually need the day name to compute the proper time.
The new version has some extra math in ap_gm_timestr_822()'s call to
gmtime() , and calls ap_psprintf() instead of ap_palloc(p, 30) +
ap_snprintf().
--/--
OTOH, a couple of strlen() calls in the original code would be a more
pragmatic solution.
Index: src/modules/proxy/proxy_util.c
===
--- src/modules/proxy/proxy_util.c (revision 588101)
+++ src/modules/proxy/proxy_util.c (working copy)
@@ -282,7 +282,8 @@
*q = ',';
if (wk == 7)
return x; /* not a valid date */
-if (q[4] != '-' || q[8] != '-' || q[11] != ' ' || q[14] !=
':' ||
+if (strlen(q) != 24 ||
+q[4] != '-' || q[8] != '-' || q[11] != ' ' || q[14] !=
':' ||
q[17] != ':' || strcmp(&q[20], " GMT") != 0)
return x;
if (sscanf(q + 2, "%u-%3s-%u %u:%u:%u %3s", &mday, month,
&year,
@@ -295,7 +296,8 @@
}
else {
/* check for acstime() date */
-if (x[3] != ' ' || x[7] != ' ' || x[10] != ' ' || x[13] !=
':' ||
+if (strlen(x) != 24 ||
+x[3] != ' ' || x[7] != ' ' || x[10] != ' ' || x[13] !=
':' ||
x[16] != ':' || x[19] != ' ' || x[24] != '\0')
return x;
if (sscanf(x, "%3s %3s %u %u:%u:%u %u", week, month,
&mday, &hour,
I vote for the small patch, and canl also throw in a spell check for
that last c
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
On 10/24/07, Jeff Trawick <[EMAIL PROTECTED]> wrote: > I vote for the small patch, and canl also throw in a spell check for > that last comment above. Couldn't find any problem in either the big or small patches, which I guess amounts to a vote for the small patch (although axing the body of that function sure looks appealing) -- Eric Covener [EMAIL PROTECTED]
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
On 10/22/07, Jeff Trawick <[EMAIL PROTECTED]> wrote:
> like 2.0/2.2/trunk
attached is an updated patch for the boil-the-ocean flavor; at the
bottom is a tiny alternative
some ways to slice through the big patch:
1. my BIG 1.3 patch vs. the 2.0 commit
essentially same changes except for
s/apr_date_parse_http(date)/ap_parseHTTPdate(date)/
s/apr_rfc822_date(ndate, time)/ap_gm_timestr_822(p, t)/
and the fact that 2.0.x previously made a copy of the input date
string before parsing
The old 1.3.x version, without the logic to make a copy of the input,
temporarily modified the input date string, but it always restored the
original contents before returning.
2. use of ap_proxy_date_canon() in 1.3.x vs. 2.0.x
proxy_http.c callers in 2.0.x are the same as proxy_http.c callers in 1.3.x but
1.3.x also has callers in proxy_cache.c:
/* get the If-Modified-Since date of the request, if it exists */
c->ims = BAD_DATE;
datestr = ap_table_get(r->headers_in, "If-Modified-Since");
if (datestr != NULL) {
/* this may modify the value in the original table */
datestr = ap_proxy_date_canon(r->pool, datestr);
c->ims = ap_parseHTTPdate(datestr);
if (c->ims == BAD_DATE) /* bad or out of range date; remove it */
ap_table_unset(r->headers_in, "If-Modified-Since");
}
/* get the If-Unmodified-Since date of the request, if it exists */
c->ius = BAD_DATE;
datestr = ap_table_get(r->headers_in, "If-Unmodified-Since");
if (datestr != NULL) {
/* this may modify the value in the original table */
datestr = ap_proxy_date_canon(r->pool, datestr);
c->ius = ap_parseHTTPdate(datestr);
if (c->ius == BAD_DATE) /* bad or out of range date; remove it */
ap_table_unset(r->headers_in, "If-Unmodified-Since");
}
The comment "this may modify the value in the original table" is
perhaps alarming, but since ap_proxy_date_canon() restored the
original value, proxy_cache.c isn't relying on the modification as a
useful side-effect.
The call to ap_parseHTTPdate() right after ap_proxy_date_canon() is
more obviously wasteful now, since ap_parseHTTPdate() is the first
thing that ap_proxy_date_canon() calls, and the new string is
discarded.Thus the block for handling each date header can be
simplified to:
/* get the If-Modified-Since date of the request, if it exists and
is valid */
datestr = ap_table_get(r->headers_in, "If-Modified-Since");
if (datestr != NULL) {
c->ims = ap_parseHTTPdate(datestr);
if (c->ims == BAD_DATE) /* bad or out of range date; remove it */
ap_table_unset(r->headers_in, "If-Modified-Since");
}
else {
c->ims = BAD_DATE;
}
The proxy_cache.c code could be left alone with its extra parsing and
formatting, but as it deserves testing anyway it is best to test the
simpler path than the more complex one.
3. apr_date_parse_http(date) vs. ap_parseHTTPdate(date)
apr_date_parse_http supports an additional RFC 1123 variation with
only one digit for day of month ("6 Oct 2007" instead of "06 Oct
2007"). The old ap_proxy_date_canon() didn't support either, so it
doesn't matter.
4. apr_rfc822_date(ndate, time) vs. ap_gm_timestr_822(p, t)
same format created
5. other comments
ap_parseHTTPdate() ignores the name of the day but old
ap_proxy_date_canon() verified it. ap_parseHTTPdate() doesn't
actually need the day name to compute the proper time.
The new version has some extra math in ap_gm_timestr_822()'s call to
gmtime() , and calls ap_psprintf() instead of ap_palloc(p, 30) +
ap_snprintf().
--/--
OTOH, a couple of strlen() calls in the original code would be a more
pragmatic solution.
Index: src/modules/proxy/proxy_util.c
===
--- src/modules/proxy/proxy_util.c (revision 588101)
+++ src/modules/proxy/proxy_util.c (working copy)
@@ -282,7 +282,8 @@
*q = ',';
if (wk == 7)
return x; /* not a valid date */
-if (q[4] != '-' || q[8] != '-' || q[11] != ' ' || q[14] != ':' ||
+if (strlen(q) != 24 ||
+q[4] != '-' || q[8] != '-' || q[11] != ' ' || q[14] != ':' ||
q[17] != ':' || strcmp(&q[20], " GMT") != 0)
return x;
if (sscanf(q + 2, "%u-%3s-%u %u:%u:%u %3s", &mday, month, &year,
@@ -295,7 +296,8 @@
}
else {
/* check for acstime() date */
-if (x[3] != ' ' || x[7] != ' ' || x[10] != ' ' || x[13] != ':' ||
+if (strlen(x) != 24 ||
+x[3] != ' ' || x[7] != ' ' || x[10] != ' ' || x[13] != ':' ||
x[16] != ':' || x[19] != ' ' || x[24] != '\0')
return x;
if (sscanf(x, "%3s %3s %u %u:%u:%u %u", week, month, &mday, &hour,
I vote for the small patch, and canl also throw in a spell check for
that last comment above.
Index: src/modules/proxy/proxy_util.c
===
--- src/m
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
On Oct 23, 2007, at 10:21 AM, Jeff Trawick wrote: On 10/23/07, Jim Jagielski <[EMAIL PROTECTED]> wrote: On Oct 22, 2007, at 7:20 PM, Jeff Trawick wrote: like 2.0/2.2/trunk I'm looking for a conceptual +1 out there. You got it. Conceptual +1 :) Thanks ;) Kind of silly I realize, but I wanted to make sure that if I spend more time on this then somebody is likely to read/review further. No prob... plus, not only read and review but vote :)
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
On 10/23/07, Jim Jagielski <[EMAIL PROTECTED]> wrote: > > On Oct 22, 2007, at 7:20 PM, Jeff Trawick wrote: > > > like 2.0/2.2/trunk > > > > I'm looking for a conceptual +1 out there. > > > > You got it. Conceptual +1 :) Thanks ;) Kind of silly I realize, but I wanted to make sure that if I spend more time on this then somebody is likely to read/review further.
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
On Oct 22, 2007, at 7:20 PM, Jeff Trawick wrote: like 2.0/2.2/trunk I'm looking for a conceptual +1 out there. You got it. Conceptual +1 :)
[PATCH] fix 1.3's ap_proxy_date_canon error handling
like 2.0/2.2/trunk
I'm looking for a conceptual +1 out there.
I've barely tested it and need to summarize the diffs between ap_ and
apr_ functions again. (e.g., "apr_date_parse_http supports format XXX
that ap_parseHTTPdate() doesn't support, but ap_proxy_date_canon()
didn't allow that before anyway so it doesn't regress...")
--
Born in Roswell... married an alien...
Index: src/modules/proxy/proxy_util.c
===
--- src/modules/proxy/proxy_util.c (revision 586541)
+++ src/modules/proxy/proxy_util.c (working copy)
@@ -267,61 +267,17 @@
* formatted, then it exits very quickly.
*/
const char *
- ap_proxy_date_canon(pool *p, const char *x)
+ ap_proxy_date_canon(pool *p, const char *date)
{
-int wk, mday, year, hour, min, sec, mon;
-char *q, month[4], zone[4], week[4];
+time_t t;
-q = strchr(x, ',');
-/* check for RFC 850 date */
-if (q != NULL && q - x > 3 && q[1] == ' ') {
-*q = '\0';
-for (wk = 0; wk < 7; wk++)
-if (strcmp(x, lwday[wk]) == 0)
-break;
-*q = ',';
-if (wk == 7)
-return x; /* not a valid date */
-if (q[4] != '-' || q[8] != '-' || q[11] != ' ' || q[14] != ':' ||
-q[17] != ':' || strcmp(&q[20], " GMT") != 0)
-return x;
-if (sscanf(q + 2, "%u-%3s-%u %u:%u:%u %3s", &mday, month, &year,
- &hour, &min, &sec, zone) != 7)
-return x;
-if (year < 70)
-year += 2000;
-else
-year += 1900;
+t = ap_parseHTTPdate(date);
+if (t == BAD_DATE) {
+return date;
}
-else {
-/* check for acstime() date */
-if (x[3] != ' ' || x[7] != ' ' || x[10] != ' ' || x[13] != ':' ||
-x[16] != ':' || x[19] != ' ' || x[24] != '\0')
-return x;
-if (sscanf(x, "%3s %3s %u %u:%u:%u %u", week, month, &mday, &hour,
- &min, &sec, &year) != 7)
-return x;
-for (wk = 0; wk < 7; wk++)
-if (strcmp(week, ap_day_snames[wk]) == 0)
-break;
-if (wk == 7)
-return x;
-}
-
-/* check date */
-for (mon = 0; mon < 12; mon++)
-if (strcmp(month, ap_month_snames[mon]) == 0)
-break;
-if (mon == 12)
-return x;
-
-q = ap_palloc(p, 30);
-ap_snprintf(q, 30, "%s, %.2d %s %d %.2d:%.2d:%.2d GMT", ap_day_snames[wk],
mday,
-ap_month_snames[mon], year, hour, min, sec);
-return q;
+return ap_gm_timestr_822(p, t);
}
-
/*
* Reads headers from a buffer and returns an array of headers.
* Returns NULL on file error
