On 6/17/20 11:52 AM, Yann Ylavic wrote:
> On Wed, Jun 17, 2020 at 10:43 AM Joe Orton <[email protected]> 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.
Regards
Rüdiger
