You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2002/09/24 16:36:04 UTC
[PATCH] Be more lenient with ap_proxy_read_headers in 2.0
In the 2.0 proxy, a bogus header line causes an immediate 502 error,
whereas in 1.3, we try to handle it. The below patch attemps to make
2.0 a bit more lenient in what we accept, and allows for backwards
compatibility with what 1.3 does (principle of least astonishment).
I haven't fully tested this yet, but comments/feedback are welcome.
Index: modules/proxy/proxy_util.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_util.c,v
retrieving revision 1.98
diff -u -r1.98 proxy_util.c
--- modules/proxy/proxy_util.c 25 Aug 2002 20:40:11 -0000 1.98
+++ modules/proxy/proxy_util.c 24 Sep 2002 14:21:06 -0000
@@ -433,6 +433,7 @@
int len;
char *value, *end;
char field[MAX_STRING_LEN];
+ int saw_headers = 0;
headers_out = apr_table_make(r->pool, 20);
@@ -447,12 +448,25 @@
/* Buggy MS IIS servers sometimes return invalid headers
* (an extra "HTTP/1.0 200, OK" line sprinkled in between
* the usual MIME headers). Try to deal with it in a sensible
- * way, but log the fact.
+ * way, but log the fact. If it's not that, but we've seen
+ * some headers up to now, return them, The also handles the
+ * fact that buggy MS IIS servers sometimes forget the CRLF
+ * between headers and content.
* XXX: The mask check is buggy if we ever see an HTTP/1.10 */
if (!apr_date_checkmask(buffer, "HTTP/#.# ###*")) {
/* Nope, it wasn't even an extra HTTP header. Give up. */
- return NULL;
+ if (saw_headers) {
+ ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
+ "proxy: Ignoring bogus non-header in headers "
+ "returned by %s (%s)", r->uri, r->method);
+ return headers_out;
+ } else {
+ ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
+ "proxy: No HTTP headers "
+ "returned by %s (%s)", r->uri, r->method);
+ return NULL;
+ }
}
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
@@ -475,6 +489,7 @@
/* make sure we add so as not to destroy duplicated headers */
apr_table_add(headers_out, buffer, value);
+ saw_headers = 1;
/* the header was too long; at the least we should skip extra data */
if (len >= size - 1) {
--
===========================================================================
Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
"A society that will trade a little liberty for a little order
will lose both and deserve neither" - T.Jefferson
Re: [PATCH] Be more lenient with ap_proxy_read_headers in 2.0
Posted by Jim Jagielski <ji...@jaguNET.com>.
At 6:16 PM +0200 9/25/02, Graham Leggett wrote:
>
>The above two cases of brokenness are mutually exclusive. If a non-header is encountered when a header is expected, we have the choice of either assuming it's a header and ignoring it, or assuming the headers are over and starting the body. We cannot do both at the same time.
The patch would do the latter. The sole exception is that if it sees
the HTTP/1.1 response again, it ignores that. Otherwise, game over.
>What's more there is a feeling out there that it is Apache's responsiblity to "fix" all the broken servers out there. I believe it is not. It makes sense to be lenient in what we accept, but to accept any old rubbish given to us is just wrong.
>
Oh, I agree. See my comments on Bugz 11800. The *only* reason why
I'm leaning towards "fixing" that (and that is *not* the right word,
since "fix" implies something is broken, which is not the case here)
is that 1.3 is, in this case, more robust than 2.0 is. Migration to 2.0
is not an option for those who tickle this bug, and they are "stuck"
with 1.3. Simple on that basis is why I created and posted the patch.
I agree that we cannot and should not fix or work-around everything that's
broken out there, but the 1.3 argument sometimes is enough...
--
===========================================================================
Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
"A society that will trade a little liberty for a little order
will lose both and deserve neither" - T.Jefferson
Re: [PATCH] Be more lenient with ap_proxy_read_headers in 2.0
Posted by Graham Leggett <mi...@sharp.fm>.
Peter Van Biesen wrote:
> I've sent a patch doing the same some time ago, but it was not accepted
> so don't get your hopes up ;-).
The big problem is that the code is starting to be so lenient it is
getting silly.
If a bogus header comes along (ie a header without a ":" in it) it is
relatively safe and easy to throw it away and ignore that specific
header. AFAIK there is code in there that already does that, and if it
doesn't such a change should not be too hard to put in.
However, the blank line after the headers is the key indicator that the
headers are finished and the body is starting. If a server out there
"forgets" this blank line, there is no way we can reliably tell that the
headers have ended, which means we are probably going to be sending
garbage to the downstream browser as a result anyway.
The above two cases of brokenness are mutually exclusive. If a
non-header is encountered when a header is expected, we have the choice
of either assuming it's a header and ignoring it, or assuming the
headers are over and starting the body. We cannot do both at the same time.
What's more there is a feeling out there that it is Apache's
responsiblity to "fix" all the broken servers out there. I believe it is
not. It makes sense to be lenient in what we accept, but to accept any
old rubbish given to us is just wrong.
Hopefully this explains the reasons why some of the patches have not
been committed.
Regards,
Graham
--
-----------------------------------------
minfrin@sharp.fm "There's a moon
over Bourbon Street
tonight..."
Re: [PATCH] Be more lenient with ap_proxy_read_headers in 2.0
Posted by Peter Van Biesen <pe...@vlafo.be>.
I've sent a patch doing the same some time ago, but it was not accepted
so don't get your hopes up ;-).
Peter.
Jim Jagielski wrote:
>
> In the 2.0 proxy, a bogus header line causes an immediate 502 error,
> whereas in 1.3, we try to handle it. The below patch attemps to make
> 2.0 a bit more lenient in what we accept, and allows for backwards
> compatibility with what 1.3 does (principle of least astonishment).
> I haven't fully tested this yet, but comments/feedback are welcome.
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_util.c,v
> retrieving revision 1.98
> diff -u -r1.98 proxy_util.c
> --- modules/proxy/proxy_util.c 25 Aug 2002 20:40:11 -0000 1.98
> +++ modules/proxy/proxy_util.c 24 Sep 2002 14:21:06 -0000
> @@ -433,6 +433,7 @@
> int len;
> char *value, *end;
> char field[MAX_STRING_LEN];
> + int saw_headers = 0;
>
> headers_out = apr_table_make(r->pool, 20);
>
> @@ -447,12 +448,25 @@
> /* Buggy MS IIS servers sometimes return invalid headers
> * (an extra "HTTP/1.0 200, OK" line sprinkled in between
> * the usual MIME headers). Try to deal with it in a sensible
> - * way, but log the fact.
> + * way, but log the fact. If it's not that, but we've seen
> + * some headers up to now, return them, The also handles the
> + * fact that buggy MS IIS servers sometimes forget the CRLF
> + * between headers and content.
> * XXX: The mask check is buggy if we ever see an HTTP/1.10 */
>
> if (!apr_date_checkmask(buffer, "HTTP/#.# ###*")) {
> /* Nope, it wasn't even an extra HTTP header. Give up. */
> - return NULL;
> + if (saw_headers) {
> + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
> + "proxy: Ignoring bogus non-header in headers "
> + "returned by %s (%s)", r->uri, r->method);
> + return headers_out;
> + } else {
> + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
> + "proxy: No HTTP headers "
> + "returned by %s (%s)", r->uri, r->method);
> + return NULL;
> + }
> }
>
> ap_log_error(APLOG_MARK, APLOG_WARNING, 0, r->server,
> @@ -475,6 +489,7 @@
>
> /* make sure we add so as not to destroy duplicated headers */
> apr_table_add(headers_out, buffer, value);
> + saw_headers = 1;
>
> /* the header was too long; at the least we should skip extra data */
> if (len >= size - 1) {
> --
> ===========================================================================
> Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
> "A society that will trade a little liberty for a little order
> will lose both and deserve neither" - T.Jefferson