I just read ap_parse_request_line() again. It seems to consist of 2 parts: 1. the parsing of the text line into protocol/method/uri bits with error checks 2. the check on method/uri correctness in combination with some protocol version specific checks
I think the 2nd part is vital to run for every HTTP version as it concerns itself with "pure" HTTP semantics (which method is known and allowed, which uri values do we accept for processing). "Faking" a request line just for running the 2nd part, well...it's a bit ugly. Can we not split the method after line 904? If not, I'd rather copy the assignments and applicable checks into mod_h2. Cheers, Stefan PS. There is a basic theme here where the server, mainly for historical and understandable reasons, assumes HTTP == HTTP/1.x with some special glue for the honorable HTTP/0.9 ancestor. There is architecture for other protocols, but not for different http protocols. The separation between HTTP semantics and HTTP transports. Another example is ap_remove_output_filter_byhandle(r->output_filters, "HTTP_HEADER"); in h2_h2.c:#728 where mod_h2 needs to prevent the server of serializing the response headers in HTTP/1.x format. This is ap_http_header_filter() from modules/http/http_filters.c which also does HTTP generic checks (for example permissible headers in a 304 response) *and* puts the response headers onto the output brigade in HTTP/1.1 format. The first half of that h1 filter is duplicated into h2_filter_headers_out() from h2_from_h1.c#525, submethod create_response(), line 171. I hope they did not drift too much apart over the years. > Am 17.06.2020 um 10:43 schrieb Joe Orton <[email protected]>: > > On Tue, Jun 16, 2020 at 12:08:41PM +0200, Ruediger Pluem wrote: >> >> >> On 6/16/20 9:29 AM, Stefan Eissing wrote: >>>> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <[email protected]>: >>>> >>>> I would like to unblock the test failure on trunk soon. Any comments on >>>> the below? >>> >>> I am not really familiar with ap_parse_request_line() and why it was added >>> there. Yann? >>> >>> As to "faking" the http version of a request, that does not look good. Do >>> we need to preserve the fairy tale for our code that everything is HTTP/1.x? >> >> I think the point is that ap_parse_request_line is only designed to parse <= >> HTTP/1.x requests, if it should parse >= HTTP/2.0 >> I guess we need to adjust the API such that we can tell it which major and >> probably minor version to consider. >> But looking at the code again that brought me to the point that I would need >> to reset r->the_request to >> >> r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0", req->method, >> req->path ? req->path : ""); >> >> after the ap_parse_request_line() call. >> 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. 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'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. > > It looks cheaper - and not too complex - to set the fields of the > request_rec directly as appropriate for HTTP/2 as > ap_parse_request_line() does for HTTP/1.1? > > Regards, Joe
