You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ic...@apache.org on 2022/03/04 08:51:47 UTC
svn commit: r1898586 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/pr65881.txt modules/http2/h2_request.c test/modules/http2/env.py test/modules/http2/htdocs/cgi/hello.py test/modules/http2/test_502_proxy_port.py
Author: icing
Date: Fri Mar 4 08:51:47 2022
New Revision: 1898586
URL: http://svn.apache.org/viewvc?rev=1898586&view=rev
Log:
merge of 1898146,1898173 from trunk:
*) mod_http2: preserve the port number given in a HTTP/1.1
request that was Upgraded to HTTP/2. Fixes PR65881.
Added:
httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
- copied unchanged from r1898146, httpd/httpd/trunk/changes-entries/pr65881.txt
httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
- copied unchanged from r1898146, httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
Modified:
httpd/httpd/branches/2.4.x/ (props changed)
httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
httpd/httpd/branches/2.4.x/test/modules/http2/env.py
httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
Propchange: httpd/httpd/branches/2.4.x/
------------------------------------------------------------------------------
Merged /httpd/httpd/trunk:r1898146,1898173
Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586&r1=1898585&r2=1898586&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
+++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar 4 08:51:47 2022
@@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
return APR_EINVAL;
}
- if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
- apr_port_t defport = apr_uri_port_of_scheme(scheme);
- if (defport != r->server->port) {
- /* port info missing and port is not default for scheme: append */
- authority = apr_psprintf(pool, "%s:%d", authority,
- (int)r->server->port);
+ /* The authority we carry in h2_request is the 'authority' part of
+ * the URL for the request. r->hostname has stripped any port info that
+ * might have been present. Do we need to add it?
+ */
+ if (!ap_strchr_c(authority, ':')) {
+ if (r->parsed_uri.port_str) {
+ /* Yes, it was there, add it again. */
+ authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
+ }
+ else if (!r->parsed_uri.hostname && r->server && r->server->port) {
+ /* If there was no hostname in the parsed URL, the URL was relative.
+ * In that case, we restore port from our server->port, if it
+ * is known and not the default port for the scheme. */
+ apr_port_t defport = apr_uri_port_of_scheme(scheme);
+ if (defport != r->server->port) {
+ /* port info missing and port is not default for scheme: append */
+ authority = apr_psprintf(pool, "%s:%d", authority,
+ (int)r->server->port);
+ }
}
}
Modified: httpd/httpd/branches/2.4.x/test/modules/http2/env.py
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/test/modules/http2/env.py?rev=1898586&r1=1898585&r2=1898586&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/test/modules/http2/env.py (original)
+++ httpd/httpd/branches/2.4.x/test/modules/http2/env.py Fri Mar 4 08:51:47 2022
@@ -86,6 +86,7 @@ class H2TestEnv(HttpdTestEnv):
'AH00135',
'AH02261', # Re-negotiation handshake failed (our test_101)
'AH03490', # scoreboard full, happens on limit tests
+ 'AH01247', # cgi reports sometimes error on accept on cgid socket
])
self.httpd_error_log.add_ignored_patterns([
re.compile(r'.*malformed header from script \'hecho.py\': Bad header: x.*'),
Modified: httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py?rev=1898586&r1=1898585&r2=1898586&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py (original)
+++ httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py Fri Mar 4 08:51:47 2022
@@ -7,6 +7,7 @@ print()
print("{")
print(" \"https\" : \"%s\"," % (os.getenv('HTTPS', '')))
print(" \"host\" : \"%s\"," % (os.getenv('SERVER_NAME', '')))
+print(" \"port\" : \"%s\"," % (os.getenv('SERVER_PORT', '')))
print(" \"protocol\" : \"%s\"," % (os.getenv('SERVER_PROTOCOL', '')))
print(" \"ssl_protocol\" : \"%s\"," % (os.getenv('SSL_PROTOCOL', '')))
print(" \"h2\" : \"%s\"," % (os.getenv('HTTP2', '')))
Re: svn commit: r1898586 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/pr65881.txt modules/http2/h2_request.c test/modules/http2/env.py test/modules/http2/htdocs/cgi/hello.py test/modules/http2/test_502_proxy_port.py
Posted by Stefan Eissing <st...@eissing.org>.
> Am 04.03.2022 um 11:09 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 3/4/22 10:50 AM, Stefan Eissing wrote:
>>
>>
>>> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem <rp...@apache.org>:
>>>
>>>
>>>
>>> On 3/4/22 9:51 AM, icing@apache.org wrote:
>>>> Author: icing
>>>> Date: Fri Mar 4 08:51:47 2022
>>>> New Revision: 1898586
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1898586&view=rev
>>>> Log:
>>>> merge of 1898146,1898173 from trunk:
>>>>
>>>> *) mod_http2: preserve the port number given in a HTTP/1.1
>>>> request that was Upgraded to HTTP/2. Fixes PR65881.
>>>>
>>>>
>>>> Added:
>>>> httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
>>>> - copied unchanged from r1898146, httpd/httpd/trunk/changes-entries/pr65881.txt
>>>> httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
>>>> - copied unchanged from r1898146, httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
>>>> Modified:
>>>> httpd/httpd/branches/2.4.x/ (props changed)
>>>> httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>>> httpd/httpd/branches/2.4.x/test/modules/http2/env.py
>>>> httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
>>>>
>>>> Propchange: httpd/httpd/branches/2.4.x/
>>>> ------------------------------------------------------------------------------
>>>> Merged /httpd/httpd/trunk:r1898146,1898173
>>>>
>>>> Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>>> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586&r1=1898585&r2=1898586&view=diff
>>>> ==============================================================================
>>>> --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
>>>> +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar 4 08:51:47 2022
>>>> @@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
>>>> return APR_EINVAL;
>>>> }
>>>>
>>>> - if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
>>>> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>>> - if (defport != r->server->port) {
>>>> - /* port info missing and port is not default for scheme: append */
>>>> - authority = apr_psprintf(pool, "%s:%d", authority,
>>>> - (int)r->server->port);
>>>> + /* The authority we carry in h2_request is the 'authority' part of
>>>> + * the URL for the request. r->hostname has stripped any port info that
>>>> + * might have been present. Do we need to add it?
>>>> + */
>>>> + if (!ap_strchr_c(authority, ':')) {
>>>> + if (r->parsed_uri.port_str) {
>>>> + /* Yes, it was there, add it again. */
>>>> + authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
>>>> + }
>>>> + else if (!r->parsed_uri.hostname && r->server && r->server->port) {
>>>> + /* If there was no hostname in the parsed URL, the URL was relative.
>>>> + * In that case, we restore port from our server->port, if it
>>>> + * is known and not the default port for the scheme. */
>>>> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>>> + if (defport != r->server->port) {
>>>> + /* port info missing and port is not default for scheme: append */
>>>> + authority = apr_psprintf(pool, "%s:%d", authority,
>>>> + (int)r->server->port);
>>>> + }
>>>> }
>>>> }
>>>
>>> Sorry for my late comment, but I think the above ignores the setting of UseCanonicalPhysicalPort and UseCanonicalName.
>>>
>>> I think we should add what is returned by ap_get_server_port in case this differs from apr_uri_port_of_scheme(scheme)
>>>
>>> I think of something like the below:
>>>
>>> Index: modules/http2/h2_request.c
>>> ===================================================================
>>> --- modules/http2/h2_request.c (revision 1898509)
>>> +++ modules/http2/h2_request.c (working copy)
>>> @@ -95,21 +95,13 @@
>>> * might have been present. Do we need to add it?
>>> */
>>> if (!ap_strchr_c(authority, ':')) {
>>> - if (r->parsed_uri.port_str) {
>>> - /* Yes, it was there, add it again. */
>>> - authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
>>> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> + apr_port_t port = ap_get_server_port(r);
>>> +
>>> + if (defport != port) {
>>> + /* port info missing and port is not default for scheme: append */
>>> + authority = apr_psprintf(pool, "%s:%d", authority, (int)port);
>>> }
>>> - else if (!r->parsed_uri.hostname && r->server && r->server->port) {
>>> - /* If there was no hostname in the parsed URL, the URL was relative.
>>> - * In that case, we restore port from our server->port, if it
>>> - * is known and not the default port for the scheme. */
>>> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> - if (defport != r->server->port) {
>>> - /* port info missing and port is not default for scheme: append */
>>> - authority = apr_psprintf(pool, "%s:%d", authority,
>>> - (int)r->server->port);
>>> - }
>>> - }
>>> }
>>>
>>> req = apr_pcalloc(pool, sizeof(*req));
>>>
>>
>> Like in r1898593 now?
>
> Not completely. UseCanonicalPhysicalPort and UseCanonicalName IMHO aim to give the admin
> control whether he wants to accept user provided input data with regards to hostnames and ports.
> r1898593 uses r->parsed_uri.port_str if present no matter what UseCanonicalPhysicalPort and UseCanonicalName
> say. This is wrong from my point of view, but my point of view might be wrong. Hence let's hear what others say.
Ah, but the upgraded h1 request is converted to a h2 request which will then enter out "normal" request
processing again afterwards. Wouldn't all the directives apply then? I hope so.
>
> Regards
>
> Rüdiger
Re: svn commit: r1898586 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/pr65881.txt modules/http2/h2_request.c test/modules/http2/env.py test/modules/http2/htdocs/cgi/hello.py test/modules/http2/test_502_proxy_port.py
Posted by Ruediger Pluem <rp...@apache.org>.
On 3/4/22 10:50 AM, Stefan Eissing wrote:
>
>
>> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem <rp...@apache.org>:
>>
>>
>>
>> On 3/4/22 9:51 AM, icing@apache.org wrote:
>>> Author: icing
>>> Date: Fri Mar 4 08:51:47 2022
>>> New Revision: 1898586
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1898586&view=rev
>>> Log:
>>> merge of 1898146,1898173 from trunk:
>>>
>>> *) mod_http2: preserve the port number given in a HTTP/1.1
>>> request that was Upgraded to HTTP/2. Fixes PR65881.
>>>
>>>
>>> Added:
>>> httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
>>> - copied unchanged from r1898146, httpd/httpd/trunk/changes-entries/pr65881.txt
>>> httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
>>> - copied unchanged from r1898146, httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
>>> Modified:
>>> httpd/httpd/branches/2.4.x/ (props changed)
>>> httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>> httpd/httpd/branches/2.4.x/test/modules/http2/env.py
>>> httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
>>>
>>> Propchange: httpd/httpd/branches/2.4.x/
>>> ------------------------------------------------------------------------------
>>> Merged /httpd/httpd/trunk:r1898146,1898173
>>>
>>> Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>>> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586&r1=1898585&r2=1898586&view=diff
>>> ==============================================================================
>>> --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
>>> +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar 4 08:51:47 2022
>>> @@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
>>> return APR_EINVAL;
>>> }
>>>
>>> - if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
>>> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> - if (defport != r->server->port) {
>>> - /* port info missing and port is not default for scheme: append */
>>> - authority = apr_psprintf(pool, "%s:%d", authority,
>>> - (int)r->server->port);
>>> + /* The authority we carry in h2_request is the 'authority' part of
>>> + * the URL for the request. r->hostname has stripped any port info that
>>> + * might have been present. Do we need to add it?
>>> + */
>>> + if (!ap_strchr_c(authority, ':')) {
>>> + if (r->parsed_uri.port_str) {
>>> + /* Yes, it was there, add it again. */
>>> + authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
>>> + }
>>> + else if (!r->parsed_uri.hostname && r->server && r->server->port) {
>>> + /* If there was no hostname in the parsed URL, the URL was relative.
>>> + * In that case, we restore port from our server->port, if it
>>> + * is known and not the default port for the scheme. */
>>> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
>>> + if (defport != r->server->port) {
>>> + /* port info missing and port is not default for scheme: append */
>>> + authority = apr_psprintf(pool, "%s:%d", authority,
>>> + (int)r->server->port);
>>> + }
>>> }
>>> }
>>
>> Sorry for my late comment, but I think the above ignores the setting of UseCanonicalPhysicalPort and UseCanonicalName.
>>
>> I think we should add what is returned by ap_get_server_port in case this differs from apr_uri_port_of_scheme(scheme)
>>
>> I think of something like the below:
>>
>> Index: modules/http2/h2_request.c
>> ===================================================================
>> --- modules/http2/h2_request.c (revision 1898509)
>> +++ modules/http2/h2_request.c (working copy)
>> @@ -95,21 +95,13 @@
>> * might have been present. Do we need to add it?
>> */
>> if (!ap_strchr_c(authority, ':')) {
>> - if (r->parsed_uri.port_str) {
>> - /* Yes, it was there, add it again. */
>> - authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
>> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
>> + apr_port_t port = ap_get_server_port(r);
>> +
>> + if (defport != port) {
>> + /* port info missing and port is not default for scheme: append */
>> + authority = apr_psprintf(pool, "%s:%d", authority, (int)port);
>> }
>> - else if (!r->parsed_uri.hostname && r->server && r->server->port) {
>> - /* If there was no hostname in the parsed URL, the URL was relative.
>> - * In that case, we restore port from our server->port, if it
>> - * is known and not the default port for the scheme. */
>> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
>> - if (defport != r->server->port) {
>> - /* port info missing and port is not default for scheme: append */
>> - authority = apr_psprintf(pool, "%s:%d", authority,
>> - (int)r->server->port);
>> - }
>> - }
>> }
>>
>> req = apr_pcalloc(pool, sizeof(*req));
>>
>
> Like in r1898593 now?
Not completely. UseCanonicalPhysicalPort and UseCanonicalName IMHO aim to give the admin
control whether he wants to accept user provided input data with regards to hostnames and ports.
r1898593 uses r->parsed_uri.port_str if present no matter what UseCanonicalPhysicalPort and UseCanonicalName
say. This is wrong from my point of view, but my point of view might be wrong. Hence let's hear what others say.
Regards
Rüdiger
Re: svn commit: r1898586 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/pr65881.txt modules/http2/h2_request.c test/modules/http2/env.py test/modules/http2/htdocs/cgi/hello.py test/modules/http2/test_502_proxy_port.py
Posted by Stefan Eissing <st...@eissing.org>.
> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 3/4/22 9:51 AM, icing@apache.org wrote:
>> Author: icing
>> Date: Fri Mar 4 08:51:47 2022
>> New Revision: 1898586
>>
>> URL: http://svn.apache.org/viewvc?rev=1898586&view=rev
>> Log:
>> merge of 1898146,1898173 from trunk:
>>
>> *) mod_http2: preserve the port number given in a HTTP/1.1
>> request that was Upgraded to HTTP/2. Fixes PR65881.
>>
>>
>> Added:
>> httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
>> - copied unchanged from r1898146, httpd/httpd/trunk/changes-entries/pr65881.txt
>> httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
>> - copied unchanged from r1898146, httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
>> Modified:
>> httpd/httpd/branches/2.4.x/ (props changed)
>> httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>> httpd/httpd/branches/2.4.x/test/modules/http2/env.py
>> httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
>>
>> Propchange: httpd/httpd/branches/2.4.x/
>> ------------------------------------------------------------------------------
>> Merged /httpd/httpd/trunk:r1898146,1898173
>>
>> Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586&r1=1898585&r2=1898586&view=diff
>> ==============================================================================
>> --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
>> +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar 4 08:51:47 2022
>> @@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
>> return APR_EINVAL;
>> }
>>
>> - if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
>> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
>> - if (defport != r->server->port) {
>> - /* port info missing and port is not default for scheme: append */
>> - authority = apr_psprintf(pool, "%s:%d", authority,
>> - (int)r->server->port);
>> + /* The authority we carry in h2_request is the 'authority' part of
>> + * the URL for the request. r->hostname has stripped any port info that
>> + * might have been present. Do we need to add it?
>> + */
>> + if (!ap_strchr_c(authority, ':')) {
>> + if (r->parsed_uri.port_str) {
>> + /* Yes, it was there, add it again. */
>> + authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
>> + }
>> + else if (!r->parsed_uri.hostname && r->server && r->server->port) {
>> + /* If there was no hostname in the parsed URL, the URL was relative.
>> + * In that case, we restore port from our server->port, if it
>> + * is known and not the default port for the scheme. */
>> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
>> + if (defport != r->server->port) {
>> + /* port info missing and port is not default for scheme: append */
>> + authority = apr_psprintf(pool, "%s:%d", authority,
>> + (int)r->server->port);
>> + }
>> }
>> }
>
> Sorry for my late comment, but I think the above ignores the setting of UseCanonicalPhysicalPort and UseCanonicalName.
>
> I think we should add what is returned by ap_get_server_port in case this differs from apr_uri_port_of_scheme(scheme)
>
> I think of something like the below:
>
> Index: modules/http2/h2_request.c
> ===================================================================
> --- modules/http2/h2_request.c (revision 1898509)
> +++ modules/http2/h2_request.c (working copy)
> @@ -95,21 +95,13 @@
> * might have been present. Do we need to add it?
> */
> if (!ap_strchr_c(authority, ':')) {
> - if (r->parsed_uri.port_str) {
> - /* Yes, it was there, add it again. */
> - authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
> + apr_port_t port = ap_get_server_port(r);
> +
> + if (defport != port) {
> + /* port info missing and port is not default for scheme: append */
> + authority = apr_psprintf(pool, "%s:%d", authority, (int)port);
> }
> - else if (!r->parsed_uri.hostname && r->server && r->server->port) {
> - /* If there was no hostname in the parsed URL, the URL was relative.
> - * In that case, we restore port from our server->port, if it
> - * is known and not the default port for the scheme. */
> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
> - if (defport != r->server->port) {
> - /* port info missing and port is not default for scheme: append */
> - authority = apr_psprintf(pool, "%s:%d", authority,
> - (int)r->server->port);
> - }
> - }
> }
>
> req = apr_pcalloc(pool, sizeof(*req));
>
Like in r1898593 now?
Kind Regards,
Stefan
>
> Regards
>
> Rüdiger
>
>
Re: svn commit: r1898586 - in /httpd/httpd/branches/2.4.x: ./ changes-entries/pr65881.txt modules/http2/h2_request.c test/modules/http2/env.py test/modules/http2/htdocs/cgi/hello.py test/modules/http2/test_502_proxy_port.py
Posted by Ruediger Pluem <rp...@apache.org>.
On 3/4/22 9:51 AM, icing@apache.org wrote:
> Author: icing
> Date: Fri Mar 4 08:51:47 2022
> New Revision: 1898586
>
> URL: http://svn.apache.org/viewvc?rev=1898586&view=rev
> Log:
> merge of 1898146,1898173 from trunk:
>
> *) mod_http2: preserve the port number given in a HTTP/1.1
> request that was Upgraded to HTTP/2. Fixes PR65881.
>
>
> Added:
> httpd/httpd/branches/2.4.x/changes-entries/pr65881.txt
> - copied unchanged from r1898146, httpd/httpd/trunk/changes-entries/pr65881.txt
> httpd/httpd/branches/2.4.x/test/modules/http2/test_502_proxy_port.py
> - copied unchanged from r1898146, httpd/httpd/trunk/test/modules/http2/test_502_proxy_port.py
> Modified:
> httpd/httpd/branches/2.4.x/ (props changed)
> httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
> httpd/httpd/branches/2.4.x/test/modules/http2/env.py
> httpd/httpd/branches/2.4.x/test/modules/http2/htdocs/cgi/hello.py
>
> Propchange: httpd/httpd/branches/2.4.x/
> ------------------------------------------------------------------------------
> Merged /httpd/httpd/trunk:r1898146,1898173
>
> Modified: httpd/httpd/branches/2.4.x/modules/http2/h2_request.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/http2/h2_request.c?rev=1898586&r1=1898585&r2=1898586&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/modules/http2/h2_request.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/http2/h2_request.c Fri Mar 4 08:51:47 2022
> @@ -69,12 +69,25 @@ apr_status_t h2_request_rcreate(h2_reque
> return APR_EINVAL;
> }
>
> - if (!ap_strchr_c(authority, ':') && r->server && r->server->port) {
> - apr_port_t defport = apr_uri_port_of_scheme(scheme);
> - if (defport != r->server->port) {
> - /* port info missing and port is not default for scheme: append */
> - authority = apr_psprintf(pool, "%s:%d", authority,
> - (int)r->server->port);
> + /* The authority we carry in h2_request is the 'authority' part of
> + * the URL for the request. r->hostname has stripped any port info that
> + * might have been present. Do we need to add it?
> + */
> + if (!ap_strchr_c(authority, ':')) {
> + if (r->parsed_uri.port_str) {
> + /* Yes, it was there, add it again. */
> + authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
> + }
> + else if (!r->parsed_uri.hostname && r->server && r->server->port) {
> + /* If there was no hostname in the parsed URL, the URL was relative.
> + * In that case, we restore port from our server->port, if it
> + * is known and not the default port for the scheme. */
> + apr_port_t defport = apr_uri_port_of_scheme(scheme);
> + if (defport != r->server->port) {
> + /* port info missing and port is not default for scheme: append */
> + authority = apr_psprintf(pool, "%s:%d", authority,
> + (int)r->server->port);
> + }
> }
> }
Sorry for my late comment, but I think the above ignores the setting of UseCanonicalPhysicalPort and UseCanonicalName.
I think we should add what is returned by ap_get_server_port in case this differs from apr_uri_port_of_scheme(scheme)
I think of something like the below:
Index: modules/http2/h2_request.c
===================================================================
--- modules/http2/h2_request.c (revision 1898509)
+++ modules/http2/h2_request.c (working copy)
@@ -95,21 +95,13 @@
* might have been present. Do we need to add it?
*/
if (!ap_strchr_c(authority, ':')) {
- if (r->parsed_uri.port_str) {
- /* Yes, it was there, add it again. */
- authority = apr_pstrcat(pool, authority, ":", r->parsed_uri.port_str, NULL);
+ apr_port_t defport = apr_uri_port_of_scheme(scheme);
+ apr_port_t port = ap_get_server_port(r);
+
+ if (defport != port) {
+ /* port info missing and port is not default for scheme: append */
+ authority = apr_psprintf(pool, "%s:%d", authority, (int)port);
}
- else if (!r->parsed_uri.hostname && r->server && r->server->port) {
- /* If there was no hostname in the parsed URL, the URL was relative.
- * In that case, we restore port from our server->port, if it
- * is known and not the default port for the scheme. */
- apr_port_t defport = apr_uri_port_of_scheme(scheme);
- if (defport != r->server->port) {
- /* port info missing and port is not default for scheme: append */
- authority = apr_psprintf(pool, "%s:%d", authority,
- (int)r->server->port);
- }
- }
}
req = apr_pcalloc(pool, sizeof(*req));
Regards
Rüdiger