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

[Adhemerval Zanella]

On 28/12/2020 08:42, Adhemerval Zanella wrote:
>>>   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.

Also, it would require a recent kernel (5.8) to avoid faccessat to fallback
to use __NR_faccessat (it would make each call to faccessat2 to issue
two syscall).  We can live with this and if it turns a performance issue we
ca the hack to set a global variable to avoid it.

Now this snippet:

 47 # include <sysdep.h>
 48 # ifdef __ASSUME_FACCESSAT2
 49 #  define FACCESSAT_NEVER_EOVERFLOWS __ASSUME_FACCESSAT2
 50 # else
 51 #  define FACCESSAT_NEVER_EOVERFLOWS true
 52 # endif

is not required, since Linux faccessat fallback uses LFS stat call that
does not return EOVERFLOW.  And it also bleeds Linux implementation details
on generic code.

I will send a patch to fix it.