You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Adam Sussman <my...@vishnu.vidya.com> on 2001/12/29 02:24:00 UTC
[PATCH] mod_proxy infinite cpu eating loop
ap_proxy_string_read currently goes into an infinite loop when the proxied server
closes the connection without sending any data. This patch fixes the problem
but I am not sure that this is the right way to do it.
-adam
Index: modules/proxy/proxy_util.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/proxy/proxy_util.c,v
retrieving revision 1.73
diff -u -r1.73 proxy_util.c
--- modules/proxy/proxy_util.c 28 Nov 2001 21:07:32 -0000 1.73
+++ modules/proxy/proxy_util.c 29 Dec 2001 00:14:18 -0000
@@ -1039,6 +1039,7 @@
APR_BUCKET_REMOVE(e);
apr_bucket_destroy(e);
}
+ if (APR_BRIGADE_EMPTY(bb)) break;
}
return APR_SUCCESS;
Re: [PATCH] mod_proxy infinite cpu eating loop
Posted by Bill Stoddard <bi...@wstoddard.com>.
Can you identify where the seg fault is happening? I am really suspicious of why checking
for length == 0 would avoid the segfault. As I mentioned in an earlier post, I suspect
this check is masking another problem.
Thanks,
Bill
>
> Bill,
>
> Everything looks good except that ap_proxy_string_read() now segfaults on
> its apr_bucket_read(). It appears that APR_BRIGADE_EMPTY is not sufficient
> to detect the case of the closed socket with no data. I suspect that this
> is because the core filter seeds the brigade with a socket bucket and that
> this is what is giving apr_bucket_read problems.
>
> This bit of code seems to do the trick, but again I am still a little unsure
> of my footing with filters and buckets.
>
> -adam
>
>
> Index: proxy_util.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/proxy/proxy_util.c,v
> retrieving revision 1.75
> diff -u -r1.75 proxy_util.c
> --- proxy_util.c 31 Dec 2001 20:43:59 -0000 1.75
> +++ proxy_util.c 2 Jan 2002 01:29:58 -0000
> @@ -1031,7 +1031,13 @@
> if (APR_BUCKET_IS_EOS(e)) {
> *eos = 1;
> }
> + else if (e->length == 0) {
> + APR_BUCKET_REMOVE(e);
> + apr_bucket_destroy(e);
> + break;
> + }
> else {
> +
> if (APR_SUCCESS != apr_bucket_read(e, (const char **)&response, &len,
APR_BLOCK_READ)) {
> return rv;
> }
>
Re: [PATCH] mod_proxy infinite cpu eating loop
Posted by Adam Sussman <my...@vishnu.vidya.com>.
Bill,
Everything looks good except that ap_proxy_string_read() now segfaults on
its apr_bucket_read(). It appears that APR_BRIGADE_EMPTY is not sufficient
to detect the case of the closed socket with no data. I suspect that this
is because the core filter seeds the brigade with a socket bucket and that
this is what is giving apr_bucket_read problems.
This bit of code seems to do the trick, but again I am still a little unsure
of my footing with filters and buckets.
-adam
Index: proxy_util.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/proxy/proxy_util.c,v
retrieving revision 1.75
diff -u -r1.75 proxy_util.c
--- proxy_util.c 31 Dec 2001 20:43:59 -0000 1.75
+++ proxy_util.c 2 Jan 2002 01:29:58 -0000
@@ -1031,7 +1031,13 @@
if (APR_BUCKET_IS_EOS(e)) {
*eos = 1;
}
+ else if (e->length == 0) {
+ APR_BUCKET_REMOVE(e);
+ apr_bucket_destroy(e);
+ break;
+ }
else {
+
if (APR_SUCCESS != apr_bucket_read(e, (const char **)&response, &len, APR_BLOCK_READ)) {
return rv;
}
Re: [PATCH] mod_proxy infinite cpu eating loop
Posted by Bill Stoddard <bi...@wstoddard.com>.
Adam,
I am leaving ap_proxy_string_read() in place for now. I think the 'right' thing to do is
to use ap_getline() but I havent the time to figure out how to replace
ap_proxy_string_read() in the proxy_ftp code (no request_rec and ap_getline requires a
request_rec).
I am checking in your fixes with some small tweaks here and there as needed.
Bill
>
> hmm, so I tried out this patch and found that it does work correctly
> for most cases and it does solve the original infinite loop problem.
> However, it appears to have introduced a new infinite loop problem
> as well as some truncation of proxy data.
>
> Once status and header data have been read (or attempted to be read
> in the case of HTTP/0.9), mod_proxy is busy waiting for body content.
> This shows up as 100% cpu on my setup. The loop where this is happening
> is based on a non-blocking call to ap_get_brigade() in proxy_http.c:856.
> Can anyone tell me why this call should not block?
>
> In the case of a HTTP/0.9 response, the line feed on the first line
> (where status is tested) is eaten and never shows up in the output.
> I suspect that is because of ap_rgetline().
>
> Lastly, proxy_ftp also uses ap_proxy_string_read and will need to be
> dealt with if we trash that function.
>
> -adam
>
> On Sat, Dec 29, 2001 at 08:02:32AM -0500, Bill Stoddard wrote:
> > I spent a bit of time looking at this one and I am pretty sure this is not the right
> > patch. The problem is that ap_proxy_string_read() is completely broken. Among other
> > things, it completely chokes if the 'string' spans multiple brigades.
> > ap_proxy_string_read should be trashed and something like this patch should be used
> > instead (not tested):
> >
> > Index: proxy_http.c
> > ===================================================================
> > RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_http.c,v
> > retrieving revision 1.114
> > diff -u -r1.114 proxy_http.c
> > --- proxy_http.c 19 Dec 2001 16:32:01 -0000 1.114
> > +++ proxy_http.c 29 Dec 2001 12:57:09 -0000
> > @@ -657,6 +657,22 @@
> > while (received_continue) {
> > apr_brigade_cleanup(bb);
> >
> > + while ((len = ap_getline(buffer, sizeof(buffer), rp, 0)) <= 0) {
> > + if (len < 0) {
> > + /* return status... what? timeout? connection dropped?
> > + * for now, just use what was returned in the original broken code
> > + * set rp->aborted?
> > + */
> > + apr_socket_close(p_conn->sock);
> > + backend->connection = NULL;
> > + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> > + "proxy: error reading status line from remote "
> > + "server %s", p_conn->name);
> > + return ap_proxyerror(r, HTTP_BAD_GATEWAY,
> > + "Error reading from remote server");
> > + }
> > + }
> > +#if 0
> > if (APR_SUCCESS != (rv = ap_proxy_string_read(origin, bb, buffer,
sizeof(buffer),
> > &eos))) {
> > apr_socket_close(p_conn->sock);
> > backend->connection = NULL;
> > @@ -667,7 +683,7 @@
> > "Error reading from remote server");
> > }
> > len = strlen(buffer);
> > -
> > +#endif
> > /* Is it an HTTP/1 response?
> > * This is buggy if we ever see an HTTP/1.10
> > */
> >
> > ----- Original Message -----
> > From: "Adam Sussman" <my...@vishnu.vidya.com>
> > To: <de...@httpd.apache.org>
> > Sent: Friday, December 28, 2001 8:24 PM
> > Subject: [PATCH] mod_proxy infinite cpu eating loop
> >
> >
> > >
> > > ap_proxy_string_read currently goes into an infinite loop when the proxied server
> > > closes the connection without sending any data. This patch fixes the problem
> > > but I am not sure that this is the right way to do it.
> > >
> > > -adam
> > >
> > >
> > > Index: modules/proxy/proxy_util.c
> > > ===================================================================
> > > RCS file: /home/cvspublic/httpd-2.0/modules/proxy/proxy_util.c,v
> > > retrieving revision 1.73
> > > diff -u -r1.73 proxy_util.c
> > > --- modules/proxy/proxy_util.c 28 Nov 2001 21:07:32 -0000 1.73
> > > +++ modules/proxy/proxy_util.c 29 Dec 2001 00:14:18 -0000
> > > @@ -1039,6 +1039,7 @@
> > > APR_BUCKET_REMOVE(e);
> > > apr_bucket_destroy(e);
> > > }
> > > + if (APR_BRIGADE_EMPTY(bb)) break;
> > > }
> > >
> > > return APR_SUCCESS;
> > >
>
>
>
> --
>
> "I believe in Kadath in the cold waste, and Ultima Thule. But you
> cannot prove to me that Harvard Law School actually exists."
> - Theodora Goss
>
> "I'm not like that, I have a cat, I don't need you.. My cat, and
> about 18 lines of bourne shell code replace you in life."
> - anonymous
>
>
> Adam Sussman
> Vidya Media Ventures
>
> asussman@vidya.com
>
Re: [PATCH] mod_proxy infinite cpu eating loop
Posted by Adam Sussman <my...@vishnu.vidya.com>.
hmm, so I tried out this patch and found that it does work correctly
for most cases and it does solve the original infinite loop problem.
However, it appears to have introduced a new infinite loop problem
as well as some truncation of proxy data.
Once status and header data have been read (or attempted to be read
in the case of HTTP/0.9), mod_proxy is busy waiting for body content.
This shows up as 100% cpu on my setup. The loop where this is happening
is based on a non-blocking call to ap_get_brigade() in proxy_http.c:856.
Can anyone tell me why this call should not block?
In the case of a HTTP/0.9 response, the line feed on the first line
(where status is tested) is eaten and never shows up in the output.
I suspect that is because of ap_rgetline().
Lastly, proxy_ftp also uses ap_proxy_string_read and will need to be
dealt with if we trash that function.
-adam
On Sat, Dec 29, 2001 at 08:02:32AM -0500, Bill Stoddard wrote:
> I spent a bit of time looking at this one and I am pretty sure this is not the right
> patch. The problem is that ap_proxy_string_read() is completely broken. Among other
> things, it completely chokes if the 'string' spans multiple brigades.
> ap_proxy_string_read should be trashed and something like this patch should be used
> instead (not tested):
>
> Index: proxy_http.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_http.c,v
> retrieving revision 1.114
> diff -u -r1.114 proxy_http.c
> --- proxy_http.c 19 Dec 2001 16:32:01 -0000 1.114
> +++ proxy_http.c 29 Dec 2001 12:57:09 -0000
> @@ -657,6 +657,22 @@
> while (received_continue) {
> apr_brigade_cleanup(bb);
>
> + while ((len = ap_getline(buffer, sizeof(buffer), rp, 0)) <= 0) {
> + if (len < 0) {
> + /* return status... what? timeout? connection dropped?
> + * for now, just use what was returned in the original broken code
> + * set rp->aborted?
> + */
> + apr_socket_close(p_conn->sock);
> + backend->connection = NULL;
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> + "proxy: error reading status line from remote "
> + "server %s", p_conn->name);
> + return ap_proxyerror(r, HTTP_BAD_GATEWAY,
> + "Error reading from remote server");
> + }
> + }
> +#if 0
> if (APR_SUCCESS != (rv = ap_proxy_string_read(origin, bb, buffer, sizeof(buffer),
> &eos))) {
> apr_socket_close(p_conn->sock);
> backend->connection = NULL;
> @@ -667,7 +683,7 @@
> "Error reading from remote server");
> }
> len = strlen(buffer);
> -
> +#endif
> /* Is it an HTTP/1 response?
> * This is buggy if we ever see an HTTP/1.10
> */
>
> ----- Original Message -----
> From: "Adam Sussman" <my...@vishnu.vidya.com>
> To: <de...@httpd.apache.org>
> Sent: Friday, December 28, 2001 8:24 PM
> Subject: [PATCH] mod_proxy infinite cpu eating loop
>
>
> >
> > ap_proxy_string_read currently goes into an infinite loop when the proxied server
> > closes the connection without sending any data. This patch fixes the problem
> > but I am not sure that this is the right way to do it.
> >
> > -adam
> >
> >
> > Index: modules/proxy/proxy_util.c
> > ===================================================================
> > RCS file: /home/cvspublic/httpd-2.0/modules/proxy/proxy_util.c,v
> > retrieving revision 1.73
> > diff -u -r1.73 proxy_util.c
> > --- modules/proxy/proxy_util.c 28 Nov 2001 21:07:32 -0000 1.73
> > +++ modules/proxy/proxy_util.c 29 Dec 2001 00:14:18 -0000
> > @@ -1039,6 +1039,7 @@
> > APR_BUCKET_REMOVE(e);
> > apr_bucket_destroy(e);
> > }
> > + if (APR_BRIGADE_EMPTY(bb)) break;
> > }
> >
> > return APR_SUCCESS;
> >
--
"I believe in Kadath in the cold waste, and Ultima Thule. But you
cannot prove to me that Harvard Law School actually exists."
- Theodora Goss
"I'm not like that, I have a cat, I don't need you.. My cat, and
about 18 lines of bourne shell code replace you in life."
- anonymous
Adam Sussman
Vidya Media Ventures
asussman@vidya.com
Re: [PATCH] mod_proxy infinite cpu eating loop
Posted by Bill Stoddard <bi...@wstoddard.com>.
I spent a bit of time looking at this one and I am pretty sure this is not the right
patch. The problem is that ap_proxy_string_read() is completely broken. Among other
things, it completely chokes if the 'string' spans multiple brigades.
ap_proxy_string_read should be trashed and something like this patch should be used
instead (not tested):
Index: proxy_http.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/proxy/proxy_http.c,v
retrieving revision 1.114
diff -u -r1.114 proxy_http.c
--- proxy_http.c 19 Dec 2001 16:32:01 -0000 1.114
+++ proxy_http.c 29 Dec 2001 12:57:09 -0000
@@ -657,6 +657,22 @@
while (received_continue) {
apr_brigade_cleanup(bb);
+ while ((len = ap_getline(buffer, sizeof(buffer), rp, 0)) <= 0) {
+ if (len < 0) {
+ /* return status... what? timeout? connection dropped?
+ * for now, just use what was returned in the original broken code
+ * set rp->aborted?
+ */
+ apr_socket_close(p_conn->sock);
+ backend->connection = NULL;
+ ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
+ "proxy: error reading status line from remote "
+ "server %s", p_conn->name);
+ return ap_proxyerror(r, HTTP_BAD_GATEWAY,
+ "Error reading from remote server");
+ }
+ }
+#if 0
if (APR_SUCCESS != (rv = ap_proxy_string_read(origin, bb, buffer, sizeof(buffer),
&eos))) {
apr_socket_close(p_conn->sock);
backend->connection = NULL;
@@ -667,7 +683,7 @@
"Error reading from remote server");
}
len = strlen(buffer);
-
+#endif
/* Is it an HTTP/1 response?
* This is buggy if we ever see an HTTP/1.10
*/
----- Original Message -----
From: "Adam Sussman" <my...@vishnu.vidya.com>
To: <de...@httpd.apache.org>
Sent: Friday, December 28, 2001 8:24 PM
Subject: [PATCH] mod_proxy infinite cpu eating loop
>
> ap_proxy_string_read currently goes into an infinite loop when the proxied server
> closes the connection without sending any data. This patch fixes the problem
> but I am not sure that this is the right way to do it.
>
> -adam
>
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/modules/proxy/proxy_util.c,v
> retrieving revision 1.73
> diff -u -r1.73 proxy_util.c
> --- modules/proxy/proxy_util.c 28 Nov 2001 21:07:32 -0000 1.73
> +++ modules/proxy/proxy_util.c 29 Dec 2001 00:14:18 -0000
> @@ -1039,6 +1039,7 @@
> APR_BUCKET_REMOVE(e);
> apr_bucket_destroy(e);
> }
> + if (APR_BRIGADE_EMPTY(bb)) break;
> }
>
> return APR_SUCCESS;
>