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?
>
> > @@ -1498,10 +1478,23 @@ request_rec *ap_read_request(conn_rec *c
> > * HTTP/1.1 mentions twice (S9, S14.23) that a request MUST contain
> > * a Host: header, and the server MUST respond with 400 if it
> > doesn't.
> > */
> > - access_status = HTTP_BAD_REQUEST;
> > ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00569)
> > "client sent HTTP/1.1 request without hostname "
> > "(see RFC2616 section 14.23): %s", r->uri);
> > + access_status = HTTP_BAD_REQUEST;
> > + goto die_early;
>
> Why do we die_early here and not just die later?
Same here.
Regards,
Yann.
