Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

From: Jeff King <peff@peff.net>
Date: Mar 26, 2021, 05:28 UTC
Message-ID: <YF1w/xKbozpQn7Vf@coredump.intra.peff.net>
In-Reply-To: <20210326024005.26962-2-stdedos+git@gmail.com>

On Fri, Mar 26, 2021 at 04:40:04AM +0200, Σταύρος Ντέντος wrote:

> +void check_mishandled_exclude(const char *entry) {
> +	size_t entry_len = strlen(entry);
> +	char flags[entry_len];
> +	char path[entry_len];

We avoid using variable-length arrays in our codebase. For one thing, they were not historically supported by all platforms (we are slowly using more C99 features, but we are introducing them slowly and intentionally).

But two, they are limited in size and the failure mode is not graceful. If "entry" is larger than the available stack, then we'll get a segfault with no option to handle it better.

> +	if (sscanf(entry, ":!(%4096[^)])%4096s", &flags, &path) != 2) {
> +		return;

We also generally avoid using scanf, because it's error-prone. The "4096" is scary here, but I don't _think_ it's a buffer overflow, because "path" is already the same size as "entry" (not including the NUL terminator, but that is negated by the fact that we'll have skipped at least ":!").

Is this "%4096[^)]" actually valid? I don't think scanf understands regular expressions.

We'd want to avoid making an extra copy of the string anyway. So you'd probably want to just parse left-to-right in the original string, like:

  const char *p = entry;

  /* skip past stuff we know must be there */
  if (!skip_prefix(p, ":!(", &p))
        return;

  /* this checks count_slashes() > 0 in the flags section, though I'm
   * not sure I understand what that is looking for... */
  for (; *p && *p != ')'; p++) {
        if (*p == '/')
	        return;
  }
  if (*p++ != ')')
        return;

  /* now p is pointing at "path", though we don't seem to do anything
   * with it... */

-Peff