You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by William A Rowe Jr <wr...@rowe-clan.net> on 2016/05/12 14:55:46 UTC
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
On Sun, May 8, 2016 at 8:53 AM, <rj...@apache.org> 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?
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, May 13, 2016 at 7:25 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, May 13, 2016 at 6:57 PM, Yann Ylavic <yl...@gmail.com> 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.
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, May 13, 2016 at 7:25 PM, Yann Ylavic <yl...@gmail.com> 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
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, May 13, 2016 at 6:57 PM, Yann Ylavic <yl...@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 <yl...@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;
_
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
Posted by Yann Ylavic <yl...@gmail.com>.
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 <yl...@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.
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Fri, May 13, 2016 at 11:41 AM, Yann Ylavic <yl...@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.
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
Posted by Yann Ylavic <yl...@gmail.com>.
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().
Re: svn commit: r1742822 - /httpd/httpd/trunk/modules/http/http_core.c
Posted by William A Rowe Jr <wr...@rowe-clan.net>.
On Thu, May 12, 2016 at 9:55 AM, William A Rowe Jr <wr...@rowe-clan.net>
wrote:
> On Sun, May 8, 2016 at 8:53 AM, <rj...@apache.org> 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
>
>
>
>