Re: Empty Reason Phrase (BZ 44995/45092)
Jeff Trawick wrote: On Tue, May 27, 2008 at 3:29 PM, Rainer Jung [EMAIL PROTECTED] wrote: Jeff Trawick schrieb: On Tue, May 20, 2008 at 3:27 PM, Ruediger Pluem [EMAIL PROTECTED] wrote: On 05/20/2008 08:52 PM, Jeff Trawick wrote: Do we really need to require the space in the case that the reason phrase is empty? No... Both sets of code should be changed to consider the space optional. We just need to make sure we generate the space in our response so some other software doesn't have to be so generous. I attached two patches against trunk to BZ 44995, one for the status_line validation in http_filters.c and one for the test when used in error pages in http_protocol.c. Backports should be straighforward, because the code didn't change locally. I didn't yet test thoroughly, but if you like them in principle I'll go through it. re: https://issues.apache.org/bugzilla/attachment.cgi?id=22019 I don't think r-status_line should be updated by this function (ap_send_error_response()). OK, so there are two cases which need clarification w.r.t. ap_send_error_response() in http_protocol.c: 1) status_line =NNN 3 digit status code without trailing space This would be non-conforming to RFC 2616. We correct it in validate_status_line() in http_filters.c but I neither know, if there is an order between ap_send_error_response() and validate_status_line(), nor if the status line could change in between. If we use the old behaviour, we simply switch to the standard 500 status line if we detect NNN in ap_send_error_response(). Anyone knowing more about the order of processing between ap_send_error_response() and validate_status_line() (which is called by basic_http_header_check(), which in turn is used inside ap_basic_http_header() and ap_http_header_filter())? Even if we correct the status_line here (adding a space), we still have to decide on the second case below. 2) status_line =NNN 3 digit status code with trailing space but no following reason phrase What should be used as a header in the error page body. Usually we use everything behind the space, which is an emptry string here. Before the patch we would switch to the standard status 500, although the status line was valid. Should we use something like Unknown Reason in the error page headline but keep status_line as is (NNN )? In this case I would also suggest to set the title of the page to NNN Unknown Reason instead of NNN .
Re: Empty Reason Phrase (BZ 44995/45092)
Jeff Trawick schrieb: On Tue, May 20, 2008 at 3:27 PM, Ruediger Pluem [EMAIL PROTECTED] wrote: On 05/20/2008 08:52 PM, Jeff Trawick wrote: Do we really need to require the space in the case that the reason phrase is empty? No... Both sets of code should be changed to consider the space optional. We just need to make sure we generate the space in our response so some other software doesn't have to be so generous. I attached two patches against trunk to BZ 44995, one for the status_line validation in http_filters.c and one for the test when used in error pages in http_protocol.c. Backports should be straighforward, because the code didn't change locally. I didn't yet test thoroughly, but if you like them in principle I'll go through it. Regards, Rainer
Re: Empty Reason Phrase (BZ 44995/45092)
On Tue, May 27, 2008 at 3:29 PM, Rainer Jung [EMAIL PROTECTED] wrote: Jeff Trawick schrieb: On Tue, May 20, 2008 at 3:27 PM, Ruediger Pluem [EMAIL PROTECTED] wrote: On 05/20/2008 08:52 PM, Jeff Trawick wrote: Do we really need to require the space in the case that the reason phrase is empty? No... Both sets of code should be changed to consider the space optional. We just need to make sure we generate the space in our response so some other software doesn't have to be so generous. I attached two patches against trunk to BZ 44995, one for the status_line validation in http_filters.c and one for the test when used in error pages in http_protocol.c. Backports should be straighforward, because the code didn't change locally. I didn't yet test thoroughly, but if you like them in principle I'll go through it. re: https://issues.apache.org/bugzilla/attachment.cgi?id=22019 I don't think r-status_line should be updated by this function (ap_send_error_response()).
Re: Empty Reason Phrase (BZ 44995/45092)
On Tue, May 20, 2008 at 3:27 PM, Ruediger Pluem [EMAIL PROTECTED] wrote: On 05/20/2008 08:52 PM, Jeff Trawick wrote: Do we really need to require the space in the case that the reason phrase is empty? No... Both sets of code should be changed to consider the space optional. We just need to make sure we generate the space in our response so some other software doesn't have to be so generous.
Empty Reason Phrase (BZ 44995/45092)
Hi,
I did some research on BZ 44995 and BZ 45092 and stumbled into the
following observation:
It seems that httpd 2.0 and 2.2 require a non empty reason phrase in the
status line. RFC 2616 allows an empty reason phrase:
6.1 Status-Line
Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF
6.1.1 Status Code and Reason Phrase
Reason-Phrase = *TEXT, excluding CR, LF
Because of the star (*) I read this as empty reason phrase is allowed.
This inconsistency showed up in the above issues, because Tomcat
returned an empty reason phrase for custom HTTP status codes and mod_jk
passed the empty reason phrase along to httpd. mod_proxy_ajp seems to
behave in the same way. This resulted in httpd returning a 500 status
code instead of the custom status code, because any status line with an
empty reason phrase is first replaced by an empty one, and then the
empty one is replaced by the default one. Since there is no default
status line for custom http status codes (like 450), we use the status
500 line. The request is then returned to the client with status 500 and
logged in the access log with the custom status code.
Do you agree, that empty reason phrases should be allowed?
If so, the below code fragments should be reviewed. I could provide the
(trivial) patch.
Furthermore there is a second, related problem (for 2.x *and* 1.3):
error pages use the status line as a title. If the line has an empty
reason phrase *and* uses a custom http status code, error pages will
show the title for status code 500.
I'll add the info to the BZ issues, if there is consensus, that empty
reason phrases should be allowed.
Apache 2.0
==
/httpd/httpd/branches/2.0.x/modules/http/http_protocol.c
1174 /* Confirm that the status line is well-formed and matches r-status.
1175 * Otherwise, a filter may have negated the status line set by a
1176 * handler.
1177 * Zap r-status_line if bad.
1178 */
1179 static void validate_status_line(request_rec *r)
1180 {
1181 char *end;
1182
1183 if (r-status_line
1184 (strlen(r-status_line) = 4
!! Why 4 instead of 3? nnnSPACE should be allowed.
1185 || apr_strtoi64(r-status_line, end, 10) != r-status
1186 || *end != ' '
1187 || (end - 3) != r-status_line)) {
1188 r-status_line = NULL;
1189 }
1190 }
Apache 2.2
==
/httpd/httpd/branches/2.2.x/modules/http/http_filters.c
---
797 /* Confirm that the status line is well-formed and matches r-status.
798 * If they don't match, a filter may have negated the status line
set by a
799 * handler.
800 * Zap r-status_line if bad.
801 */
802 static void validate_status_line(request_rec *r)
803 {
804 char *end;
805
806 if (r-status_line
807 (strlen(r-status_line) = 4
!! Why 4 instead of 3? nnnSPACE should be allowed.
808 || apr_strtoi64(r-status_line, end, 10) != r-status
809 || *end != ' '
810 || (end - 3) != r-status_line)) {
811 r-status_line = NULL;
812 }
813 }
trunk
=
/httpd/httpd/trunk/modules/http/http_filters.c
796 /* Confirm that the status line is well-formed and matches r-status.
797 * If they don't match, a filter may have negated the status line
set by a
798 * handler.
799 * Zap r-status_line if bad.
800 */
801 static void validate_status_line(request_rec *r)
802 {
803 char *end;
804
805 if (r-status_line
806 (strlen(r-status_line) = 4
807 || apr_strtoi64(r-status_line, end, 10) != r-status
808 || *end != ' '
809 || (end - 3) != r-status_line)) {
810 r-status_line = NULL;
811 }
812 }
Regards,
Rainer
Re: Empty Reason Phrase (BZ 44995/45092)
Sorry: BZ 45092 - 45026
Re: Empty Reason Phrase (BZ 44995/45092)
On Tue, May 20, 2008 at 8:56 AM, Rainer Jung [EMAIL PROTECTED] wrote: Hi, I did some research on BZ 44995 and BZ 45092 and stumbled into the following observation: It seems that httpd 2.0 and 2.2 require a non empty reason phrase in the status line. RFC 2616 allows an empty reason phrase: just a little background for the time being, not intended to address the points you raise http://marc.info/?l=apache-httpd-devm=114012084824065w=2 http://marc.info/?l=apache-httpd-devm=114019226127669w=2
Re: Empty Reason Phrase (BZ 44995/45092)
Hi Jeff, Jeff Trawick wrote: On Tue, May 20, 2008 at 8:56 AM, Rainer Jung [EMAIL PROTECTED] wrote: Hi, I did some research on BZ 44995 and BZ 45092 and stumbled into the following observation: It seems that httpd 2.0 and 2.2 require a non empty reason phrase in the status line. RFC 2616 allows an empty reason phrase: just a little background for the time being, not intended to address the points you raise http://marc.info/?l=apache-httpd-devm=114012084824065w=2 http://marc.info/?l=apache-httpd-devm=114019226127669w=2 Thanks for those pointers. Indeed the code fragment cited in my post tries to handle those situations and I don't intend to remove them. What seems to strong for me, is the check for non-emptiness of the reason phrase. Checking consistency between the status codes in r-status and r-status_line is still needed. Regards, Rainer
Re: Empty Reason Phrase (BZ 44995/45092)
On Tue, May 20, 2008 at 8:56 AM, Rainer Jung [EMAIL PROTECTED] wrote:
It seems that httpd 2.0 and 2.2 require a non empty reason phrase in the
status line. RFC 2616 allows an empty reason phrase:
6.1 Status-Line
Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF
6.1.1 Status Code and Reason Phrase
Reason-Phrase = *TEXT, excluding CR, LF
Because of the star (*) I read this as empty reason phrase is allowed.
right, there can be 0 repetitions
my bad ;)
Do you agree, that empty reason phrases should be allowed?
yes
If so, the below code fragments should be reviewed. I could provide the
(trivial) patch.
replace = with
Furthermore there is a second, related problem (for 2.x *and* 1.3): error
pages use the status line as a title. If the line has an empty reason phrase
*and* uses a custom http status code, error pages will show the title for
status code 500.
looks like this code:
Index: modules/http/http_protocol.c
===
--- modules/http/http_protocol.c(revision 658385)
+++ modules/http/http_protocol.c(working copy)
@@ -1235,12 +1235,12 @@
* with the 3 digit status code
*/
if (r-status_line != NULL
- strlen(r-status_line) 4 /* long enough */
+ strlen(r-status_line) = 4 /* long enough */
apr_isdigit(r-status_line[0])
apr_isdigit(r-status_line[1])
apr_isdigit(r-status_line[2])
apr_isspace(r-status_line[3])
- apr_isalnum(r-status_line[4])) {
+ (r-status_line[4] == '\0' || apr_isalnum(r-status_line[4]))) {
title = r-status_line;
}
(nothing tested)
Re: Empty Reason Phrase (BZ 44995/45092)
On 05/20/2008 08:52 PM, Jeff Trawick wrote:
On Tue, May 20, 2008 at 8:56 AM, Rainer Jung [EMAIL PROTECTED] wrote:
It seems that httpd 2.0 and 2.2 require a non empty reason phrase in the
status line. RFC 2616 allows an empty reason phrase:
6.1 Status-Line
Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF
6.1.1 Status Code and Reason Phrase
Reason-Phrase = *TEXT, excluding CR, LF
Because of the star (*) I read this as empty reason phrase is allowed.
right, there can be 0 repetitions
my bad ;)
Do you agree, that empty reason phrases should be allowed?
yes
If so, the below code fragments should be reviewed. I could provide the
(trivial) patch.
replace = with
Furthermore there is a second, related problem (for 2.x *and* 1.3): error
pages use the status line as a title. If the line has an empty reason phrase
*and* uses a custom http status code, error pages will show the title for
status code 500.
looks like this code:
Index: modules/http/http_protocol.c
===
--- modules/http/http_protocol.c(revision 658385)
+++ modules/http/http_protocol.c(working copy)
@@ -1235,12 +1235,12 @@
* with the 3 digit status code
*/
if (r-status_line != NULL
- strlen(r-status_line) 4 /* long enough */
+ strlen(r-status_line) = 4 /* long enough */
apr_isdigit(r-status_line[0])
apr_isdigit(r-status_line[1])
apr_isdigit(r-status_line[2])
apr_isspace(r-status_line[3])
Do we really need to require the space in the case that the reason phrase is
empty?
- apr_isalnum(r-status_line[4])) {
+ (r-status_line[4] == '\0' || apr_isalnum(r-status_line[4]))) {
title = r-status_line;
}
Regards
Rüdiger
Re: Empty Reason Phrase (BZ 44995/45092)
Ruediger Pluem schrieb:
On 05/20/2008 08:52 PM, Jeff Trawick wrote:
On Tue, May 20, 2008 at 8:56 AM, Rainer Jung [EMAIL PROTECTED]
wrote:
It seems that httpd 2.0 and 2.2 require a non empty reason phrase in the
status line. RFC 2616 allows an empty reason phrase:
6.1 Status-Line
Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF
6.1.1 Status Code and Reason Phrase
Reason-Phrase = *TEXT, excluding CR, LF
Because of the star (*) I read this as empty reason phrase is allowed.
right, there can be 0 repetitions
my bad ;)
Do you agree, that empty reason phrases should be allowed?
yes
If so, the below code fragments should be reviewed. I could provide the
(trivial) patch.
replace = with
Furthermore there is a second, related problem (for 2.x *and* 1.3):
error
pages use the status line as a title. If the line has an empty reason
phrase
*and* uses a custom http status code, error pages will show the title
for
status code 500.
looks like this code:
Index: modules/http/http_protocol.c
===
--- modules/http/http_protocol.c(revision 658385)
+++ modules/http/http_protocol.c(working copy)
@@ -1235,12 +1235,12 @@
* with the 3 digit status code
*/
if (r-status_line != NULL
- strlen(r-status_line) 4 /* long enough */
+ strlen(r-status_line) = 4 /* long enough */
apr_isdigit(r-status_line[0])
apr_isdigit(r-status_line[1])
apr_isdigit(r-status_line[2])
apr_isspace(r-status_line[3])
Do we really need to require the space in the case that the reason
phrase is empty?
It seems the SPEC syntax for the status line requires the space. Since
one can't be sure, what expressions the clients use to find the end of
the status code, it would be safe to allow status lines with empty
reason phrase and no space at the end, but to add the space at the end
in this case.
- apr_isalnum(r-status_line[4])) {
+ (r-status_line[4] == '\0' ||
apr_isalnum(r-status_line[4]))) {
I wonder what the reason was, to check only the first character of the
reason phrase? I would have expected either a complete check, if it
completely consists of TEXT, excluding CR, LF, or no check at all.
title = r-status_line;
}
Regards,
Rainer
