Re: [PATCH 5/5] stdlib: Remove lstat usage from realpath [BZ #24970]

[Paul Eggert]

This email finishes the review of this proposed glibc patchset. (I 
didn't look at patch 4/5 for test cases.)

On 12/24/20 7:17 AM, Adhemerval Zanella wrote:

> +/* Check if BUFFER is using the internal buffer.  */
> +static __always_inline bool
> +scratch_buffer_using_internal (struct scratch_buffer *buffer)
> +{
> +  return buffer->data == buffer->__space.__c;
> +}
> +
> +/* Return the internal buffer from BUFFER if it is dynamic allocated,
> +   otherwise returns NULL.  Initializes the BUFFER if the internal
> +   dynamic buffer is returned.  */
> +static __always_inline void *
> +scratch_buffer_take_buffer (struct scratch_buffer *buffer)
> +{
> +  if (scratch_buffer_using_internal (buffer))
> +    return NULL;
> +
> +  void *r = buffer->data;
> +  scratch_buffer_init (buffer);
> +  return r;
> +}

This combination of functions is a little tricky. Instead, how about a 
single function that duplicates the scratch buffer on the heap, and 
frees the scratch buffer? Please see the attached proposed patch for 
Gnulib, which implements this idea. (I have not installed this into Gnulib.)

Also, shouldn't we merge the already-existing Gnulib scratch_buffer 
changes into glibc, along with this new change?

>   static idx_t
> +readlink_scratch_buffer (const char *path, struct scratch_buffer *buf)
> +{
> +  ssize_t r;
> +  while (true)
> +    {
> +      ptrdiff_t bufsize = buf->length;
> +      r = __readlink (path, buf->data, bufsize - 1);
> +      if (r < bufsize - 1)
> +       break;
> +      if (!scratch_buffer_grow (buf))
> +       return -1;
> +    }
> +  return r;
> +}

This function seems to exist because the proposed code calls readlink in 
two places. Current gnulib has been changed to call it in just one 
place, so there's less need to split out the function (the splitting 
complicates out-of-memory checking).

> -  scratch_buffer_init (rname_buf);
> -  char *rname_on_stack = rname_buf->data;
> ...
> +  scratch_buffer_init (&rname_buffer);
> +  char *rname = rname_buffer.data;

Doesn't this sort of thing have the potential to run into GCC bug 93644? 
That bug tends to be flaky; change platforms, or a few lines of code, 
and the problem recurs. Although it's just a bogus warning it cannot be 
turned off Gnulib has a GCC_LINT fix for this, which glibc could use 
simply with "#define GCC_LINT 1" in the "#ifdef _GLIBC" code.

> @@ -206,6 +219,20 @@ __realpath (const char *name, char *resolved)
>           /* nothing */;
>         else if (startlen == 2 && start[0] == '.' && start[1] == '.')
>           {
> +          if (!ISSLASH (dest[-1]))
> +            *dest++ = '/';
> +          *dest = '\0';
> +
> +         ssize_t n = readlink_scratch_buffer (rname, &link_buffer);
> +          if (n < 0)
> +            {
> +              if (errno == ENOTDIR && dest[-1] == '/')
> +                dest[-1] = '\0';
> +             if (errno == ENOMEM)
> +               goto error_nomem;
> +              if (errno != EINVAL)
> +                goto error;
> +            }

This can call readlink twice, once with trailing slash and once without. 
Better to call it just once.

> +             char *buf = (char*) link_buffer.data;
>   
>                 buf[n] = '\0';
>   
> @@ -279,7 +296,7 @@ __realpath (const char *name, char *resolved)
>   
>                 /* Careful here, end may be a pointer into extra_buf... */
>                 memmove (&extra_buf[n], end, len + 1);
> -              name = end = memcpy (extra_buf, buf, n);
> +              name = end = memcpy (extra_buf, link_buffer.data, n);

If buf already equals link_buffer.data, no need for the patch to change 
buf to link_buffer.data.

> -          else if (! (startlen == 0
> -                      ? stat (rname, &st) == 0 || errno == EOVERFLOW
> -                      : errno == EINVAL))
> -            goto error;

I think current Gnulib addresses this issue in a different way, that 
doesn't involve the extra readlink calls.
>     if (DOUBLE_SLASH_IS_DISTINCT_ROOT && dest == rname + 1 && !prefix_len
>         && ISSLASH (*dest) && !ISSLASH (dest[1]))
>       dest++;
> +  *dest = '\0';
>     failed = false;
>   
>   error:
> -  *dest++ = '\0';

This looks dubious, as the error case also needs *dest to be '\0' and to 
increment dest (for when returning NULL when resolved != NULL).

Proposed patch to Gnulib attached. I hope this patch (along with what's 
already in Gnulib) addresses all the issues raised in your glibc 
patches, in the sense that the relevant files can be identical in Glibc 
and in Gnulib. I haven't installed this into Gnulib master on savannah.

0001-canonicalize-simplify-via-scratch_buffer_dupfree.patch
Description: Text Data