On 6/17/20 3:11 PM, Ruediger Pluem wrote:
>
>
> On 6/17/20 11:52 AM, Yann Ylavic wrote:
>> On Wed, Jun 17, 2020 at 10:43 AM Joe Orton <jor...@redhat.com> wrote:
>>>
>>>> And yes this makes me think if this kind of fake really makes sense. The
>>>> need to reset 3 request_rec fields after
>>>> ap_parse_request_line doesn't sound good.
>>
>> At this point it isn't an HTTP/2 request IMHO, it's an HTTP/1 request
>> extracted from an h2 stream.
>> I suppose that using r->protocol = HTTP/2.0 is for (custom-)logging
>> purpose, or is it specified? Stefan?
>>
>>>> OTOH calling ap_parse_request_line makes it possible to apply the same
>>>> policies to
>>>> HTTP/2.0 and HTTP/1.x requests at least where they overlap. Maybe the
>>>> change to the API is the way out.
>>>> Keen to see Yann's feedback on this :-)
>>
>> I think we should not mark h2 to h1 requests using r->protocol, it's
>> confusing whereas it somehow should be transparent for h1
>> core/modules.
>> If something really needs to know, at worst r->proto_num could do, but
>> I'd prefer a separate field or notes. If it's only for logging,
>> possibly we could override that at a later stage/hook?
>> As seen here, they really are not h2 requests as for h1 core/modules
>> which actually handle them.
>>
>>>
>>> I'm not Yann but my 2c is that ap_parse_request_line() is designed to
>>> safely parse an HTTP/1.x request-line off the wire and probably
>>> shouldn't be used in mod_h2. The complexity of that function is dealing
>>> with all the error cases, which is not useful since none of them will be
>>> hit with HTTP/2.
>>
>> I'm not sure, what guarantees otherwise that the method and URI-path
>> extracted from HTTP/2 (i.e. in h2_request_add_header) are valid?
>
> I think both views are somehow correct. I think some of the checks in
> ap_parse_request_line() are
> not needed for these HTTP/1 requests extracted from a HTTP/2 stream. Others
> could be quite useful like
> correct URI encoding or checking that only registered methods are used and we
> avoid code duplication
> here. So what I can think of here is that we either split
> ap_parse_request_line even further
> in tests useful only for the line from the wire in the HTTP/1 case and the
> checks useful for the HTTP/2
> use case as well and have HTTP/1 use both and HTTP/2 only one or we supply
> the expected HTTP version
> to ap_parse_request_line and behave accordingly.
Even with r1878938 and r1878939 now in place and trunk back to green I think no
final decision on the
approach is done. But after r1878926 I wanted to fix the shortcomings of my own
initial proposal
in order to have a complete approach in trunk. I am happy to go a different way
if this is thought to be
better.
Regards
Rüdiger
