On Fri, May 13, 2016 at 7:25 PM, Yann Ylavic <[email protected]> wrote:
> On Fri, May 13, 2016 at 6:57 PM, Yann Ylavic <[email protected]> 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);
r->status = HTTP_BAD_REQUEST;
conn->keepalive = AP_CONN_CLOSE;
+ 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;
@@ -1179,8 +1195,9 @@ request_rec *ap_read_request(conn_rec *conn)
r->uri);
r->header_only = 0;
r->status = HTTP_BAD_REQUEST;
+ 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;
@@ -1234,8 +1251,9 @@ request_rec *ap_read_request(conn_rec *conn)
if (access_status != HTTP_OK
|| (access_status = ap_run_post_read_request(r))) {
+ ap_update_child_status(conn->sbh, SERVER_BUSY_WRITE, r);
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;
goto traceout;
@@ -1261,8 +1279,9 @@ request_rec *ap_read_request(conn_rec *conn)
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(00570)
"client sent an unrecognized expectation value "
"of Expect: %s", expect);
+ 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);
goto traceout;
} else {
