On Fri, May 13, 2016 at 6:57 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
> On Fri, May 13, 2016 at 6:49 PM, William A Rowe Jr <wr...@rowe-clan.net>
> wrote:
>> On Fri, May 13, 2016 at 11:41 AM, Yann Ylavic <ylavic....@gmail.com> wrote:
>>>
>>> On Thu, May 12, 2016 at 4:55 PM, William A Rowe Jr <wr...@rowe-clan.net>
>>> 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;
_
