On Fri, Apr 17, 2020 at 11:50 PM Yann Ylavic <[email protected]> wrote:
>
> On Fri, Apr 17, 2020 at 9:43 PM Ruediger Pluem
> >
> > On 4/17/20 3:07 PM, ylavic
> > >
> > > - apr_brigade_destroy(tmp_bb);
> >
> > Why don't we destroy it any longer?
>
> tmp_bb is reused below in the die_early code, and always _cleanup()
> after use in ap_read_request().
> If the brigade is not used anymore, there is no real advantage in
> using _destroy() over _cleanup() IMHO, the former will unregister the
> cleanup callback (thus walk the list) but the callback is a noop after
> apr_brigade_cleanup() anyway..
>
> >
> > > + if (r->status != HTTP_OK) {
> > > + access_status = r->status;
> > > + goto die_early;
> >
> > Why do we die_early here and not just die later?
>
> While both "die_early" and "die" labels finally call ap_die(), the
> former is branched only before we call any hook, thus before anything
> can call ap_die() by itself. It matters because ap_die() can recurse,
> so we should set r->status = HTTP_OK only for the first call.
>
> But thinking more about it, die_early is also cleaning up the input
> filters (for potential further reads to always return APR_EOF), and
> this may be suitable only for the case where we could not read the
> request line or the headers.
> So possibly the code is missing a third label (e.g. die_first) in
> between die_early and die (at r->status = HTTP_OK), which we would
> branch instead of die_early when failing with a "suitable" input state
> (but before post_read_request() hooks still after which we can only
> call ap_die() without resetting r->status).
> Actually it depends on whether (or not) we want ap_die() (i.e.
> ErrorDocument configuration) to be able to read the body at these
> points of failure, and if so we should probably insert the HTTP_IN
> filter earlier too.
>
> WDYT?
The patch would look like the attached. It also renames the labels to
(try to) be more explicit.
Regards,
Yann.
Index: server/protocol.c
===================================================================
--- server/protocol.c (revision 1876675)
+++ server/protocol.c (working copy)
@@ -1461,7 +1461,7 @@ request_rec *ap_read_request(conn_rec *conn)
"request failed: malformed request line");
}
access_status = r->status;
- goto die_early;
+ goto die_unusable_input;
case HTTP_REQUEST_TIME_OUT:
/* Just log, no further action on this connection. */
@@ -1473,7 +1473,7 @@ request_rec *ap_read_request(conn_rec *conn)
/* Not worth dying with. */
conn->keepalive = AP_CONN_CLOSE;
apr_pool_destroy(r->pool);
- goto ignore;
+ goto ignore_request;
}
apr_brigade_cleanup(tmp_bb);
@@ -1497,7 +1497,7 @@ request_rec *ap_read_request(conn_rec *conn)
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00567)
"request failed: error reading the headers");
access_status = r->status;
- goto die_early;
+ goto die_unusable_input;
}
tenc = apr_table_get(r->headers_in, "Transfer-Encoding");
@@ -1513,7 +1513,7 @@ request_rec *ap_read_request(conn_rec *conn)
"client sent unknown Transfer-Encoding "
"(%s): %s", tenc, r->uri);
access_status = HTTP_BAD_REQUEST;
- goto die_early;
+ goto die_unusable_input;
}
/* http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
@@ -1526,9 +1526,19 @@ request_rec *ap_read_request(conn_rec *conn)
}
}
+ /*
+ * Add the HTTP_IN filter here to ensure that ap_discard_request_body
+ * called by ap_die and by ap_send_error_response works correctly on
+ * status codes that do not cause the connection to be dropped and
+ * in situations where the connection should be kept alive.
+ */
+ ap_add_input_filter_handle(ap_http_input_filter_handle,
+ NULL, r, r->connection);
+
+ /* Validate Host/Expect headers and select vhost. */
if (!ap_check_request_header(r)) {
access_status = r->status;
- goto die_early;
+ goto die_before_hooks;
}
/* we may have switched to another server */
@@ -1542,17 +1552,8 @@ request_rec *ap_read_request(conn_rec *conn)
cur_timeout = r->server->timeout;
}
- /*
- * Add the HTTP_IN filter here to ensure that ap_discard_request_body
- * called by ap_die and by ap_send_error_response works correctly on
- * status codes that do not cause the connection to be dropped and
- * in situations where the connection should be kept alive.
- */
- ap_add_input_filter_handle(ap_http_input_filter_handle,
- NULL, r, r->connection);
-
if ((access_status = ap_run_post_read_request(r))) {
- goto die;
+ goto die_after_hooks;
}
AP_READ_REQUEST_SUCCESS((uintptr_t)r, (char *)r->method,
@@ -1560,7 +1561,8 @@ request_rec *ap_read_request(conn_rec *conn)
r->status);
return r;
-die_early:
+ /* Everything falls through on failure */
+die_unusable_input:
/* Input filters are in an undeterminate state, cleanup (including
* CORE_IN's socket) such that any further attempt to read is EOF.
*/
@@ -1579,9 +1581,11 @@ request_rec *ap_read_request(conn_rec *conn)
conn->keepalive = AP_CONN_CLOSE;
}
- /* fallthru ap_die() (non recursive) */
+die_before_hooks:
+ /* First call to ap_die() (non recursive) */
r->status = HTTP_OK;
-die:
+
+die_after_hooks:
ap_die(access_status, r);
/* ap_die() sent the response through the output filters, we must now
@@ -1593,8 +1597,7 @@ request_rec *ap_read_request(conn_rec *conn)
ap_pass_brigade(conn->output_filters, tmp_bb);
ap_release_brigade(conn, tmp_bb);
- /* fallthru */
-ignore:
+ignore_request:
r = NULL;
AP_READ_REQUEST_FAILURE((uintptr_t)r);
return NULL;
