Re: 1.3.31 regression affecting Front Page?
On Wed, 9 Jun 2004, Jim Jagielski wrote: On Jun 9, 2004, at 3:24 PM, Rasmus Lerdorf wrote: I guess what we are agreeing on here is that the logic that sets keepalive to 0 is faulty and that is probably where the real fix lies. yeah... it's pretty inconsistent. Looking at ap_set_keepalive even after we know the connection will be closed, we set keepalive to 0, for example. Ok, I had a closer look at the flow. There are 6 main cases I care about. Static, Dynamic and Error requests for configs KeepAlive=Off and KeepAlive=On. Here is what happens: With KeepAlive On For both static and dynamic requests the flow is similar. We start connection-keepalive starts at 0 at the beginning of the request process_request_internal eventually leads to an ap_send_http_header call which calls ap_set_keepalive which determines that the config has KeepAlive on and sets connection-keepalive to 1 For an error, like a 404, it is different. We start off with connection-keepalive being set to 0, process_internal_request calls ap_die on the error which calls ap_send_error_response which in turn calls ap_send_http_header which finally sets connection_keepalive to 1. But this happens after ap_die checks conn-keepalive to determine whether to discard the request body or not. With KeepAlive Off The picture is as above, except ap_set_keepalive called from ap_send_http_header sets connection-keepalive to 0 instead of 1. So for the duration of the request, before and after checking whether we are on a keepalive connection, connection-keepalive is 0. The summary here is that checking connection-keepalive before ap_set_keepalive() has been called really doesn't make any sense. And we can't just call ap_set_keepalive in ap_die before the check because it would end up getting called twice and there is no guard against that in it. It would double-count the request in the keepalives counter. We need to either call ap_set_keepalive earlier on, like in process_request_internal before we hit ap_die, or we need to add a double-call guard in it and just add a call in ap_die before the keepalive check. Another alternative would be to clean up this mess which has our undecided state indistinguishable from our disabled state. Checking for 0 in ap_die is only wrong because the check is before the ap_set_keepalive call. The meaning of that 0 changes on that call. -Rasmus
Re: 1.3.31 regression affecting Front Page?
I had sent private Email to your @apache.org address (since that's the one you use to provide HTTPD related patches). On Jun 8, 2004, at 5:10 PM, Rasmus Lerdorf wrote: Uh, I never received anything on this. Did you actually send me something? I'll have a look at addressing this issue. Releasing 1.3.32 without this fix would be a nasty backwards step. The original problem this fixes is serious. -Rasmus On Fri, 28 May 2004, Jim Jagielski wrote: I've backed out that patch and asked Rasmus to send a replacemnet which addresses his specific problem but does not cause the below behavior. I'm tempted to release 1.3.32... Jeff Trawick wrote: This patch did it: http://cvs.apache.org/viewcvs.cgi/apache-1.3/src/main/ http_request.c?r1=1.173r2=1.174 See also http://issues.apache.org/bugzilla/show_bug.cgi?id=29257 http://www.rtr.com/fp2002disc/_disc2/0a71.htm -- == = Jim Jagielski [|] [EMAIL PROTECTED] [|] 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: 1.3.31 regression affecting Front Page?
Don't see that anywhere. Either eaten by spam filters or a gerbil. Anyway, I don't understand why this would have broken mod_dav. If mod_dav wants a keepalive connection it should determine this prior to the ap_die and set conn-keepalive to 1. Or am I missing something with respect to what mod_dav is doing here? I suppose we could add an ugly exception for a PROPFIND here, but I'd like to make sure that is actually needed. Without this patch non-keepalive connections are not being dropped when we know there is nothing more to do. For example, on a server that doesn't allow POST someone can POST to it and it will happily sit there and read the entire POST request. This defeats the purpose of adding a Limit POST and introduces a DoS. Same for a 404 or any other error handler. I can POST to a bogus URL and Apache will read the entire POST request even though we know it is a 404 at this point and that we can safely discard the request body. I don't think releasing .32 without addressing this issue is a good idea. -Rasmus On Wed, 9 Jun 2004, Jim Jagielski wrote: I had sent private Email to your @apache.org address (since that's the one you use to provide HTTPD related patches). On Jun 8, 2004, at 5:10 PM, Rasmus Lerdorf wrote: Uh, I never received anything on this. Did you actually send me something? I'll have a look at addressing this issue. Releasing 1.3.32 without this fix would be a nasty backwards step. The original problem this fixes is serious. -Rasmus On Fri, 28 May 2004, Jim Jagielski wrote: I've backed out that patch and asked Rasmus to send a replacemnet which addresses his specific problem but does not cause the below behavior. I'm tempted to release 1.3.32... Jeff Trawick wrote: This patch did it: http://cvs.apache.org/viewcvs.cgi/apache-1.3/src/main/ http_request.c?r1=1.173r2=1.174 See also http://issues.apache.org/bugzilla/show_bug.cgi?id=29257 http://www.rtr.com/fp2002disc/_disc2/0a71.htm -- == = Jim Jagielski [|] [EMAIL PROTECTED] [|] 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: 1.3.31 regression affecting Front Page?
On Wed, Jun 09, 2004 at 09:21:07AM -0700, Rasmus Lerdorf wrote: Don't see that anywhere. Either eaten by spam filters or a gerbil. Anyway, I don't understand why this would have broken mod_dav. If mod_dav wants a keepalive connection it should determine this prior to the ap_die and set conn-keepalive to 1. Or am I missing something with respect to what mod_dav is doing here? I suppose we could add an ugly exception for a PROPFIND here, but I'd like to make sure that is actually needed. From my debugging: mod_dav doesn't actually get involved at all, the check_user_id handler from the auth module returns AUTH_REQUIRED for the request, and ap_set_keepalive is not called before ap_die is invoked in that case. r-connection-keepalive has never been changed from 0 when the test in question is reached - it's called later by ap_send_error_response, but that's too late. Without this patch non-keepalive connections are not being dropped when we know there is nothing more to do. For example, on a server that doesn't allow POST someone can POST to it and it will happily sit there and read the entire POST request. This defeats the purpose of adding a Limit POST and introduces a DoS. Same for a 404 or any other error handler. I can POST to a bogus URL and Apache will read the entire POST request even though we know it is a 404 at this point and that we can safely discard the request body. I don't think releasing .32 without addressing this issue is a good idea.
Re: 1.3.31 regression affecting Front Page?
On Wed, 9 Jun 2004, Joe Orton wrote: On Wed, Jun 09, 2004 at 09:21:07AM -0700, Rasmus Lerdorf wrote: Don't see that anywhere. Either eaten by spam filters or a gerbil. Anyway, I don't understand why this would have broken mod_dav. If mod_dav wants a keepalive connection it should determine this prior to the ap_die and set conn-keepalive to 1. Or am I missing something with respect to what mod_dav is doing here? I suppose we could add an ugly exception for a PROPFIND here, but I'd like to make sure that is actually needed. From my debugging: mod_dav doesn't actually get involved at all, the check_user_id handler from the auth module returns AUTH_REQUIRED for the request, and ap_set_keepalive is not called before ap_die is invoked in that case. r-connection-keepalive has never been changed from 0 when the test in question is reached - it's called later by ap_send_error_response, but that's too late. Just to save me some debugging time, are you saying that on a 401 we call ap_set_keepalive after calling ap_die or that we are not calling ap_set_keepalive at all? The former should never happen as far as I am concerned, and the latter should be fine as the request simply won't be a keepalive request and nothing should expect it to be. -Rasmus
Re: 1.3.31 regression affecting Front Page?
On Wed, Jun 09, 2004 at 11:04:23AM -0700, Rasmus Lerdorf wrote: On Wed, 9 Jun 2004, Joe Orton wrote: On Wed, Jun 09, 2004 at 09:21:07AM -0700, Rasmus Lerdorf wrote: Don't see that anywhere. Either eaten by spam filters or a gerbil. Anyway, I don't understand why this would have broken mod_dav. If mod_dav wants a keepalive connection it should determine this prior to the ap_die and set conn-keepalive to 1. Or am I missing something with respect to what mod_dav is doing here? I suppose we could add an ugly exception for a PROPFIND here, but I'd like to make sure that is actually needed. From my debugging: mod_dav doesn't actually get involved at all, the check_user_id handler from the auth module returns AUTH_REQUIRED for the request, and ap_set_keepalive is not called before ap_die is invoked in that case. r-connection-keepalive has never been changed from 0 when the test in question is reached - it's called later by ap_send_error_response, but that's too late. Just to save me some debugging time, are you saying that on a 401 we call ap_set_keepalive after calling ap_die or that we are not calling ap_set_keepalive at all? The former should never happen as far as I am concerned, and the latter should be fine as the request simply won't be a keepalive request and nothing should expect it to be. When ap_die() is called, ap_set_keepalive() has not been called, and r-connection-keepalive is set to 0, as it was initialized so. The last thing ap_die does is call ap_send_error_response, which calls ap_send_http_header, which calls ap_set_keepalive, which sets r-connection-keepalive = 1. FWIW, r-connection-keeaplive == 0 is documented in the header to mean undecided, not specifically no keep alive, as you seem to want to use it. joe
Re: 1.3.31 regression affecting Front Page?
On Wed, 9 Jun 2004, Joe Orton wrote: When ap_die() is called, ap_set_keepalive() has not been called, and r-connection-keepalive is set to 0, as it was initialized so. The last thing ap_die does is call ap_send_error_response, which calls ap_send_http_header, which calls ap_set_keepalive, which sets r-connection-keepalive = 1. FWIW, r-connection-keeaplive == 0 is documented in the header to mean undecided, not specifically no keep alive, as you seem to want to use it. Well, we have a bunch of cases where we leave it undecided when it is decidedly not undecided. For example, when I configure a server to exlicitly not use keepalives and restrict Post requests with a Limit Post clause, I don't expect a POST request with a huge file upload to hit this ap_die code and suck in the entire POST request because something has decided that the keepalive state is undecided. It's not undecided, I have told it that it is off and the 403 or 404 is the second big clue. I guess what we are agreeing on here is that the logic that sets keepalive to 0 is faulty and that is probably where the real fix lies. -Rasmus
Re: 1.3.31 regression affecting Front Page?
On Jun 9, 2004, at 3:24 PM, Rasmus Lerdorf wrote: I guess what we are agreeing on here is that the logic that sets keepalive to 0 is faulty and that is probably where the real fix lies. yeah... it's pretty inconsistent. Looking at ap_set_keepalive even after we know the connection will be closed, we set keepalive to 0, for example.
Re: 1.3.31 regression affecting Front Page?
William A. Rowe, Jr. wrote: At 07:09 AM 5/28/2004, Jim Jagielski wrote: I've backed out that patch and asked Rasmus to send a replacemnet which addresses his specific problem but does not cause the below behavior. I'm tempted to release 1.3.32... Collect another week or few of data on other problems first, perhaps? Once the replacement patch arrives - we have a home for this over in /dist/httpd/patches/apply_to_1_3_31/, right? I'd also like to see a few other minor quibbles fixed, like the change of canonical name behavior. I do have to wrap my head around that issue still. Well, I didn't mean immediately :) I do mean to release 1.3.32 much sooner than previous 1.3 versions have been released, like sometime in June. Although I'm still mulling over the UCN issue (and when/how to have Apache sort through the logic flow of how to choose the correct port number), there are other patches that would be beneficial to fold in, like the various ones you provided. -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] 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: 1.3.31 regression affecting Front Page?
Jeff Trawick wrote: This patch did it: http://cvs.apache.org/viewcvs.cgi/apache-1.3/src/main/http_request.c?r1=1.173r2=1.174 Backing out the patch also fixes a DAV regression. See http://issues.apache.org/bugzilla/show_bug.cgi?id=29237 See also http://issues.apache.org/bugzilla/show_bug.cgi?id=29257 http://www.rtr.com/fp2002disc/_disc2/0a71.htm
Re: 1.3.31 regression affecting Front Page?
On Fri, May 28, 2004 at 06:14:30AM -0400, Jeff Trawick wrote: Jeff Trawick wrote: This patch did it: http://cvs.apache.org/viewcvs.cgi/apache-1.3/src/main/http_request.c?r1=1.173r2=1.174 Backing out the patch also fixes a DAV regression. See http://issues.apache.org/bugzilla/show_bug.cgi?id=29237 It's simple enough to reproduce; the request body is not getting discarded (since r-connection-keepalive = 0) for an error response on an actually-will-be-keptalive connection, e.g. the 401 response to a PROPFIND with a body. joe
Re: 1.3.31 regression affecting Front Page?
Hmm.. this was a patch suggested by Rasmus, iirc. Jeff Trawick wrote: Jeff Trawick wrote: This patch did it: http://cvs.apache.org/viewcvs.cgi/apache-1.3/src/main/http_request.c?r1=1.173r2=1.174 Backing out the patch also fixes a DAV regression. See http://issues.apache.org/bugzilla/show_bug.cgi?id=29237 See also http://issues.apache.org/bugzilla/show_bug.cgi?id=29257 http://www.rtr.com/fp2002disc/_disc2/0a71.htm -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] 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: 1.3.31 regression affecting Front Page?
I've backed out that patch and asked Rasmus to send a replacemnet which addresses his specific problem but does not cause the below behavior. I'm tempted to release 1.3.32... Jeff Trawick wrote: This patch did it: http://cvs.apache.org/viewcvs.cgi/apache-1.3/src/main/http_request.c?r1=1.173r2=1.174 See also http://issues.apache.org/bugzilla/show_bug.cgi?id=29257 http://www.rtr.com/fp2002disc/_disc2/0a71.htm -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] 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: 1.3.31 regression affecting Front Page?
At 07:09 AM 5/28/2004, Jim Jagielski wrote: I've backed out that patch and asked Rasmus to send a replacemnet which addresses his specific problem but does not cause the below behavior. I'm tempted to release 1.3.32... Collect another week or few of data on other problems first, perhaps? Once the replacement patch arrives - we have a home for this over in /dist/httpd/patches/apply_to_1_3_31/, right? I'd also like to see a few other minor quibbles fixed, like the change of canonical name behavior. I do have to wrap my head around that issue still. Bill
1.3.31 regression affecting Front Page?
This patch did it: http://cvs.apache.org/viewcvs.cgi/apache-1.3/src/main/http_request.c?r1=1.173r2=1.174 See also http://issues.apache.org/bugzilla/show_bug.cgi?id=29257 http://www.rtr.com/fp2002disc/_disc2/0a71.htm
