Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Fri, May 13, 2016 at 7:25 PM, Yann Ylavic wrote:
> On Fri, May 13, 2016 at 6:57 PM, Yann Ylavic wrote:
>>
>> But for the above cases or an error while reading/validating the
>> headers or running post_read_request(), we finally call ap_die() or
>> ap_send_error_response().
>> I think we should report SERVER_BUSY_WRITE with r before those calls,
>> and use NULL for SERVER_BUSY_LOG.
>
> For the REQUEST_TIME_OUT case, we may need something like the below too.
> The goal would be to answer something to the client (408) if the
> timeout occurs with some request line byte(s) received (that an
> invalid request but still a request), and report the r (even "NULL"
> for the first empty request on the connection).
Just to be clear, full proposed patch (including yours) attached.
Index: modules/http/http_core.c
===
--- modules/http/http_core.c (revision 1743265)
+++ modules/http/http_core.c (working copy)
@@ -148,10 +148,9 @@ static int ap_process_http_async_connection(conn_r
c->keepalive = AP_CONN_UNKNOWN;
/* process the request if it was read without error */
-ap_update_child_status(c->sbh, SERVER_BUSY_WRITE,
- r->the_request ? r : NULL);
if (r->status == HTTP_OK) {
cs->state = CONN_STATE_HANDLER;
+ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
ap_process_async_request(r);
/* After the call to ap_process_request, the
* request pool may have been deleted. We set
@@ -204,11 +203,10 @@ static int ap_process_http_sync_connection(conn_re
c->keepalive = AP_CONN_UNKNOWN;
/* process the request if it was read without error */
-ap_update_child_status(c->sbh, SERVER_BUSY_WRITE,
- r->the_request ? r : NULL);
if (r->status == HTTP_OK) {
if (cs)
cs->state = CONN_STATE_HANDLER;
+ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
ap_process_request(r);
/* After the call to ap_process_request, the
* request pool will have been deleted. We set
Index: server/protocol.c
===
--- server/protocol.c (revision 1743265)
+++ server/protocol.c (working copy)
@@ -1090,20 +1090,34 @@ request_rec *ap_read_request(conn_rec *conn)
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00566)
"request failed: invalid characters in URI");
}
+ap_update_child_status(conn->sbh, SERVER_BUSY_WRITE, r);
access_status = r->status;
r->status = HTTP_OK;
ap_die(access_status, r);
-ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
+ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, NULL);
ap_run_log_transaction(r);
r = NULL;
apr_brigade_destroy(tmp_bb);
goto traceout;
case HTTP_REQUEST_TIME_OUT:
-ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, NULL);
-if (!r->connection->keepalives)
+if (r->the_request || !r->connection->keepalives) {
+request_rec *log_r = r;
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO()
+ "request failed: client's request-line timed out");
+if (r->the_request) {
+ap_update_child_status(conn->sbh, SERVER_BUSY_WRITE, r);
+access_status = r->status;
+r->status = HTTP_OK;
+ap_die(access_status, r);
+log_r = NULL;
+}
+ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, log_r);
ap_run_log_transaction(r);
-apr_brigade_destroy(tmp_bb);
-goto traceout;
+r = NULL;
+apr_brigade_destroy(tmp_bb);
+goto traceout;
+}
+/* fall through */
default:
apr_brigade_destroy(tmp_bb);
r = NULL;
@@ -1129,8 +1143,9 @@ request_rec *ap_read_request(conn_rec *conn)
if (r->status != HTTP_OK) {
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00567)
"request failed: error reading the headers");
+ap_update_child_status(conn->sbh, SERVER_BUSY_WRITE, r);
ap_send_error_response(r, 0);
-ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
+ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, NULL);
ap_run_log_transaction(r);
apr_brigade_destroy(tmp_bb);
goto traceout;
@@ -1151,8 +1166,9 @@ request_rec *ap_read_request(conn_rec *conn)
"(%s): %s", tenc, r->uri);
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Fri, May 13, 2016 at 7:25 PM, Yann Ylavic wrote:
> +if (r->the_request) {
> +access_status = r->status;
> +r->status = HTTP_OK;
> +ap_update_child_status(conn->sbh, SERVER_BUSY_WRITE, r);
> +ap_die(access_status, r);
> +r = NULL;
> +}
With an "else" here instead of r = NULL above...
> +ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
... so to not segfault below :)
> ap_run_log_transaction(r);
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Fri, May 13, 2016 at 6:57 PM, Yann Ylavic wrote:
> On Fri, May 13, 2016 at 6:49 PM, William A Rowe Jr
> wrote:
>> On Fri, May 13, 2016 at 11:41 AM, Yann Ylavic wrote:
>>>
>>> On Thu, May 12, 2016 at 4:55 PM, William A Rowe Jr
>>> wrote:
>>> >
>>> > I'd explained in another thread this week why this patch is invalid,
>>> > and I've gone ahead and reverted.
>>> >
>>> > We agreed there is a defect here, what about the attached fix?
>>>
>>> Looks good, if something bad happened in ap_read_request() we already
>>> have responded with ap_send_error_response().
>>>
>>> What may be missing is reporting SERVER_BUSY_WRITE (with the
>>> partial/bad request) in ap_send_error_response(), and then we'd
>>> probably don't need to pass r for SERVER_BUSY_LOG in the error paths
>>> of ap_read_request().
>>
>>
>> AIUI, at that point in the code, if there was an error response it was
>> already
>> sent, the other paths appear to be simple disconnection states with no
>> further
>> logging or socket activity, other than forcing lingering close... perhaps.
>
> That's the case where reading the request line fails for anything but
> URI_TOO_LARGE, BAD_REQUEST or REQUEST_TIMEOUT.
>
> But for the above cases or an error while reading/validating the
> headers or running post_read_request(), we finally call ap_die() or
> ap_send_error_response().
> I think we should report SERVER_BUSY_WRITE with r before those calls,
> and use NULL for SERVER_BUSY_LOG.
For the REQUEST_TIME_OUT case, we may need something like the below too.
The goal would be to answer something to the client (408) if the
timeout occurs with some request line byte(s) received (that an
invalid request but still a request), and report the r (even "NULL"
for the first empty request on the connection).
Index: server/protocol.c
===
--- server/protocol.c(revision 1743265)
+++ server/protocol.c(working copy)
@@ -1099,11 +1099,23 @@ request_rec *ap_read_request(conn_rec *conn)
apr_brigade_destroy(tmp_bb);
goto traceout;
case HTTP_REQUEST_TIME_OUT:
-ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, NULL);
-if (!r->connection->keepalives)
+if (r->the_request || !r->connection->keepalives) {
+ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO()
+ "request failed: client's request-line
timed out");
+if (r->the_request) {
+access_status = r->status;
+r->status = HTTP_OK;
+ap_update_child_status(conn->sbh, SERVER_BUSY_WRITE, r);
+ap_die(access_status, r);
+r = NULL;
+}
+ap_update_child_status(conn->sbh, SERVER_BUSY_LOG, r);
ap_run_log_transaction(r);
-apr_brigade_destroy(tmp_bb);
-goto traceout;
+r = NULL;
+apr_brigade_destroy(tmp_bb);
+goto traceout;
+}
+/* fall through */
default:
apr_brigade_destroy(tmp_bb);
r = NULL;
_
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Fri, May 13, 2016 at 6:49 PM, William A Rowe Jr wrote: > On Fri, May 13, 2016 at 11:41 AM, Yann Ylavic wrote: >> >> On Thu, May 12, 2016 at 4:55 PM, William A Rowe Jr >> wrote: >> > >> > I'd explained in another thread this week why this patch is invalid, >> > and I've gone ahead and reverted. >> > >> > We agreed there is a defect here, what about the attached fix? >> >> Looks good, if something bad happened in ap_read_request() we already >> have responded with ap_send_error_response(). >> >> What may be missing is reporting SERVER_BUSY_WRITE (with the >> partial/bad request) in ap_send_error_response(), and then we'd >> probably don't need to pass r for SERVER_BUSY_LOG in the error paths >> of ap_read_request(). > > > AIUI, at that point in the code, if there was an error response it was > already > sent, the other paths appear to be simple disconnection states with no > further > logging or socket activity, other than forcing lingering close... perhaps. That's the case where reading the request line fails for anything but URI_TOO_LARGE, BAD_REQUEST or REQUEST_TIMEOUT. But for the above cases or an error while reading/validating the headers or running post_read_request(), we finally call ap_die() or ap_send_error_response(). I think we should report SERVER_BUSY_WRITE with r before those calls, and use NULL for SERVER_BUSY_LOG.
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Fri, May 13, 2016 at 11:41 AM, Yann Ylavic wrote: > On Thu, May 12, 2016 at 4:55 PM, William A Rowe Jr > wrote: > > > > I'd explained in another thread this week why this patch is invalid, > > and I've gone ahead and reverted. > > > > We agreed there is a defect here, what about the attached fix? > > Looks good, if something bad happened in ap_read_request() we already > have responded with ap_send_error_response(). > > What may be missing is reporting SERVER_BUSY_WRITE (with the > partial/bad request) in ap_send_error_response(), and then we'd > probably don't need to pass r for SERVER_BUSY_LOG in the error paths > of ap_read_request(). > AIUI, at that point in the code, if there was an error response it was already sent, the other paths appear to be simple disconnection states with no further logging or socket activity, other than forcing lingering close... perhaps. In the lingering close phase, we do update the score status, so I don't think that needs to be done in these alternate code paths here.
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Thu, May 12, 2016 at 4:55 PM, William A Rowe Jr wrote: > > I'd explained in another thread this week why this patch is invalid, > and I've gone ahead and reverted. > > We agreed there is a defect here, what about the attached fix? Looks good, if something bad happened in ap_read_request() we already have responded with ap_send_error_response(). What may be missing is reporting SERVER_BUSY_WRITE (with the partial/bad request) in ap_send_error_response(), and then we'd probably don't need to pass r for SERVER_BUSY_LOG in the error paths of ap_read_request().
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Thu, May 12, 2016 at 9:55 AM, William A Rowe Jr
wrote:
> On Sun, May 8, 2016 at 8:53 AM, wrote:
>
>> Author: rjung
>> Date: Sun May 8 13:53:37 2016
>> New Revision: 1742822
>>
>> URL: http://svn.apache.org/viewvc?rev=1742822&view=rev
>> Log:
>> Fix yet another case where we clobber the
>> server-status request info when a timeout happens.
>>
>> Modified:
>> httpd/httpd/trunk/modules/http/http_core.c
>>
>> Modified: httpd/httpd/trunk/modules/http/http_core.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_core.c?rev=1742822&r1=1742821&r2=1742822&view=diff
>>
>> ==
>> --- httpd/httpd/trunk/modules/http/http_core.c (original)
>> +++ httpd/httpd/trunk/modules/http/http_core.c Sun May 8 13:53:37 2016
>> @@ -148,7 +148,8 @@ static int ap_process_http_async_connect
>> c->keepalive = AP_CONN_UNKNOWN;
>> /* process the request if it was read without error */
>>
>> -ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
>> +ap_update_child_status(c->sbh, SERVER_BUSY_WRITE,
>> + r->the_request ? r : NULL);
>> if (r->status == HTTP_OK) {
>> cs->state = CONN_STATE_HANDLER;
>> ap_process_async_request(r);
>> @@ -203,7 +204,8 @@ static int ap_process_http_sync_connecti
>> c->keepalive = AP_CONN_UNKNOWN;
>> /* process the request if it was read without error */
>>
>> -ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
>> +ap_update_child_status(c->sbh, SERVER_BUSY_WRITE,
>> + r->the_request ? r : NULL);
>> if (r->status == HTTP_OK) {
>> if (cs)
>> cs->state = CONN_STATE_HANDLER;
>>
>
> I'd explained in another thread this week why this patch is invalid,
> and I've gone ahead and reverted.
>
> We agreed there is a defect here, what about the attached fix?
>
Reposting, since gmail likes to base64 encode attached patches...
--- modules/http/http_core.c (revision 1743511)
+++ modules/http/http_core.c (working copy)
@@ -148,9 +148,9 @@
c->keepalive = AP_CONN_UNKNOWN;
/* process the request if it was read without error */
-ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
if (r->status == HTTP_OK) {
cs->state = CONN_STATE_HANDLER;
+ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
ap_process_async_request(r);
/* After the call to ap_process_request, the
* request pool may have been deleted. We set
@@ -203,10 +203,10 @@
c->keepalive = AP_CONN_UNKNOWN;
/* process the request if it was read without error */
-ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
if (r->status == HTTP_OK) {
if (cs)
cs->state = CONN_STATE_HANDLER;
+ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
ap_process_request(r);
/* After the call to ap_process_request, the
* request pool will have been deleted. We set
>
>
>
>
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Sun, May 8, 2016 at 8:53 AM, wrote:
> Author: rjung
> Date: Sun May 8 13:53:37 2016
> New Revision: 1742822
>
> URL: http://svn.apache.org/viewvc?rev=1742822&view=rev
> Log:
> Fix yet another case where we clobber the
> server-status request info when a timeout happens.
>
> Modified:
> httpd/httpd/trunk/modules/http/http_core.c
>
> Modified: httpd/httpd/trunk/modules/http/http_core.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_core.c?rev=1742822&r1=1742821&r2=1742822&view=diff
>
> ==
> --- httpd/httpd/trunk/modules/http/http_core.c (original)
> +++ httpd/httpd/trunk/modules/http/http_core.c Sun May 8 13:53:37 2016
> @@ -148,7 +148,8 @@ static int ap_process_http_async_connect
> c->keepalive = AP_CONN_UNKNOWN;
> /* process the request if it was read without error */
>
> -ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
> +ap_update_child_status(c->sbh, SERVER_BUSY_WRITE,
> + r->the_request ? r : NULL);
> if (r->status == HTTP_OK) {
> cs->state = CONN_STATE_HANDLER;
> ap_process_async_request(r);
> @@ -203,7 +204,8 @@ static int ap_process_http_sync_connecti
> c->keepalive = AP_CONN_UNKNOWN;
> /* process the request if it was read without error */
>
> -ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
> +ap_update_child_status(c->sbh, SERVER_BUSY_WRITE,
> + r->the_request ? r : NULL);
> if (r->status == HTTP_OK) {
> if (cs)
> cs->state = CONN_STATE_HANDLER;
>
I'd explained in another thread this week why this patch is invalid,
and I've gone ahead and reverted.
We agreed there is a defect here, what about the attached fix?
Index: modules/http/http_core.c
===
--- modules/http/http_core.c (revision 1743511)
+++ modules/http/http_core.c (working copy)
@@ -148,9 +148,9 @@
c->keepalive = AP_CONN_UNKNOWN;
/* process the request if it was read without error */
-ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
if (r->status == HTTP_OK) {
cs->state = CONN_STATE_HANDLER;
+ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
ap_process_async_request(r);
/* After the call to ap_process_request, the
* request pool may have been deleted. We set
@@ -203,10 +203,10 @@
c->keepalive = AP_CONN_UNKNOWN;
/* process the request if it was read without error */
-ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
if (r->status == HTTP_OK) {
if (cs)
cs->state = CONN_STATE_HANDLER;
+ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, r);
ap_process_request(r);
/* After the call to ap_process_request, the
* request pool will have been deleted. We set
