On 6/16/20 9:29 AM, Stefan Eissing wrote:
>> Am 15.06.2020 um 21:46 schrieb Ruediger Pluem <rpl...@apache.org>:
>>
>> 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 :-)
Regards
Rüdiger
>
> - Stefan
>
>> Regards
>>
>> Rüdiger
>>
>> On 6/10/20 4:31 PM, Ruediger Pluem wrote:
>>> Sorry for not testing before and breaking stuff with r1878708. There are
>>> basically two failures here:
>>>
>>> 1. The test failure in t/apache/http_strict.t is because I missed to adjust
>>> the test and I think the following would do
>>> (at least it now passes again):
>>>
>>> Index: t/apache/http_strict.t
>>> ===================================================================
>>> --- t/apache/http_strict.t (revision 1878712)
>>> +++ t/apache/http_strict.t (working copy)
>>> @@ -12,6 +12,10 @@
>>> need_min_apache_fix("2.4.34", "2.5.1") :
>>> need_min_apache_version('2.4.34');
>>>
>>> +my $test_unsupported_version = defined(&need_min_apache_fix) ?
>>> + need_min_apache_fix("2.5.1") :
>>> + need_min_apache_version('2.5.1');
>>> +
>>> # possible expected results:
>>> # 0: any HTTP error
>>> # 1: any HTTP success
>>> @@ -32,7 +36,7 @@
>>> [ "GET\t/ HTTP/1.0\r\n\r\n" => 400],
>>> [ "GET / HTT/1.0\r\n\r\n" => 0],
>>> [ "GET / HTTP/1.0\r\nHost: localhost\r\n\r\n" => 1],
>>> - [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1],
>>> + [ "GET / HTTP/2.0\r\nHost: localhost\r\n\r\n" => 1, 505,
>>> $test_unsupported_version],
>>> [ "GET / HTTP/1.2\r\nHost: localhost\r\n\r\n" => 1],
>>> [ "GET / HTTP/1.11\r\nHost: localhost\r\n\r\n" => 400],
>>> [ "GET / HTTP/10.0\r\nHost: localhost\r\n\r\n" => 400],
>>>
>>>
>>> 2. The test failures in t/modules/http2.t are caused because in
>>> modules/http2/h2_request.c(h2_request_create_rec)
>>> we call ap_parse_request_line with a static 'HTTP/2.0' version string in
>>> order to
>>> 'Validate HTTP/1 request' as the comment states. In practice this request
>>> that we give to ap_parse_request_line
>>> should be a valid HTTP/1.x request as otherwise ap_parse_request_line
>>> could fail. So I though of the following patch
>>> below which makes the tests pass again. I will reset r->proto_num and
>>> r->protocol afterwards to ensure that for
>>> further processing (e.g. env vars, logging) it is clear that this was
>>> actually a HTTP/2.0 request and keep the previous
>>> behavior with regards to this.
>>> Comments or different approaches?
>>>
>>> Index: modules/http2/h2_request.c
>>> ===================================================================
>>> --- modules/http2/h2_request.c (revision 1878470)
>>> +++ modules/http2/h2_request.c (working copy)
>>> @@ -278,7 +278,11 @@ request_rec *h2_request_create_rec(const h2_reques
>>>
>>> /* Time to populate r with the data we have. */
>>> r->request_time = req->request_time;
>>> - r->the_request = apr_psprintf(r->pool, "%s %s HTTP/2.0",
>>> + /*
>>> + * Use HTTP/1.1 as ap_parse_request_line only deals with
>>> + * HTTP/1.x requests.
>>> + */
>>> + r->the_request = apr_psprintf(r->pool, "%s %s HTTP/1.1",
>>> req->method, req->path ? req->path : "");
>>> r->headers_in = apr_table_clone(r->pool, req->headers);
>>>
>>> @@ -293,8 +297,14 @@ request_rec *h2_request_create_rec(const h2_reques
>>> r->per_dir_config = r->server->lookup_defaults;
>>> access_status = r->status;
>>> r->status = HTTP_OK;
>>> + /* Note that this is actually a HTTP/2.0 request */
>>> + r->proto_num = HTTP_VERSION(2,0);
>>> + r->protocol = "HTTP/2.0";
>>> goto die;
>>> }
>>> + /* Note that this is actually a HTTP/2.0 request */
>>> + r->proto_num = HTTP_VERSION(2,0);
>>> + r->protocol = "HTTP/2.0";
>>>
>>> /* we may have switched to another server */
>>> r->per_dir_config = r->server->lookup_defaults;
>>>
>>>
>>> Regards
>>>
>>> Rüdiger
>>>
>>> On 6/10/20 2:46 PM, Travis CI wrote:
>>>> apache
>>>>
>>>> /
>>>>
>>>> httpd
>>>>
>>>> <https://travis-ci.org/github/apache/httpd?utm_medium=notification&utm_source=email>
>>>>
>>>> branch icontrunk <https://github.com/apache/httpd/tree/trunk>
>>>>
>>>> build has failed
>>>> Build #804 was broken
>>>> <https://travis-ci.org/github/apache/httpd/builds/696809088?utm_medium=notification&utm_source=email>
>>>> arrow to build time
>>>> clock icon12 mins and 35 secs
>>>>
>>>> Ruediger Pluem avatarRuediger Pluem
>>>>
>>>> 97bc128 CHANGESET →
>>>> <https://github.com/apache/httpd/compare/75350541f0af...97bc128df241>
>>>>
>>>> * Have the HTTP 0.9 / 1.1 processing code reject requests for
>>>> HTTP >= 2.0 with a HTTP Version Not Support status code.
>>>>
>>>>
>>>> git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1878708
>>>> 13f79535-47bb-0310-9956-ffa450edef68
>>>>
>>>
>
>
