Re: [PATCH v2 1/1] worktree add: sanitize worktree names

[Jeff King <peff@peff.net>]

On Thu, Feb 21, 2019 at 07:19:43PM +0700, Nguyễn Thái Ngọc Duy wrote:

> +/*
> + * worktree name is part of refname and has to pass
> + * check_refname_component(). Remove unallowed characters to make it
> + * valid.
> + */
> +static void sanitize_worktree_name(struct strbuf *name)
> +{
> +	char *orig_name = xstrdup(name->buf);
> +	int i;
> +
> +	/*
> +	 * All special chars replaced with dashes. See
> +	 * check_refname_component() for reference.
> +	 * Note that .lock is also turned to -lock, removing its
> +	 * special status.
> +	 */
> +	for (i = 0; i < name->len; i++) {
> +		if (strchr(":?[]\\~ \t@{}*/.", name->buf[i]))
> +			name->buf[i] = '-';
> +	}

This is reject-known-bad, but I think there are still some other
characters that are not allowed in refnames (e.g., ASCII control
characters). Which would lead to us hitting the BUG() below.

It might make sense to provide access to refname_disposition() and use
it here. Alternatively, I think if we did an allow-known-good, it might
be OK to have a slightly more restrictive scheme (say, alnum plus
dashes, plus high-bit chars).

> +	/* remove consecutive dashes, leading or trailing dashes */
> +	for (i = 0; i < name->len; i++) {
> +		while (name->buf[i] == '-' &&
> +		       (i == 0 ||
> +			i == name->len - 1 ||
> +			(i < name->len - 1 && name->buf[i + 1] == '-')))
> +			strbuf_remove(name, i, 1);
> +	}

I think this is correct, though it is possibly to be quadratic in the
string length due to the O(n) remove. I think this kind of sanitizing is
more readable if done between two strings rather than in-place, like:

  for (i = 0; i < name->len; i++) {
	if (is_allowed(name->buf[i])) {
		strbuf_addch(&dest, name->buf[i]);
		last_was_dash = 0;
	} else if (!last_was_dash && dest->len)
		strbuf_addch(&dest, '-');
		last_was_dash = 1;
	}
  }
  /* still must handle removal from end of stray "-" and ".lock" */
  strbuf_swap(name, &dest);
  strbuf_release(&dest);

but that may just be personal preference. I'm OK with it if you prefer
the in-place way.

-Peff