On Mon, Sep 6, 2021 at 12:16 PM Ruediger Pluem <rpl...@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.