You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2022/04/04 13:43:48 UTC
Re: svn commit: r1899550 - in /httpd/httpd/trunk: include/ap_mmn.h include/http_protocol.h modules/http/http_protocol.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
On 4/4/22 11:41 AM, icing@apache.org wrote:
> Author: icing
> Date: Mon Apr 4 09:41:25 2022
> New Revision: 1899550
>
> URL: http://svn.apache.org/viewvc?rev=1899550&view=rev
> Log:
> *) core: add ap_h1_append_header() for single header values.
> *) mod_proxy: use of new ap_h1_header(s) functions for
> formatting HTTP/1.1 requests.
>
>
> Modified:
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/include/http_protocol.h
> httpd/httpd/trunk/modules/http/http_protocol.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1899550&r1=1899549&r2=1899550&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Apr 4 09:41:25 2022
> @@ -3913,28 +3910,51 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
> ap_xlate_proto_to_ascii(buf, strlen(buf));
> e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(header_brigade, e);
> +
> + /*
> + * Make a copy on r->headers_in for the request we make to the backend.
> + * This we modify according to our configuration and connection handling.
> + * Leave the original headers we received from the client untouched.
> + *
> + * Note: We need to take r->pool for apr_table_copy as the key / value
> + * pairs in r->headers_in have been created out of r->pool and
> + * p might be (and actually is) a longer living pool.
Hm. I found two calls to ap_proxy_create_hdrbr in mod_proxy_http.c and mod_proxy_wstunnel.c and both seem to pass r->pool as pool p.
There is currently no documentation on how p relates to r->pool.
But I agree with your further comments that we should allocate further headers from r->pool to be consistent in the table and
to use r->pool for the table copy.
> + * This would trigger the bad pool ancestry abort in apr_table_copy if
> + * apr is compiled with APR_POOL_DEBUG.
> + *
> + * icing: if p indeed lives longer than r->pool, we should allocate
> + * all new header values from r->pool as well and avoid leakage.
> + */
> + request_headers = apr_table_copy(r->pool, r->headers_in);
> +
> + /* We used to send `Host: ` always first, so let's keep it that
> + * way. No telling which legacy backend is relying no this.
> + */
> if (dconf->preserve_host == 0) {
> + const char *nhost;
> if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address */
> if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> - buf = apr_pstrcat(p, "Host: [", uri->hostname, "]:",
> - uri->port_str, CRLF, NULL);
> + nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]:",
> + uri->port_str, NULL);
> } else {
> - buf = apr_pstrcat(p, "Host: [", uri->hostname, "]", CRLF, NULL);
> + nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]", NULL);
> }
> } else {
> if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
> - buf = apr_pstrcat(p, "Host: ", uri->hostname, ":",
> - uri->port_str, CRLF, NULL);
> + nhost = apr_pstrcat(r->pool, uri->hostname, ":",
> + uri->port_str, NULL);
> } else {
> - buf = apr_pstrcat(p, "Host: ", uri->hostname, CRLF, NULL);
> + nhost = uri->hostname;
> }
> }
> + ap_h1_append_header(header_brigade, r->pool, "Host", nhost);
> + apr_table_unset(request_headers, "Host");
> }
> else {
> /* don't want to use r->hostname, as the incoming header might have a
> * port attached
> */
> - const char* hostname = apr_table_get(r->headers_in,"Host");
> + const char* hostname = apr_table_get(request_headers, "Host");
> if (!hostname) {
> hostname = r->server->server_hostname;
> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01092)
Regards
Rüdiger
Re: svn commit: r1899550 - in /httpd/httpd/trunk: include/ap_mmn.h include/http_protocol.h modules/http/http_protocol.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c
Posted by Stefan Eissing <st...@eissing.org>.
> Am 04.04.2022 um 15:43 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 4/4/22 11:41 AM, icing@apache.org wrote:
>> Author: icing
>> Date: Mon Apr 4 09:41:25 2022
>> New Revision: 1899550
>>
>> URL: http://svn.apache.org/viewvc?rev=1899550&view=rev
>> Log:
>> *) core: add ap_h1_append_header() for single header values.
>> *) mod_proxy: use of new ap_h1_header(s) functions for
>> formatting HTTP/1.1 requests.
>>
>>
>> Modified:
>> httpd/httpd/trunk/include/ap_mmn.h
>> httpd/httpd/trunk/include/http_protocol.h
>> httpd/httpd/trunk/modules/http/http_protocol.c
>> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
>> httpd/httpd/trunk/modules/proxy/proxy_util.c
>>
>
>> Modified: httpd/httpd/trunk/modules/proxy/proxy_util.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?rev=1899550&r1=1899549&r2=1899550&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/proxy/proxy_util.c (original)
>> +++ httpd/httpd/trunk/modules/proxy/proxy_util.c Mon Apr 4 09:41:25 2022
>
>> @@ -3913,28 +3910,51 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbr
>> ap_xlate_proto_to_ascii(buf, strlen(buf));
>> e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc);
>> APR_BRIGADE_INSERT_TAIL(header_brigade, e);
>> +
>> + /*
>> + * Make a copy on r->headers_in for the request we make to the backend.
>> + * This we modify according to our configuration and connection handling.
>> + * Leave the original headers we received from the client untouched.
>> + *
>> + * Note: We need to take r->pool for apr_table_copy as the key / value
>> + * pairs in r->headers_in have been created out of r->pool and
>> + * p might be (and actually is) a longer living pool.
>
> Hm. I found two calls to ap_proxy_create_hdrbr in mod_proxy_http.c and mod_proxy_wstunnel.c and both seem to pass r->pool as pool p.
> There is currently no documentation on how p relates to r->pool.
> But I agree with your further comments that we should allocate further headers from r->pool to be consistent in the table and
> to use r->pool for the table copy.
Thanks, it was a bit of mystery to me, too. Tried to preserver what I found in code and comments.
>
>> + * This would trigger the bad pool ancestry abort in apr_table_copy if
>> + * apr is compiled with APR_POOL_DEBUG.
>> + *
>> + * icing: if p indeed lives longer than r->pool, we should allocate
>> + * all new header values from r->pool as well and avoid leakage.
>> + */
>> + request_headers = apr_table_copy(r->pool, r->headers_in);
>> +
>> + /* We used to send `Host: ` always first, so let's keep it that
>> + * way. No telling which legacy backend is relying no this.
>> + */
>> if (dconf->preserve_host == 0) {
>> + const char *nhost;
>> if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 address */
>> if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
>> - buf = apr_pstrcat(p, "Host: [", uri->hostname, "]:",
>> - uri->port_str, CRLF, NULL);
>> + nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]:",
>> + uri->port_str, NULL);
>> } else {
>> - buf = apr_pstrcat(p, "Host: [", uri->hostname, "]", CRLF, NULL);
>> + nhost = apr_pstrcat(r->pool, "[", uri->hostname, "]", NULL);
>> }
>> } else {
>> if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) {
>> - buf = apr_pstrcat(p, "Host: ", uri->hostname, ":",
>> - uri->port_str, CRLF, NULL);
>> + nhost = apr_pstrcat(r->pool, uri->hostname, ":",
>> + uri->port_str, NULL);
>> } else {
>> - buf = apr_pstrcat(p, "Host: ", uri->hostname, CRLF, NULL);
>> + nhost = uri->hostname;
>> }
>> }
>> + ap_h1_append_header(header_brigade, r->pool, "Host", nhost);
>> + apr_table_unset(request_headers, "Host");
>> }
>> else {
>> /* don't want to use r->hostname, as the incoming header might have a
>> * port attached
>> */
>> - const char* hostname = apr_table_get(r->headers_in,"Host");
>> + const char* hostname = apr_table_get(request_headers, "Host");
>> if (!hostname) {
>> hostname = r->server->server_hostname;
>> ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01092)
>
> Regards
>
> Rüdiger