Message ID | 20200417132209.22065-3-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [01/10] linux: Move posix dir implementations to Linux | expand |
* 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?
diff --git a/include/dirent.h b/include/dirent.h index fdf4c4a2f1..8325a19e5f 100644 --- a/include/dirent.h +++ b/include/dirent.h @@ -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, diff --git a/sysdeps/unix/sysv/linux/readdir.c b/sysdeps/unix/sysv/linux/readdir.c index 2e03e66e69..ca2a8964e9 100644 --- a/sysdeps/unix/sysv/linux/readdir.c +++ b/sysdeps/unix/sysv/linux/readdir.c @@ -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 diff --git a/sysdeps/unix/sysv/linux/readdir_r.c b/sysdeps/unix/sysv/linux/readdir_r.c index 0069041394..a01d2845a6 100644 --- a/sysdeps/unix/sysv/linux/readdir_r.c +++ b/sysdeps/unix/sysv/linux/readdir_r.c @@ -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)