You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2005/07/06 05:35:56 UTC
[Patch 1.3] Strict proxy C-L / T-E conformance
The attached patch restricts T-E responses to 'chunked', and
discards C-L in that case, setting the response to nocache
as a just-in-case.
It also adds a note in case anyone wants to use connection
keep-alive, warning of dire circumstances.
Comments/votes?
Bill
Re: Apache 2.2 (was 1.3) Strict proxy C-L / T-E conformance
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 08:35 AM 7/7/2005, Roy T. Fielding wrote:
>On Jul 5, 2005, at 8:56 PM, William A. Rowe, Jr. wrote:
>
>>Attached is the mystery patch [omitted from the last note - whoops].
>>
>>IMHO we should apply the same to ap_http_filter() in 2.1's
>>http_filters.c
>
>It looks like a band-aid to me -- how does this module know that
>the server doesn't support other transfer encodings? Wouldn't
>it be better to register a set of transfer encoding filters and
>then error if none matches? Oh, never mind, this is for 1.3. +0.
To that idea, for Apache 2.2, a huge ++1! I'll ensure we are
a bit more flexible for 2.1-dev.
As I think about it, we are wasting cycles injecting a filter
which keeps looking left and right to decide if it is handling
a C-L body or a T-E body. IMHO these need to become two different
filters, and the body filter would -only- be injected in the absence
of all other T-E options.
A registered T-E filter would stack (with 'chunked' at the lowest
layer of the stack). The order of stacking remains the only puzzle
to be solved.
Bill
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Jul 5, 2005, at 8:56 PM, William A. Rowe, Jr. wrote:
> Attached is the mystery patch [omitted from the last note - whoops].
>
> IMHO we should apply the same to ap_http_filter() in 2.1's
> http_filters.c
It looks like a band-aid to me -- how does this module know that
the server doesn't support other transfer encodings? Wouldn't
it be better to register a set of transfer encoding filters and
then error if none matches? Oh, never mind, this is for 1.3. +0.
....Roy
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:22 PM 7/6/2005, Joe Orton wrote:
>You should copy the logic used on the trunk to get the correct tests for
>a strtol parse error:
>
> errno || len_end == content_length || *len_end || c->len < 0
What is len_end == content_length? wouldn't *content_length
be clearer? And this test doesn't test for a null len_end, is
that safe?
Bill
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 12:09 PM 7/7/2005, Jim Jagielski wrote:
>This was, iirc, to handle cases where a strtol could possibly set it
>to NULL; someone, can't recall who, seemed to remember one implementation
>which did that, so we just figured to-hell-with-it and add a safety
>check, just in case :)
In httpd-1.3, src/ap/ap_strtol.c, don't we provide our own
strtol which we can trust?
Bill
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jul 06, 2005 at 02:53:52PM -0400, Jim Jagielski wrote:
>
> On Jul 6, 2005, at 2:22 PM, Joe Orton wrote:
>
> >On Wed, Jul 06, 2005 at 11:45:21AM -0500, William Rowe wrote:
> >...
> >
> >>+ else {
> >>+ char *len_end;
> >>+ errno = 0;
> >>+ c->len = ap_strtol(content_length, &len_end, 10);
> >>
> >...
> >
> >>+ if (errno || (c->len < 0) || (len_end &&
> >>*len_end)) {
> >>
> >
> >You should copy the logic used on the trunk to get the correct
> >tests for
> >a strtol parse error:
> >
> > errno || len_end == content_length || *len_end || c->len < 0
> >
>
> The len_end == content_length just means that there was no digits
> seen (possibly a blank)... not sure if we should consider that
> an error.
An empty string, right: I think it's certainly correct to treat that as
invalid C-L header; indeed some strtol's themselves set errno for that
case. (the perl-framework C-L tests picked up this inconsistency a
while back)
> That's why in all other places we've used the (len_end && *len_end)
> check, which ensures that
There is no case where strtol will set len_end = NULL so the first half
of the conditional is redundant. (also len_end was not NULL-initialized
so if it was an attempt to catch cases where strtol does *not* set
len_end, it was not correct ;)
> it's valid *and* that it's seen an invalid char. ap_strtol(" "...)
> returns 0 but isn't an "error" (though in this context, I see your
> point).
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jul 6, 2005, at 2:22 PM, Joe Orton wrote:
> On Wed, Jul 06, 2005 at 11:45:21AM -0500, William Rowe wrote:
> ...
>
>> + else {
>> + char *len_end;
>> + errno = 0;
>> + c->len = ap_strtol(content_length, &len_end, 10);
>>
> ...
>
>> + if (errno || (c->len < 0) || (len_end &&
>> *len_end)) {
>>
>
> You should copy the logic used on the trunk to get the correct
> tests for
> a strtol parse error:
>
> errno || len_end == content_length || *len_end || c->len < 0
>
The len_end == content_length just means that there was no digits
seen (possibly a blank)... not sure if we should consider that
an error. That's why in all other places we've used
the (len_end && *len_end) check, which ensures that
it's valid *and* that it's seen an invalid char. ap_strtol(" "...)
returns 0 but isn't an "error" (though in this context,
I see your point).
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Jul 06, 2005 at 11:45:21AM -0500, William Rowe wrote:
...
> + else {
> + char *len_end;
> + errno = 0;
> + c->len = ap_strtol(content_length, &len_end, 10);
...
> + if (errno || (c->len < 0) || (len_end && *len_end)) {
You should copy the logic used on the trunk to get the correct tests for
a strtol parse error:
errno || len_end == content_length || *len_end || c->len < 0
joe
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 08:12 AM 7/6/2005, Jim Jagielski wrote:
>On Jul 6, 2005, at 9:06 AM, Jim Jagielski wrote:
>
>>+ char *len_end;
>>+ c->len = ap_strtol(content_length, *len_end, 10);
>>+ if ((c->len < 0) || *len_end) {
>>
>>Oops... Should be:
>>
>> c->len = ap_strtol(content_length, &len_end, 10);
>
>You know, to be anal about it we should check for range errors; it
>should be:
>
> char *len_end;
> int errno = 0;
> ...
> if ((c->len < 0) || errno || (len_end && *len_end))
Agreed... revised patch attached.
The patch does leave me some questions, as I began looking
at Apache 2.1. Doing this in http_filter might be too late,
it seems that the http_header filter could be a better place.
Also, I'm not clear if mod_gzip can inject itself into the backend
proxy code, and whether or not mod_gzip supports T-E encoding or
only C-E, or if any other modules play with different T-E fields.
I don't have the sources handy.
But in 2.1, we allow filters to inject themselves in all sorts of
places. I'm unsure how early we want to reject T-E other than
the 'chunked' token.
Bill
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Jul 6, 2005, at 9:06 AM, Jim Jagielski wrote:
> + char *len_end;
> + c->len = ap_strtol(content_length, *len_end, 10);
> - if (c->len < 0) {
> - ap_kill_timeout(r);
> - return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
> - "Invalid Content-Length from remote server",
> - NULL));
> + if ((c->len < 0) || *len_end) {
>
>
> Oops... Should be:
>
> c->len = ap_strtol(content_length, &len_end, 10);
>
> and
>
> if ((c->len < 0) || (len_end && *len_end))
>
> I think.. :)
>
You know, to be anal about it we should check for range errors; it
should be:
char *len_end;
int errno = 0;
...
if ((c->len < 0) || errno || (len_end && *len_end))
Re: [Patch 1.3] Strict proxy C-L / T-E conformance
Posted by Jim Jagielski <ji...@jaguNET.com>.
+ char *len_end;
+ c->len = ap_strtol(content_length, *len_end, 10);
- if (c->len < 0) {
- ap_kill_timeout(r);
- return ap_proxyerror(r, HTTP_BAD_GATEWAY, ap_pstrcat(r->pool,
- "Invalid Content-Length from remote server",
- NULL));
+ if ((c->len < 0) || *len_end) {
Oops... Should be:
c->len = ap_strtol(content_length, &len_end, 10);
and
if ((c->len < 0) || (len_end && *len_end))
I think.. :)
[Patch 1.3] Strict proxy C-L / T-E conformance
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Attached is the mystery patch [omitted from the last note - whoops].
IMHO we should apply the same to ap_http_filter() in 2.1's
http_filters.c
Bill
At 10:35 PM 7/5/2005, William A. Rowe, Jr. wrote:
>The attached patch restricts T-E responses to 'chunked', and
>discards C-L in that case, setting the response to nocache
>as a just-in-case.
>
>It also adds a note in case anyone wants to use connection
>keep-alive, warning of dire circumstances.
>
>Comments/votes?
>
>Bill