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
