On 24/12/2020 21:27, Paul Eggert wrote:
> 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.)
Indeed this seems a better alternative.
>
> Also, shouldn't we merge the already-existing Gnulib scratch_buffer changes
> into glibc, along with this new change?
Which changes are you referring? Checking against bbaba6ce5 I see all
glibc files for scratch_buffer are similar to the gnulib ones.
>
>> 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).
Yes, this trades a stat call by an extra readlink call. However it seems
that your strategy to use faccessat should be better, assuming it is lighter
syscall.
>
>> - 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.
I wasn't aware on the GCC issue in fact (it seems to affect GCC 10/11
and I am using GCC 9.2.1).
>
>> @@ -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.
Right.
>
>> + 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.
>
Indeed, this is a left over from development.
>> - 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]))
Yes, I noted now on gnulib master that you handled it with an faccessat
on dir_check.
>> 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).
>
Yes, I think this is also a leftover from development.
> 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.
Changes seems ok, I will send a v3 with the proposed changes to sync
the gnulib headers, along with the scratch_buffer change, the gnulib
sync that fix all the issues, and the extra test.
Thanks for working on this.