On 9/7/21 4:04 PM, Yann Ylavic wrote:
> 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).
Another thought that came up with me in a similar fashion like AH00566: Are we
sure that that r->uri == r->unparsed_uri == NULL is
not used somewhere as an error check? Furthermore if r->uri != NULL doesn't the
code elsewhere assume that is does contain a valid
path starting with '/' ?
Regards
Rüdiger
