Re: [PATCH 03/10] linux: Add __readdir_unlocked

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 21/04/2020 09:16, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 21/04/2020 07:41, Florian Weimer wrote:
>>> * Adhemerval Zanella via Libc-alpha:
>>>
>>>> And use it on readdir_r implementation.
>>>
>>> I think __readdir_unlocked will not really be async-signal-safe if we
>>> have to call malloc during readdir, to allocate the long-to-off64_t
>>> translation table for the real fix for bug 23960 (when the kernel does
>>> not provide 32-bit off64_t values).
>>>
>>> That's why I think this change does not go in the right direction, sorry.
>>>
>>
>> I am not following, the whole set explicit avoid malloc by using a reserved
>> space in the allocated but on opendir.  Even on mips64, which requires
>> a special getdents64 implementation, avoid memory allocation.
> 
> The fix for bug 23960 needs to introduce some kind of memory
> allocation to store the mapping.  We can avoid it in most cases, but I
> think the readdir interface has too much baggage to make it
> async-signal-safe.

My proposed solution is to first set readdir use internally off64_t (even
for non-LFS) and whence he memory allocation is done on telldir which
will try to first allocate an entry on the dynamic array embedded on
allocated opendir __dirstream.

> 
>> And it is for both non-LFS and LFS interfaces.  The only issue for 
>> async-signal-safeness is on telldir for non-LFS mode which might require
>> to extend the dynamic array internal buffer if the entry could not be
>> added in the internal pre-allocated buffer (and this could be mitigated
>> if we added a mmap variant of dynamic array). 
> 
> telldir cannot allocate because there is no way to return error.  The
> allocation has to happen in readdir.
> 
> We should be able to optimize out allocations if telldir is never
> called (basically, pre-allocate one slot to be used for telldir).  But
> all this is really tricky.  I do wonder if it makes more sense to call
> getdents64 directly if one needs async-signal-safety.

This is essentially what the patchset does: it uses gendents64 on
both LFS and non-LFS interface, the position is already place on
the __dirstream and a default dynamic array of off64_t is used to
map internal directory offset to long int.

The problem is not really telldir, but rather seekdir that does not
allow to signal an invalid position. The standard states that any
value obtained from telldir is valid. 

My proposed fix uses a dynamic array with the default pre-defined size 
to  allocated the off64_t mapping and returns -1 if the dynamic array 
could not be extended.  Although it is not what POSIX state, man-pages
documents as a possible return error. 

But even not allowing the pre-allocated buffer to extend over its
initial size, we will still need to handle a telldir call with the
buffer full.  So I see we might have two options:

  1. Just abort on telldir once it requires an additional entry in the
     pre-allocated mapping buffer.

  2. Return -1 on failure and handle it as special sentinel value that
     does not update the directory position.  So user might still try
     to check if seekdir does succeeded by:

       long int pre = telldir (dirp);
       seek (dirp, value);
       if (pre == telldir (dirp)
         // position not updated, invalid value

In any case, this fix is only for *no-LFS* which should be considered
a legacy interface a long time ago.  The default LFS interface does not
suffer with this limitation.