[03/10] linux: Add __readdir_unlocked
Commit Message
And use it on readdir_r implementation.
Checked on i686-linux-gnu.
---
include/dirent.h | 1 +
sysdeps/unix/sysv/linux/readdir.c | 18 +++++--
sysdeps/unix/sysv/linux/readdir_r.c | 79 +++++++----------------------
3 files changed, 31 insertions(+), 67 deletions(-)
Comments
* 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.
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.
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).
But these interfaces are also not marked as async-signal-safe in any case
by POSIX and if the idea is indeed move in this direction we should first
move opendir buffer allocation to use mmap instead. I am not sure bounding
glibc implementation to this requirement is really a gain, no other libc
implementation I am aware of also provide async-safe-signal.
This change also has the effect of consolidate readdir implementation
and remove code logic duplication.
* 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.
> 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.
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.
On 21/04/2020 10:00, Adhemerval Zanella wrote:
>
>
> 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.
Ping, I would like to move forward with this patchset. Is this approach
to add a internal list allocated by telldir a possible solution for
BZ#23960?
@@ -20,6 +20,7 @@ extern DIR *__opendirat (int dfd, const char *__name) attribute_hidden;
extern DIR *__fdopendir (int __fd) attribute_hidden;
extern int __closedir (DIR *__dirp) attribute_hidden;
extern struct dirent *__readdir (DIR *__dirp) attribute_hidden;
+extern struct dirent *__readdir_unlocked (DIR *__dirp) attribute_hidden;
extern struct dirent64 *__readdir64 (DIR *__dirp);
libc_hidden_proto (__readdir64)
extern int __readdir_r (DIR *__dirp, struct dirent *__entry,
@@ -23,15 +23,11 @@
/* Read a directory entry from DIRP. */
struct dirent *
-__readdir (DIR *dirp)
+__readdir_unlocked (DIR *dirp)
{
struct dirent *dp;
int saved_errno = errno;
-#if IS_IN (libc)
- __libc_lock_lock (dirp->lock);
-#endif
-
do
{
size_t reclen;
@@ -75,6 +71,18 @@ __readdir (DIR *dirp)
/* Skip deleted files. */
} while (dp->d_ino == 0);
+ return dp;
+}
+
+struct dirent *
+__readdir (DIR *dirp)
+{
+ struct dirent *dp;
+
+#if IS_IN (libc)
+ __libc_lock_lock (dirp->lock);
+#endif
+ dp = __readdir_unlocked (dirp);
#if IS_IN (libc)
__libc_lock_unlock (dirp->lock);
#endif
@@ -25,89 +25,44 @@ __readdir_r (DIR *dirp, struct dirent *entry, struct dirent **result)
{
struct dirent *dp;
size_t reclen;
- const int saved_errno = errno;
- int ret;
__libc_lock_lock (dirp->lock);
- do
+ while (1)
{
- if (dirp->offset >= dirp->size)
- {
- /* We've emptied out our buffer. Refill it. */
-
- size_t maxread = dirp->allocation;
- ssize_t bytes;
-
- maxread = dirp->allocation;
-
- bytes = __getdents (dirp->fd, dirp->data, maxread);
- if (bytes <= 0)
- {
- /* On some systems getdents fails with ENOENT when the
- open directory has been rmdir'd already. POSIX.1
- requires that we treat this condition like normal EOF. */
- if (bytes < 0 && errno == ENOENT)
- {
- bytes = 0;
- __set_errno (saved_errno);
- }
- if (bytes < 0)
- dirp->errcode = errno;
-
- dp = NULL;
- break;
- }
- dirp->size = (size_t) bytes;
-
- /* Reset the offset into the buffer. */
- dirp->offset = 0;
- }
-
- dp = (struct dirent *) &dirp->data[dirp->offset];
+ dp = __readdir_unlocked (dirp);
+ if (dp == NULL)
+ break;
reclen = dp->d_reclen;
+ if (reclen <= offsetof (struct dirent, d_name) + NAME_MAX + 1)
+ break;
- dirp->offset += reclen;
-
- dirp->filepos = dp->d_off;
-
- if (reclen > offsetof (struct dirent, d_name) + NAME_MAX + 1)
+ /* The record is very long. It could still fit into the caller-supplied
+ buffer if we can skip padding at the end. */
+ size_t namelen = _D_EXACT_NAMLEN (dp);
+ if (namelen <= NAME_MAX)
{
- /* The record is very long. It could still fit into the
- caller-supplied buffer if we can skip padding at the
- end. */
- size_t namelen = _D_EXACT_NAMLEN (dp);
- if (namelen <= NAME_MAX)
- reclen = offsetof (struct dirent, d_name) + namelen + 1;
- else
- {
- /* The name is too long. Ignore this file. */
- dirp->errcode = ENAMETOOLONG;
- dp->d_ino = 0;
- continue;
- }
+ reclen = offsetof (struct dirent, d_name) + namelen + 1;
+ break;
}
- /* Skip deleted and ignored files. */
+ /* The name is too long. Ignore this file. */
+ dirp->errcode = ENAMETOOLONG;
+ dp->d_ino = 0;
}
- while (dp->d_ino == 0);
if (dp != NULL)
{
*result = memcpy (entry, dp, reclen);
entry->d_reclen = reclen;
- ret = 0;
}
else
- {
- *result = NULL;
- ret = dirp->errcode;
- }
+ *result = NULL;
__libc_lock_unlock (dirp->lock);
- return ret;
+ return dp != NULL ? 0 : dirp->errcode;
}
weak_alias (__readdir_r, readdir_r)