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

2022-03-04 Thread Stefan Eissing



> Am 04.03.2022 um 11:09 schrieb Ruediger Pluem :
> 
> 
> 
> On 3/4/22 10:50 AM, Stefan Eissing wrote:
>> 
>> 
>>> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem :
>>> 
>>> 
>>> 
>>> On 3/4/22 9:51 AM, ic...@apache.org wrote:
 Author: icing
 Date: Fri Mar  4 08:51:47 2022
 New Revision: 1898586
 
 URL: http://svn.apache.org/viewvc?rev=1898586=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=1898585=1898586=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.
>>> -   

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

2022-03-04 Thread Ruediger Pluem



On 3/4/22 10:50 AM, Stefan Eissing wrote:
> 
> 
>> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem :
>>
>>
>>
>> On 3/4/22 9:51 AM, ic...@apache.org wrote:
>>> Author: icing
>>> Date: Fri Mar  4 08:51:47 2022
>>> New Revision: 1898586
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1898586=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=1898585=1898586=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 = 

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

2022-03-04 Thread Stefan Eissing



> Am 04.03.2022 um 10:22 schrieb Ruediger Pluem :
> 
> 
> 
> On 3/4/22 9:51 AM, ic...@apache.org wrote:
>> Author: icing
>> Date: Fri Mar  4 08:51:47 2022
>> New Revision: 1898586
>> 
>> URL: http://svn.apache.org/viewvc?rev=1898586=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=1898585=1898586=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 

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

2022-03-04 Thread Ruediger Pluem



On 3/4/22 9:51 AM, ic...@apache.org wrote:
> Author: icing
> Date: Fri Mar  4 08:51:47 2022
> New Revision: 1898586
> 
> URL: http://svn.apache.org/viewvc?rev=1898586=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=1898585=1898586=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);
-}
-}
 }