> Am 17.06.2020 um 21:19 schrieb Ruediger Pluem <rpl...@apache.org>: > > > > 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.
Thanks, Rüdiger. After all this, I think ap_parse_request_line() should not balk at higher HTTP versions. Or being split into a HTTP/1.x part and HTTP. > > Regards > > Rüdiger
