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

On 08/08/26 02:52, Collin Funk wrote:
> 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.

We can, I will change to use.

> 
>> +  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?

Not reason in fact, and returning LEN makes more sense. I will change it.

> 
> 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].

I will work on the variable scope reduction to keep gnulib sync easier.

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

Thanks for checking on this.

> 
> 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]>

Ack.

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