You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2021/09/03 16:52:39 UTC
svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./
changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c
modules/proxy/proxy_util.c
Author: ylavic
Date: Fri Sep 3 16:52:38 2021
New Revision: 1892874
URL: http://svn.apache.org/viewvc?rev=1892874&view=rev
Log:
Merge r1892814, r1892853 from trunk:
mod_proxy: Faster unix socket path parsing in the "proxy:" URL.
The actual r->filename format is "[proxy:]unix:path|url" for UDS, no need to
strstr(,"unix:") since it's at the start of the string.
mod_proxy: Follow up to r1892814.
Save some few cycles in ap_proxy_de_socketfy() too.
Submitted by: ylavic
Reviewed by: ylavic, covener, rpluem
Added:
httpd/httpd/branches/2.4.x/changes-entries/fix_uds_filename.txt
- copied unchanged from r1892814, httpd/httpd/trunk/changes-entries/fix_uds_filename.txt
Modified:
httpd/httpd/branches/2.4.x/ (props changed)
httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
Propchange: httpd/httpd/branches/2.4.x/
------------------------------------------------------------------------------
Merged /httpd/httpd/trunk:r1892814,1892853
Modified: httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c?rev=1892874&r1=1892873&r2=1892874&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c (original)
+++ httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c Fri Sep 3 16:52:38 2021
@@ -1975,7 +1975,7 @@ PROXY_DECLARE(const char *) ap_proxy_de_
* the UDS path... ignore it
*/
if (!ap_cstr_casecmpn(url, "unix:", 5) &&
- ((ptr = ap_strchr_c(url, '|')) != NULL)) {
+ ((ptr = ap_strchr_c(url + 5, '|')) != NULL)) {
/* move past the 'unix:...|' UDS path info */
const char *ret, *c;
Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1892874&r1=1892873&r2=1892874&view=diff
==============================================================================
--- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
+++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Sep 3 16:52:38 2021
@@ -2281,8 +2281,8 @@ static void fix_uds_filename(request_rec
if (!r || !r->filename) return;
if (!strncmp(r->filename, "proxy:", 6) &&
- (ptr2 = ap_strcasestr(r->filename, "unix:")) &&
- (ptr = ap_strchr(ptr2, '|'))) {
+ !ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
+ (ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
apr_uri_t urisock;
apr_status_t rv;
*ptr = '\0';
Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./
changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c
modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 9/6/21 12:28 PM, Yann Ylavic wrote:
> On Mon, Sep 6, 2021 at 12:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
!ap_cstr_casecmpn(uds_url, "unix:", 5) &&
>>
>> Why doing case insensitive checks here? Shouldn't we insist on case here and use strncmp ?
>
> Yes, correct, spurious change.
>
>>> + {
>>> + apr_table_setn(r->notes, "uds_path", uds_path);
>>> +
>>> + /* Remove the UDS path from *url and r->filename */
>>> + url_len = strlen(origin_url);
>>> + *url = apr_pstrmemdup(r->pool, origin_url, url_len);
>>> + memcpy(uds_url, *url, url_len + 1);
>>
>> With a short uds path and a long origin_url couldn't this be overlapping?
>> I think memcpy is unsafe with overlapping memory and we should stay with memmove.
>
> *url is newly allocated here, so there is no possible overlap with the
> initial r->filename.
Good point. I missed this.
Regards
Rüdiger
Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./
changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Sep 6, 2021 at 12:16 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 9/6/21 11:38 AM, Yann Ylavic wrote:
> > Index: modules/proxy/proxy_util.c
> > ===================================================================
> > --- modules/proxy/proxy_util.c (revision 1892971)
> > +++ modules/proxy/proxy_util.c (working copy)
> > @@ -2268,33 +2268,45 @@ static int ap_proxy_retry_worker(const char *proxy
> > * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
> > * as required.
> > */
> > -static void fix_uds_filename(request_rec *r, char **url)
> > +static int fix_uds_filename(request_rec *r, char **url)
> > {
> > - char *ptr, *ptr2;
> > - if (!r || !r->filename) return;
> > + char *uds_url = r->filename + 6, *origin_url;
> >
> > - if (!strncmp(r->filename, "proxy:", 6) &&
> > - !ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> > - (ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
> > + if (!ap_cstr_casecmpn(r->filename, "proxy:", 6) &&
> > + !ap_cstr_casecmpn(uds_url, "unix:", 5) &&
>
> Why doing case insensitive checks here? Shouldn't we insist on case here and use strncmp ?
Yes, correct, spurious change.
>
> > + (origin_url = ap_strchr(uds_url + 5, '|'))) {
> > + char *uds_path = NULL;
> > + apr_size_t url_len;
> > apr_uri_t urisock;
> > apr_status_t rv;
> > - *ptr = '\0';
> > - rv = apr_uri_parse(r->pool, ptr2, &urisock);
> > - if (rv == APR_SUCCESS) {
> > - char *rurl = ptr+1;
> > - char *sockpath = ap_runtime_dir_relative(r->pool, urisock.path);
> > - apr_table_setn(r->notes, "uds_path", sockpath);
> > - *url = apr_pstrdup(r->pool, rurl); /* so we get the scheme for the uds */
> > - /* r->filename starts w/ "proxy:", so add after that */
> > - memmove(r->filename+6, rurl, strlen(rurl)+1);
> > +
> > + *origin_url = '\0';
> > + rv = apr_uri_parse(r->pool, uds_url, &urisock);
> > + *origin_url++ = '|';
> > +
> > + if (rv == APR_SUCCESS && urisock.path && !urisock.hostname) {
> > + uds_path = ap_runtime_dir_relative(r->pool, urisock.path);
> > + }
> > + if (!uds_path) {
> > + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
> > + "Invalid proxy UDS filename (%s)",
> > + r->filename);
> > + return 0;
> > + }
> > + {
> > + apr_table_setn(r->notes, "uds_path", uds_path);
> > +
> > + /* Remove the UDS path from *url and r->filename */
> > + url_len = strlen(origin_url);
> > + *url = apr_pstrmemdup(r->pool, origin_url, url_len);
> > + memcpy(uds_url, *url, url_len + 1);
>
> With a short uds path and a long origin_url couldn't this be overlapping?
> I think memcpy is unsafe with overlapping memory and we should stay with memmove.
*url is newly allocated here, so there is no possible overlap with the
initial r->filename.
Regards;
Yann.
Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./
changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c
modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 9/6/21 11:38 AM, Yann Ylavic wrote:
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1892971)
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -2268,33 +2268,45 @@ static int ap_proxy_retry_worker(const char *proxy
> * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
> * as required.
> */
> -static void fix_uds_filename(request_rec *r, char **url)
> +static int fix_uds_filename(request_rec *r, char **url)
> {
> - char *ptr, *ptr2;
> - if (!r || !r->filename) return;
> + char *uds_url = r->filename + 6, *origin_url;
>
> - if (!strncmp(r->filename, "proxy:", 6) &&
> - !ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> - (ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
> + if (!ap_cstr_casecmpn(r->filename, "proxy:", 6) &&
> + !ap_cstr_casecmpn(uds_url, "unix:", 5) &&
Why doing case insensitive checks here? Shouldn't we insist on case here and use strncmp ?
> + (origin_url = ap_strchr(uds_url + 5, '|'))) {
> + char *uds_path = NULL;
> + apr_size_t url_len;
> apr_uri_t urisock;
> apr_status_t rv;
> - *ptr = '\0';
> - rv = apr_uri_parse(r->pool, ptr2, &urisock);
> - if (rv == APR_SUCCESS) {
> - char *rurl = ptr+1;
> - char *sockpath = ap_runtime_dir_relative(r->pool, urisock.path);
> - apr_table_setn(r->notes, "uds_path", sockpath);
> - *url = apr_pstrdup(r->pool, rurl); /* so we get the scheme for the uds */
> - /* r->filename starts w/ "proxy:", so add after that */
> - memmove(r->filename+6, rurl, strlen(rurl)+1);
> +
> + *origin_url = '\0';
> + rv = apr_uri_parse(r->pool, uds_url, &urisock);
> + *origin_url++ = '|';
> +
> + if (rv == APR_SUCCESS && urisock.path && !urisock.hostname) {
> + uds_path = ap_runtime_dir_relative(r->pool, urisock.path);
> + }
> + if (!uds_path) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO()
> + "Invalid proxy UDS filename (%s)",
> + r->filename);
> + return 0;
> + }
> + {
> + apr_table_setn(r->notes, "uds_path", uds_path);
> +
> + /* Remove the UDS path from *url and r->filename */
> + url_len = strlen(origin_url);
> + *url = apr_pstrmemdup(r->pool, origin_url, url_len);
> + memcpy(uds_url, *url, url_len + 1);
With a short uds path and a long origin_url couldn't this be overlapping?
I think memcpy is unsafe with overlapping memory and we should stay with memmove.
> +
> ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> "*: rewrite of url due to UDS(%s): %s (%s)",
> - sockpath, *url, r->filename);
> + uds_path, *url, r->filename);
> }
> - else {
> - *ptr = '|';
> - }
> }
> + return 1;
> }
>
Regards
Rüdiger
Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./
changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Sep 3, 2021 at 10:11 PM Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> On 9/3/21 6:52 PM, ylavic@apache.org wrote:
> >
> > if (!strncmp(r->filename, "proxy:", 6) &&
> > - (ptr2 = ap_strcasestr(r->filename, "unix:")) &&
> > - (ptr = ap_strchr(ptr2, '|'))) {
> > + !ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> > + (ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
>
> I know that I voted for it to be backported, but after rethinking this shouldn't this be
>
> (ptr2 = r->filename + 6, ptr = ap_strchr(ptr2 + 5, '|')))
>
> as ptr2 is later used in
>
> rv = apr_uri_parse(r->pool, ptr2, &urisock);
>
> With the old code and with my proposal above ptr2 points to the 'u' of 'unix:' with the current code it points to the char after
> 'unix:' likely //.
An UDS url in r->filename should be of the form
"proxy:unix:/path/to/socket|https://www.example.net/...", configured
either from a ProxyPass, a SetHandler or a RewriteRule [P]. So ptr2
should point to a path (absolute or relative to ServerRoot), not an
authority starting with "//".
But if we want a better validation at fix_uds_filename() level, maybe
something like the attached patch could do?
Regards;
Yann.
Re: svn commit: r1892874 - in /httpd/httpd/branches/2.4.x: ./
changes-entries/fix_uds_filename.txt modules/proxy/mod_proxy.c
modules/proxy/proxy_util.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 9/3/21 6:52 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Fri Sep 3 16:52:38 2021
> New Revision: 1892874
>
> URL: http://svn.apache.org/viewvc?rev=1892874&view=rev
> Log:
> Merge r1892814, r1892853 from trunk:
>
> mod_proxy: Faster unix socket path parsing in the "proxy:" URL.
>
> The actual r->filename format is "[proxy:]unix:path|url" for UDS, no need to
> strstr(,"unix:") since it's at the start of the string.
>
>
> mod_proxy: Follow up to r1892814.
>
> Save some few cycles in ap_proxy_de_socketfy() too.
>
>
> Submitted by: ylavic
> Reviewed by: ylavic, covener, rpluem
>
> Added:
> httpd/httpd/branches/2.4.x/changes-entries/fix_uds_filename.txt
> - copied unchanged from r1892814, httpd/httpd/trunk/changes-entries/fix_uds_filename.txt
> Modified:
> httpd/httpd/branches/2.4.x/ (props changed)
> httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
> httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
>
> Propchange: httpd/httpd/branches/2.4.x/
> ------------------------------------------------------------------------------
> Merged /httpd/httpd/trunk:r1892814,1892853
>
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c?rev=1892874&r1=1892873&r2=1892874&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/mod_proxy.c Fri Sep 3 16:52:38 2021
> @@ -1975,7 +1975,7 @@ PROXY_DECLARE(const char *) ap_proxy_de_
> * the UDS path... ignore it
> */
> if (!ap_cstr_casecmpn(url, "unix:", 5) &&
> - ((ptr = ap_strchr_c(url, '|')) != NULL)) {
> + ((ptr = ap_strchr_c(url + 5, '|')) != NULL)) {
> /* move past the 'unix:...|' UDS path info */
> const char *ret, *c;
>
>
> Modified: httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c?rev=1892874&r1=1892873&r2=1892874&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c (original)
> +++ httpd/httpd/branches/2.4.x/modules/proxy/proxy_util.c Fri Sep 3 16:52:38 2021
> @@ -2281,8 +2281,8 @@ static void fix_uds_filename(request_rec
> if (!r || !r->filename) return;
>
> if (!strncmp(r->filename, "proxy:", 6) &&
> - (ptr2 = ap_strcasestr(r->filename, "unix:")) &&
> - (ptr = ap_strchr(ptr2, '|'))) {
> + !ap_cstr_casecmpn(r->filename + 6, "unix:", 5) &&
> + (ptr2 = r->filename + 6 + 5, ptr = ap_strchr(ptr2, '|'))) {
I know that I voted for it to be backported, but after rethinking this shouldn't this be
(ptr2 = r->filename + 6, ptr = ap_strchr(ptr2 + 5, '|')))
as ptr2 is later used in
rv = apr_uri_parse(r->pool, ptr2, &urisock);
With the old code and with my proposal above ptr2 points to the 'u' of 'unix:' with the current code it points to the char after
'unix:' likely //.
Regards
Rüdiger