Re: [PATCH 6/6] posix: Do not recurse once per pattern component in glob [BZ #34453]

Adhemerval Zanella <[email protected]> writes:

> +/* Remove one unquoted trailing backslash from P, which is *LENP bytes long,
> +   and update *LENP.  */
> +static void
> +glob_strip_backslash (char *p, size_t *lenp)
> +{
> +  size_t len = *lenp;
> +  char *q = &p[len - 1];
> +
> +  while (q > p && q[-1] == '\\')
> +    --q;
> +  /* Check if the number of '\' is odd.  */
> +  if ((&p[len] - q) & 1)
> +    p[--len] = '\0';
> +  *lenp = len;
> +}
> +
> +/* Remove the quoting backslashes from P, which is *LENP bytes long and
> +   contains at least one backslash, and update *LENP.  */
> +static void
> +glob_unescape (char *p, size_t *lenp)
> +{
> +  size_t len = *lenp;
> +  char *s = strchr (p, '\\');
> +  char *d = s;

Since we have LEN, can't we use memchr? Not that P will be long enough
to matter much, of course.

> +  do
> +    {
> +      if (*s == '\\')
> +        {
> +          *d = *++s;
> +          --len;
> +        }
> +      else
> +        *d = *s;
> +      ++d;
> +    }
> +  while (*s++ != '\0');
> +  *lenp = len;
> +}

I think it would be better for glob_strip_backslash and glob_unescape
should return LEN instead of modifying a pointer. Any reason why they
shouldn't?

These functions are small, so it isn't a major issue, but I find it
easier to forget to set a pointer in one path than to forget a return.

> +/* Expand DIRPATTERN, the directory part of a pattern, into PGLOB.  FLAGS
> +   holds GLOB_ERR, GLOB_NOESCAPE and GLOB_ALTDIRFUNC as inherited from the
> +   caller, plus GLOB_NOSORT and GLOB_ONLYDIR.
> +
> +   DIRPATTERN is split at its slashes into components, each recorded in a
> +   heap-allocated array along with what it has to do with the directories
> +   it matches.  The components are then matched from left to right,
> +   starting at the longest leading part that has no metacharacter.  Stack
> +   usage does not depend on how many components there are, and a component
> +   that matches nothing ends the expansion at once.  */
> +static int
> +glob_dir_pattern (const char *dirpattern, int flags,
> +                  int (*errfunc) (const char *, int), glob_t *pglob)
> +{
> +  bool quote = !(flags & GLOB_NOESCAPE);
> +  struct glob_dir_step *steps = NULL;
> +  size_t nsteps = 0;
> +  size_t maxsteps;
> +  char *work;
> +  char *p;
> +  const char *base;
> +  const char *base_pattern;
> +  size_t baselen;
> +  /* Whether the level being scanned appends slashes to its result.  A
> +     component followed by a slash passes this down instead of matching
> +     anything itself, exactly as GLOB_MARK is passed down by glob.  */
> +  bool mark = false;
> +  glob_t cur;
> +  int retval;
> +
> +  /* The pattern is split in place, so work on a copy.  */
> +  work = strdup (dirpattern);
> +  if (work == NULL)
> +    return GLOB_NOSPACE;
> +  p = work;
> +
> +  /* Every step splits the pattern at a slash, so there is never more than one
> +     step per slash.  */
> +  maxsteps = glob_count (dirpattern, '/');

I will probably need to reduce the variable scope here and some other
places before importing it to gnulib. But I think it is okay for now.
There is already a large diff in what gnulib and glibc have since
Bruno's reduction of variable scope last year [1].

Generally, the patch looks good though. I tested them in gnulib and it
passes those tests on GNU/Linux.

Anyway, I don't think the minor points I brought up require a V2. Feel
free to add my Reviewed-by after checking them, whether you decide to
make the changes or not.

Reviewed-by: Collin Funk <[email protected]>

Collin

[1] https://github.com/coreutils/gnulib/commit/6edf6dbacf1e136539da03115adee858ec8a3d33