[v4,1/5] linux: Use getdents64 on non-LFS readdir
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
The non-LFS opendir allocates a translation buffer to be used to
return the non-LFS readdir entry. The obtained dirent64 struct
is translated to the temporary buffer on each readdir call.
Entries that overflow d_off/d_ino and the buffer reallocation failure
(in case of large d_name) are ignored.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
dirent/tst-scandir.c | 6 +-
sysdeps/unix/sysv/linux/closedir.c | 4 ++
sysdeps/unix/sysv/linux/dirstream.h | 5 ++
sysdeps/unix/sysv/linux/opendir.c | 21 +++++++
sysdeps/unix/sysv/linux/readdir.c | 90 ++++++++++++++++++++---------
5 files changed, 99 insertions(+), 27 deletions(-)
Comments
On 1/26/23 11:29, Adhemerval Zanella via Libc-alpha wrote:
> +#if !defined __OFF_T_MATCHES_OFF64_T || !defined __INO_T_MATCHES_INO64_T
> + char *tbuffer; /* Translation buffer for non-LFS calls. */
> + size_t tbuffer_size; /* Size of translation buffer. */
> +#endif
How about something like this instead?
#if !defined __OFF_T_MATCHES_OFF64_T || !defined __INO_T_MATCHES_INO64_T
struct dirent tdirent;
char tbuffer[USHRT_MAX - offsetof (struct dirent64, d_name)];
#endif
In other words, simply allocate the maximum-sized buffer you might need.
This should simplify memory allocation code. Sure, it uses more memory
but that's not a big deal nowadays even on 32-bit platforms.
Similarly for dirstream_old_entry in patch 5/5.
On 26/01/23 20:06, Paul Eggert wrote:
> On 1/26/23 11:29, Adhemerval Zanella via Libc-alpha wrote:
>> +#if !defined __OFF_T_MATCHES_OFF64_T || !defined __INO_T_MATCHES_INO64_T
>> + char *tbuffer; /* Translation buffer for non-LFS calls. */
>> + size_t tbuffer_size; /* Size of translation buffer. */
>> +#endif
>
> How about something like this instead?
>
> #if !defined __OFF_T_MATCHES_OFF64_T || !defined __INO_T_MATCHES_INO64_T
> struct dirent tdirent;
> char tbuffer[USHRT_MAX - offsetof (struct dirent64, d_name)];
> #endif
>
> In other words, simply allocate the maximum-sized buffer you might need. This should simplify memory allocation code. Sure, it uses more memory but that's not a big deal nowadays even on 32-bit platforms.
>
> Similarly for dirstream_old_entry in patch 5/5.
I fact I don't think we need a translation buffer at all, since
readdir_r (which also calls __readdir_unlocked) will not return
entries with d_name larger than NAME_MAX. I think an extra
struct direct, along with the expected overflow and maximum name
checks should be suffice.
@@ -155,8 +155,12 @@ do_test (void)
}
if (n != 6)
{
+ /* Non-lfs opendir skips entries that can not be represented (for
+ instance if d_off is not an offset but rather an internal filesystem
+ representation. For this case there is no point in continue the
+ testcase. */
printf ("scandir returned %d entries instead of 6\n", n);
- return 1;
+ return EXIT_UNSUPPORTED;
}
struct
@@ -47,6 +47,10 @@ __closedir (DIR *dirp)
__libc_lock_fini (dirp->lock);
#endif
+#if !_DIRENT_MATCHES_DIRENT64
+ free (dirp->tbuffer);
+#endif
+
free ((void *) dirp);
return __close_nocancel (fd);
@@ -41,6 +41,11 @@ struct __dirstream
int errcode; /* Delayed error code. */
+#if !defined __OFF_T_MATCHES_OFF64_T || !defined __INO_T_MATCHES_INO64_T
+ char *tbuffer; /* Translation buffer for non-LFS calls. */
+ size_t tbuffer_size; /* Size of translation buffer. */
+#endif
+
/* Directory block. We must make sure that this block starts
at an address that is aligned adequately enough to store
dirent entries. Using the alignment of "void *" is not
@@ -120,6 +120,27 @@ __alloc_dir (int fd, bool close_fd, int flags,
return NULL;
}
+#if !_DIRENT_MATCHES_DIRENT64
+ /* Allocate a translation buffer to use as the returned 'struct direct'
+ for non-LFS 'readdir' calls.
+
+ The initial NAME_MAX size should handle most cases, readdir might expand
+ it if required (i.e for names larger than NAME_MAX). */
+ enum
+ {
+ tbuffer_size = sizeof (struct dirent) + NAME_MAX + 1
+ };
+ dirp->tbuffer = malloc (tbuffer_size);
+ if (dirp->tbuffer == NULL)
+ {
+ free (dirp);
+ if (close_fd)
+ __close_nocancel_nostatus (fd);
+ return NULL;
+ }
+ dirp->tbuffer_size = tbuffer_size;
+#endif
+
dirp->fd = fd;
#if IS_IN (libc)
__libc_lock_init (dirp->lock);
@@ -21,42 +21,80 @@
#if !_DIRENT_MATCHES_DIRENT64
#include <dirstream.h>
+/* Translate the DP64 entry to the non-LFS one in the translation buffer
+ at dirstream DS. Return true is the translation was possible or
+ false if either an internal field can not be represented in the non-LFS
+ entry or if the translation can not be resized. */
+static bool
+dirstream_entry (struct __dirstream *ds, const struct dirent64 *dp64)
+{
+ if (!in_off_t_range (dp64->d_off) || !in_ino_t_range (dp64->d_ino))
+ return false;
+
+ /* Expand the translation buffer to hold the new name size. */
+ size_t new_reclen = sizeof (struct dirent)
+ + dp64->d_reclen - offsetof (struct dirent64, d_name);
+ if (new_reclen > ds->tbuffer_size)
+ {
+ char *newbuffer = realloc (ds->tbuffer, new_reclen);
+ if (newbuffer == NULL)
+ return false;
+ ds->tbuffer = newbuffer;
+ ds->tbuffer_size = new_reclen;
+ }
+
+ struct dirent *dp = (struct dirent *) ds->tbuffer;
+
+ dp->d_off = dp64->d_off;
+ dp->d_ino = dp64->d_ino;
+ dp->d_reclen = new_reclen;
+ dp->d_type = dp64->d_type;
+ memcpy (dp->d_name, dp64->d_name,
+ dp64->d_reclen - offsetof (struct dirent64, d_name));
+
+ return true;
+}
+
/* Read a directory entry from DIRP. */
struct dirent *
__readdir_unlocked (DIR *dirp)
{
- struct dirent *dp;
int saved_errno = errno;
- if (dirp->offset >= dirp->size)
+ while (1)
{
- /* We've emptied out our buffer. Refill it. */
-
- size_t maxread = dirp->allocation;
- ssize_t bytes;
-
- bytes = __getdents (dirp->fd, dirp->data, maxread);
- if (bytes <= 0)
+ if (dirp->offset >= dirp->size)
+ {
+ /* We've emptied out our buffer. Refill it. */
+ ssize_t bytes = __getdents64 (dirp->fd, dirp->data,
+ dirp->allocation);
+ if (bytes <= 0)
+ {
+ /* Linux may fail with ENOENT on some file systems if the
+ directory inode is marked as dead (deleted). POSIX
+ treats this as a regular end-of-directory condition, so
+ do not set errno in that case, to indicate success. */
+ if (bytes < 0 && errno == ENOENT)
+ __set_errno (saved_errno);
+ return NULL;
+ }
+ dirp->size = bytes;
+
+ /* Reset the offset into the buffer. */
+ dirp->offset = 0;
+ }
+
+ struct dirent64 *dp64 = (struct dirent64 *) &dirp->data[dirp->offset];
+ dirp->offset += dp64->d_reclen;
+
+ /* Skip entries which might overflow d_off/d_ino or if the translation
+ buffer can not be resized. */
+ if (dirstream_entry (dirp, dp64))
{
- /* Linux may fail with ENOENT on some file systems if the
- directory inode is marked as dead (deleted). POSIX
- treats this as a regular end-of-directory condition, so
- do not set errno in that case, to indicate success. */
- if (bytes == 0 || errno == ENOENT)
- __set_errno (saved_errno);
- return NULL;
+ dirp->filepos = dp64->d_off;
+ return (struct dirent *) dirp->tbuffer;
}
- dirp->size = (size_t) bytes;
-
- /* Reset the offset into the buffer. */
- dirp->offset = 0;
}
-
- dp = (struct dirent *) &dirp->data[dirp->offset];
- dirp->offset += dp->d_reclen;
- dirp->filepos = dp->d_off;
-
- return dp;
}
struct dirent *