On Tue, Sep 7, 2021 at 3:05 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 9/7/21 2:18 PM, Yann Ylavic wrote:
> > Index: server/protocol.c
> > ===================================================================
> > --- server/protocol.c (revision 1893001)
> > +++ server/protocol.c (working copy)
> > @@ -716,6 +716,13 @@ static int read_request_line(request_rec *r, apr_b
> > if (rv != APR_SUCCESS) {
> > r->request_time = apr_time_now();
> >
> > + /* Fall through with an invalid (non NULL) request */
> > + r->method = "-";
>
> In line 1484 of server/protocol.c we check for r->method being NULL to
> determine and log that we had a malformed request line
> (AH00566). Hence this line would need to be adjusted as well.
Good catch, I will fix it.
>
> > + r->method_number = M_INVALID;
> > + r->uri = r->unparsed_uri = apr_pstrdup(r->pool, "-");
>
> I would leave this NULL as in the normal path we don't need it and thus we
> are wasting pool memory with each request.
In the "spirit" of not leaving NULLs on error for (eventually
third-party-)modules hooks/filters, this looks sensible to me. Also
r->pool is in its very youth here, probably not near the 8K boundary
so these two bytes are almost free..
Not a strong opinion though, it looks to me like r->[unparsed_]uri
could be accessed/logged from anywhere, and we call ap_die() in more
cases than we did two or three releases ago (since the error handling
changes in ap_read_request(), can't remember which version it was).
Thanks;
Yann.
