You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@gmail.com> on 2007/10/23 01:20:05 UTC
[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...
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 23, 2007, at 10:21 AM, Jeff Trawick wrote:
> On 10/23/07, Jim Jagielski <ji...@jagunet.com> 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
Posted by Jeff Trawick <tr...@gmail.com>.
On 10/23/07, Jim Jagielski <ji...@jagunet.com> 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
Posted by Jim Jagielski <ji...@jaguNET.com>.
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 :)
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
Posted by Jim Jagielski <ji...@jaguNET.com>.
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.
> <proxydate_13-v2.txt>
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
Posted by Eric Covener <co...@gmail.com>.
On 10/24/07, Jeff Trawick <tr...@gmail.com> 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
covener@gmail.com
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
Posted by Jim Jagielski <ji...@jaguNET.com>.
Should have review cycles over the weekend...
On Oct 24, 2007, at 9:37 PM, Jeff Trawick wrote:
> On 10/22/07, Jeff Trawick <tr...@gmail.com> 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.
> <proxydate_13-v2.txt>
Re: [PATCH] fix 1.3's ap_proxy_date_canon error handling
Posted by Jeff Trawick <tr...@gmail.com>.
On 10/22/07, Jeff Trawick <tr...@gmail.com> 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.