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 2000/12/14 06:20:37 UTC
RE: protocol/6973: http_protocol.c isn't compatible with HTTP RFC
> From: Tony Finch [mailto:dot@dotat.at]
> Sent: Wednesday, December 13, 2000 5:40 PM
>
> The following reply was made to PR protocol/6973; it has been
> noted by GNATS.
>
> From: Tony Finch <do...@dotat.at>
> To: Ory Segal <or...@sanctuminc.com>
> Cc: apbugs@apache.org
> Subject: Re: protocol/6973: http_protocol.c isn't compatible
> with HTTP RFC
> Date: Wed, 13 Dec 2000 23:37:23 +0000
>
> Actually, scrub that last email. This is a newly discovered problem.
> (I could have sworn I checked that line for sanity when dealing with
> 6711.)
>
> What do you think of this patch?
>
> Index: http_protocol.c
> ===================================================================
> RCS file: /home/cvs/apache-1.3/src/main/http_protocol.c,v
> retrieving revision 1.295
> diff -u -r1.295 http_protocol.c
> --- http_protocol.c 2000/11/14 09:57:09 1.295
> +++ http_protocol.c 2000/12/13 23:35:04
> @@ -151,6 +151,8 @@
> /* In the form "-5" */
> *start = clength - atol(dash + 1);
> *end = clength - 1;
> + if (*start == *end)
> + return 0;
> }
> else {
> *dash = '\0';
Two questions, a "-1" range from *start of 5 to *end of 5 is valid,
isn't it? Isn't this simply (*start > *end), which we already test
a bit later? Also, because of our leading dash test, the later test
at line 165 (if *start < 0) could be moved right into here, since
start can't be less than zero if (dash != range) (start is already
skipped for any leading '-' in the text.)
> @@ -170,8 +172,8 @@
>
> if (*start > *end)
> return 0;
> -
> - return (*start > 0 || *end < clength - 1);
> + else
> + return 1;
> }
>
> static int internal_byterange(int, long *, request_rec *, const char **, long *,
Consecutive '-' signs are caught by the test (start < end), this is good.
Couldn't the last four lines of your patch be condensed into this?
return (*start <= *end);
I'm a little concerned we aren't testing isdigit() on this stuff,
possibly by simply using strtol which returns provides the next
unparsable character. Of course, that's after we strchr for the
'-' range character.
Sorry this has proved such a pain in your side, Tony :-( Thanks for your efforts.
Bill
Re: protocol/6973: http_protocol.c isn't compatible with HTTP RFC
Posted by Tony Finch <do...@dotat.at>.
"William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
>
>> --- http_protocol.c 2000/11/14 09:57:09 1.295
>> +++ http_protocol.c 2000/12/13 23:35:04
>> @@ -151,6 +151,8 @@
>> /* In the form "-5" */
>> *start = clength - atol(dash + 1);
>> *end = clength - 1;
>> + if (*start == *end)
>> + return 0;
>> }
>> else {
>> *dash = '\0';
>
>Two questions, a "-1" range from *start of 5 to *end of 5 is valid,
>isn't it? Isn't this simply (*start > *end), which we already test
>a bit later?
Ah, yes. Thanks for spotting that. I was intending to catch a Range of
"-0", but I forgot that the end index is inclusive (I am too used to
end indices being exclusive).
>Also, because of our leading dash test, the later test
>at line 165 (if *start < 0) could be moved right into here, since
>start can't be less than zero if (dash != range) (start is already
>skipped for any leading '-' in the text.)
Yes, but equally it doesn't need to be moved so I prefer not to move it.
>> @@ -170,8 +172,8 @@
>>
>> if (*start > *end)
>> return 0;
>> -
>> - return (*start > 0 || *end < clength - 1);
>> + else
>> + return 1;
>> }
>>
>> static int internal_byterange(int, long *, request_rec *, const char **, long *,
>
>Consecutive '-' signs are caught by the test (start < end), this is good.
>Couldn't the last four lines of your patch be condensed into this?
>
> return (*start <= *end);
Yes, but IMO that is not an improvement.
>I'm a little concerned we aren't testing isdigit() on this stuff,
>possibly by simply using strtol which returns provides the next
>unparsable character.
Agreed. I can make that change if people think it is worthwhile.
>Sorry this has proved such a pain in your side, Tony :-( Thanks for your efforts.
Well, now that I have worked out the kink in my forehead from frowning
at RFC 2616 for 15 minutes I don't think any permanent damage has been
done :-)
Tony.
--
f.a.n.finch fanf@covalent.net dot@dotat.at
"Then they attacked a town. A small town, I'll admit.
But nevertheless a town of people. People who died."
RE: protocol/6973: http_protocol.c isn't compatible with HTTP RFC
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Haven't seen this pop up on new-httpd yet (which isn't a surprize, seeing
as how danged slow locus is :-(
Bill
> -----Original Message-----
> From: Tony Finch [mailto:dot@dotat.at]
> Sent: Thursday, December 14, 2000 12:40 PM
> To: William A. Rowe, Jr.
> Cc: new-httpd@apache.org
> Subject: Re: protocol/6973: http_protocol.c isn't compatible with HTTP
> RFC
>
>
> "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
> >
> > what did you come up with for this? Do we want to simply use your
> >quick fix ... do you want to roll a patch ... do you want
> someone else
> >to deal with this (can I hear a really loud "HELL YES!"?)
>
> I have a modified quick fix, which is BALGE. The 416 thing is only a
> SHOULD so not implementing it isn't a disaster.
>
> I don't know what others think, so I'm ccing new-httpd to get
> some feedback.
>
> Tony.
> --
> f.a.n.finch fanf@covalent.net dot@dotat.at
> "Perhaps on your way home you will pass someone in the dark,
> and you will never know it, for they will be from outer space."
>
Re: protocol/6973: http_protocol.c isn't compatible with HTTP RFC
Posted by Tony Finch <do...@dotat.at>.
"William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
>
> what did you come up with for this? Do we want to simply use your
>quick fix ... do you want to roll a patch ... do you want someone else
>to deal with this (can I hear a really loud "HELL YES!"?)
I have a modified quick fix, which is BALGE. The 416 thing is only a
SHOULD so not implementing it isn't a disaster.
I don't know what others think, so I'm ccing new-httpd to get some feedback.
Tony.
--
f.a.n.finch fanf@covalent.net dot@dotat.at
"Perhaps on your way home you will pass someone in the dark,
and you will never know it, for they will be from outer space."
RE: protocol/6973: http_protocol.c isn't compatible with HTTP RFC
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
> From: Tony Finch [mailto:dot@dotat.at]
> Sent: Thursday, December 14, 2000 12:16 AM
>
> "William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
> >
> >One more observation. If the code (existing or future)
> receives a range
> >1001-1000 we don't fail the request.
>
> Actually, in that case we do the right thing -- we ignore the header.
>
> >OTOH, we need to be careful here, since a request for 1000-1001 from
> >a 100 byte file would result in a 1000-99 style spec, which we should
> >dishonor, but not fail the request if there are valid requests.
>
> In that case we should generate a 406 error, which we don't
> currently do.
>
> I think the current boolean return value from parse_byterange is
> inadequate and it needs to become a three-way value: OK,
> ingore, and bad.
+1 (that is +1 to your proposal to add +1, 0, -1 :-)
Re: protocol/6973: http_protocol.c isn't compatible with HTTP RFC
Posted by Tony Finch <do...@dotat.at>.
"William A. Rowe, Jr." <wr...@rowe-clan.net> wrote:
>
>One more observation. If the code (existing or future) receives a range
>1001-1000 we don't fail the request.
Actually, in that case we do the right thing -- we ignore the header.
>OTOH, we need to be careful here, since a request for 1000-1001 from
>a 100 byte file would result in a 1000-99 style spec, which we should
>dishonor, but not fail the request if there are valid requests.
In that case we should generate a 406 error, which we don't currently do.
I think the current boolean return value from parse_byterange is
inadequate and it needs to become a three-way value: OK, ingore, and bad.
Tony.
--
f.a.n.finch fanf@covalent.net dot@dotat.at
"If I didn't see it with my own eyes I would never have believed it!"
RE: protocol/6973: http_protocol.c isn't compatible with HTTP RFC
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
One more observation. If the code (existing or future) receives a range
1001-1000 we don't fail the request. OTOH, we need to be careful here,
since a request for 1000-1001 from a 100 byte file would result in a
1000-99 style spec, which we should dishonor, but not fail the request
if there are valid requests.
I'd suggest that we fail -1 if start>end in the 'n-n' code block, and then
if start >= clength fail 0, and then move end no greater than clength - 1.
In that block, for the 'n-' case, we don't do the start>end test at all.
In the '-n' code block start, if start > clen - 1 then fail -1 (they used
a double '--' sequence), otherwise fixup start to no less than 0.
The entire block looks like:
char *dash=strchr(range, '-');
if (!dash) /* dashless request illegal */
return -1;
if (dash == range) { /* form "-5" */
*start = clength - atol(dash + 1)
if (*start > clength) /* form "--5" illegal */
return -1;
else if (start == clength) /* form "-0" meaningless */
return 0;
else if (*start < 0)
*start = 0;
*end = clength - 1;
}
else {
*(dash++) = '\0';
*start = atol(range);
if (*dash) { /* form "5-10" */
*end = atol(dash);
if (*start > *end) /* form "5-4" illegal, catches "5--5" */
return -1;
if (*end >= clength) /* req ends past end */
*end = clength - 1;
}
else /* form "5-" */
*end = clength - 1;
if (*start >= clength) /* req starts past end */
return 0;
}
return 1;
}
Does this make sense?
If this is right... then at 222 where we test (!parse_byterange())
we should either use internal_byterange() or be testing for
(parse_byterange()==1). At 305 we are even more messed up, although
the callers of internal_byterange() seem to have the concept, almost.
But at 245 we get really hosed, and don't continue parsing for valid
segments (e.g. 500-1000,1-50 should work on a 100 byte entity), and
don't track if we recieved at least one valid segment before returning.
Makes one's head spin ... no more patching for me tonight :-/
Bill
> -----Original Message-----
> From: William A. Rowe, Jr. [mailto:wrowe@rowe-clan.net]
> Sent: Wednesday, December 13, 2000 11:21 PM
> To: new-httpd@apache.org; 'Tony Finch'
> Subject: RE: protocol/6973: http_protocol.c isn't compatible with HTTP
> RFC
>
>
> > From: Tony Finch [mailto:dot@dotat.at]
> > Sent: Wednesday, December 13, 2000 5:40 PM
> >
> > The following reply was made to PR protocol/6973; it has been
> > noted by GNATS.
> >
> > From: Tony Finch <do...@dotat.at>
> > To: Ory Segal <or...@sanctuminc.com>
> > Cc: apbugs@apache.org
> > Subject: Re: protocol/6973: http_protocol.c isn't compatible
> > with HTTP RFC
> > Date: Wed, 13 Dec 2000 23:37:23 +0000
> >
> > Actually, scrub that last email. This is a newly
> discovered problem.
> > (I could have sworn I checked that line for sanity when
> dealing with
> > 6711.)
> >
> > What do you think of this patch?
> >
> > Index: http_protocol.c
> > ===================================================================
> > RCS file: /home/cvs/apache-1.3/src/main/http_protocol.c,v
> > retrieving revision 1.295
> > diff -u -r1.295 http_protocol.c
> > --- http_protocol.c 2000/11/14 09:57:09 1.295
> > +++ http_protocol.c 2000/12/13 23:35:04
> > @@ -151,6 +151,8 @@
> > /* In the form "-5" */
> > *start = clength - atol(dash + 1);
> > *end = clength - 1;
> > + if (*start == *end)
> > + return 0;
> > }
> > else {
> > *dash = '\0';
>
> Two questions, a "-1" range from *start of 5 to *end of 5 is valid,
> isn't it? Isn't this simply (*start > *end), which we already test
> a bit later? Also, because of our leading dash test, the later test
> at line 165 (if *start < 0) could be moved right into here, since
> start can't be less than zero if (dash != range) (start is already
> skipped for any leading '-' in the text.)
>
> > @@ -170,8 +172,8 @@
> >
> > if (*start > *end)
> > return 0;
> > -
> > - return (*start > 0 || *end < clength - 1);
> > + else
> > + return 1;
> > }
> >
> > static int internal_byterange(int, long *, request_rec *,
> const char **, long *,
>
> Consecutive '-' signs are caught by the test (start < end),
> this is good.
> Couldn't the last four lines of your patch be condensed into this?
>
> return (*start <= *end);
>
> I'm a little concerned we aren't testing isdigit() on this stuff,
> possibly by simply using strtol which returns provides the next
> unparsable character. Of course, that's after we strchr for the
> '-' range character.
>
> Sorry this has proved such a pain in your side, Tony :-(
> Thanks for your efforts.
>
> Bill
>