From 306bc621460e5ef39ae86babd31befdf40eaf9c1 Mon Sep 17 00:00:00 2001 From: jhb Date: Thu, 14 Aug 2014 20:20:21 +0000 Subject: [PATCH] MFC 268531,269079,269204: Fix various edge cases with rewinddir(), seekdir(), and telldir(): - In the unionfs case, opendir() and fdopendir() read the directory's full contents and cache it. This cache is not refreshed when rewinddir() is called, so rewinddir() will not notice updates to a directory. Fix this by splitting the code to fetch a directory's contents out of __opendir_common() into a new _filldir() function and call this from rewinddir() when operating on a unionfs directory. - If rewinddir() is called on a directory opened with fdopendir() before any directory entries are fetched, rewinddir() will not adjust the seek location of the backing file descriptor. If the file descriptor passed to fdopendir() had a non-zero offset, the rewinddir() will not rewind to the beginning. Fix this by always seeking back to 0 in rewinddir(). This means the dd_rewind hack can also be removed. - Add missing locking to rewinddir() - POSIX says that passing a location returned by telldir() to seekdir() after an intervening call to rewinddir() is undefined, so reclaim any pending telldir() cookies in the directory when rewinddir() is called. - If telldir() is called immediately after a call to seekdir(), POSIX requires the return value of telldir() to equal the value passed to seekdir(). The current seekdir code with SINGLEUSE enabled breaks this case as each call to telldir() allocates a new cookie. Instead, remove the SINGLEUSE code and change telldir() to look for an existing cookie for the directory's current location rather than always creating a new cookie. PR: 121656 git-svn-id: svn://svn.freebsd.org/base/stable/10@270002 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f --- include/dirent.h | 1 + lib/libc/gen/directory.3 | 13 +- lib/libc/gen/gen-private.h | 1 - lib/libc/gen/opendir.c | 365 +++++++++++++++++++++---------------- lib/libc/gen/readdir.c | 4 +- lib/libc/gen/rewinddir.c | 19 +- lib/libc/gen/telldir.c | 41 +++-- lib/libc/gen/telldir.h | 2 + 8 files changed, 253 insertions(+), 193 deletions(-) diff --git a/include/dirent.h b/include/dirent.h index d0b0a9a43..a04e88bb8 100644 --- a/include/dirent.h +++ b/include/dirent.h @@ -63,6 +63,7 @@ typedef struct _dirdesc DIR; #define DTF_NODUP 0x0002 /* don't return duplicate names */ #define DTF_REWIND 0x0004 /* rewind after reading union stack */ #define __DTF_READALL 0x0008 /* everything has been read */ +#define __DTF_SKIPREAD 0x0010 /* assume internal buffer is populated */ #else /* !__BSD_VISIBLE */ diff --git a/lib/libc/gen/directory.3 b/lib/libc/gen/directory.3 index 4573d4513..1e864f8dc 100644 --- a/lib/libc/gen/directory.3 +++ b/lib/libc/gen/directory.3 @@ -28,7 +28,7 @@ .\" @(#)directory.3 8.1 (Berkeley) 6/4/93 .\" $FreeBSD$ .\" -.Dd August 18, 2013 +.Dd July 28, 2014 .Dt DIRECTORY 3 .Os .Sh NAME @@ -169,6 +169,10 @@ If the directory is closed and then reopened, prior values returned by .Fn telldir will no longer be valid. +Values returned by +.Fn telldir +are also invalidated by a call to +.Fn rewinddir . .Pp The .Fn seekdir @@ -182,13 +186,6 @@ The new position reverts to the one associated with the when the .Fn telldir operation was performed. -State associated with the token returned by -.Fn telldir is freed when it is passed to -.Fn seekdir . -If you wish return to the same location again, -then you must create a new token with another -.Fn telldir -call. .Pp The .Fn rewinddir diff --git a/lib/libc/gen/gen-private.h b/lib/libc/gen/gen-private.h index e8854ad2d..d1fab5f31 100644 --- a/lib/libc/gen/gen-private.h +++ b/lib/libc/gen/gen-private.h @@ -48,7 +48,6 @@ struct _dirdesc { char *dd_buf; /* data buffer */ int dd_len; /* size of data buffer */ long dd_seek; /* magic cookie returned by getdirentries */ - long dd_rewind; /* magic cookie for rewinding */ int dd_flags; /* flags for readdir */ struct pthread_mutex *dd_lock; /* lock */ struct _telldir *dd_td; /* telldir position recording */ diff --git a/lib/libc/gen/opendir.c b/lib/libc/gen/opendir.c index a9eb0af1b..54928e73d 100644 --- a/lib/libc/gen/opendir.c +++ b/lib/libc/gen/opendir.c @@ -49,7 +49,7 @@ __FBSDID("$FreeBSD$"); #include "gen-private.h" #include "telldir.h" -static DIR * __opendir_common(int, int); +static DIR * __opendir_common(int, int, bool); /* * Open a directory. @@ -67,18 +67,10 @@ opendir(const char *name) DIR * fdopendir(int fd) { - struct stat statb; - /* Check that fd is associated with a directory. */ - if (_fstat(fd, &statb) != 0) - return (NULL); - if (!S_ISDIR(statb.st_mode)) { - errno = ENOTDIR; - return (NULL); - } if (_fcntl(fd, F_SETFD, FD_CLOEXEC) == -1) return (NULL); - return (__opendir_common(fd, DTF_HIDEW|DTF_NODUP)); + return (__opendir_common(fd, DTF_HIDEW|DTF_NODUP, true)); } DIR * @@ -88,11 +80,13 @@ __opendir2(const char *name, int flags) DIR *dir; int saved_errno; + if ((flags & (__DTF_READALL | __DTF_SKIPREAD)) != 0) + return (NULL); if ((fd = _open(name, O_RDONLY | O_NONBLOCK | O_DIRECTORY | O_CLOEXEC)) == -1) return (NULL); - dir = __opendir_common(fd, flags); + dir = __opendir_common(fd, flags, false); if (dir == NULL) { saved_errno = errno; _close(fd); @@ -109,23 +103,196 @@ opendir_compar(const void *p1, const void *p2) (*(const struct dirent **)p2)->d_name)); } +/* + * For a directory at the top of a unionfs stack, the entire directory's + * contents are read and cached locally until the next call to rewinddir(). + * For the fdopendir() case, the initial seek position must be preserved. + * For rewinddir(), the full directory should always be re-read from the + * beginning. + * + * If an error occurs, the existing buffer and state of 'dirp' is left + * unchanged. + */ +bool +_filldir(DIR *dirp, bool use_current_pos) +{ + struct dirent **dpv; + char *buf, *ddptr, *ddeptr; + off_t pos; + int fd2, incr, len, n, saved_errno, space; + + len = 0; + space = 0; + buf = NULL; + ddptr = NULL; + + /* + * Use the system page size if that is a multiple of DIRBLKSIZ. + * Hopefully this can be a big win someday by allowing page + * trades to user space to be done by _getdirentries(). + */ + incr = getpagesize(); + if ((incr % DIRBLKSIZ) != 0) + incr = DIRBLKSIZ; + + /* + * The strategy here is to read all the directory + * entries into a buffer, sort the buffer, and + * remove duplicate entries by setting the inode + * number to zero. + * + * We reopen the directory because _getdirentries() + * on a MNT_UNION mount modifies the open directory, + * making it refer to the lower directory after the + * upper directory's entries are exhausted. + * This would otherwise break software that uses + * the directory descriptor for fchdir or *at + * functions, such as fts.c. + */ + if ((fd2 = _openat(dirp->dd_fd, ".", O_RDONLY | O_CLOEXEC)) == -1) + return (false); + + if (use_current_pos) { + pos = lseek(dirp->dd_fd, 0, SEEK_CUR); + if (pos == -1 || lseek(fd2, pos, SEEK_SET) == -1) { + saved_errno = errno; + _close(fd2); + errno = saved_errno; + return (false); + } + } + + do { + /* + * Always make at least DIRBLKSIZ bytes + * available to _getdirentries + */ + if (space < DIRBLKSIZ) { + space += incr; + len += incr; + buf = reallocf(buf, len); + if (buf == NULL) { + saved_errno = errno; + _close(fd2); + errno = saved_errno; + return (false); + } + ddptr = buf + (len - space); + } + + n = _getdirentries(fd2, ddptr, space, &dirp->dd_seek); + if (n > 0) { + ddptr += n; + space -= n; + } + if (n < 0) { + saved_errno = errno; + _close(fd2); + errno = saved_errno; + return (false); + } + } while (n > 0); + _close(fd2); + + ddeptr = ddptr; + + /* + * There is now a buffer full of (possibly) duplicate + * names. + */ + dirp->dd_buf = buf; + + /* + * Go round this loop twice... + * + * Scan through the buffer, counting entries. + * On the second pass, save pointers to each one. + * Then sort the pointers and remove duplicate names. + */ + for (dpv = 0;;) { + n = 0; + ddptr = buf; + while (ddptr < ddeptr) { + struct dirent *dp; + + dp = (struct dirent *) ddptr; + if ((long)dp & 03L) + break; + if ((dp->d_reclen <= 0) || + (dp->d_reclen > (ddeptr + 1 - ddptr))) + break; + ddptr += dp->d_reclen; + if (dp->d_fileno) { + if (dpv) + dpv[n] = dp; + n++; + } + } + + if (dpv) { + struct dirent *xp; + + /* + * This sort must be stable. + */ + mergesort(dpv, n, sizeof(*dpv), opendir_compar); + + dpv[n] = NULL; + xp = NULL; + + /* + * Scan through the buffer in sort order, + * zapping the inode number of any + * duplicate names. + */ + for (n = 0; dpv[n]; n++) { + struct dirent *dp = dpv[n]; + + if ((xp == NULL) || + strcmp(dp->d_name, xp->d_name)) { + xp = dp; + } else { + dp->d_fileno = 0; + } + if (dp->d_type == DT_WHT && + (dirp->dd_flags & DTF_HIDEW)) + dp->d_fileno = 0; + } + + free(dpv); + break; + } else { + dpv = malloc((n+1) * sizeof(struct dirent *)); + if (dpv == NULL) + break; + } + } + + dirp->dd_len = len; + dirp->dd_size = ddptr - dirp->dd_buf; + return (true); +} + + /* * Common routine for opendir(3), __opendir2(3) and fdopendir(3). */ static DIR * -__opendir_common(int fd, int flags) +__opendir_common(int fd, int flags, bool use_current_pos) { DIR *dirp; int incr; int saved_errno; int unionstack; - int fd2; - - fd2 = -1; if ((dirp = malloc(sizeof(DIR) + sizeof(struct _telldir))) == NULL) return (NULL); + dirp->dd_buf = NULL; + dirp->dd_fd = fd; + dirp->dd_flags = flags; + dirp->dd_loc = 0; + dirp->dd_lock = NULL; dirp->dd_td = (struct _telldir *)((char *)dirp + sizeof(DIR)); LIST_INIT(&dirp->dd_td->td_locq); dirp->dd_td->td_loccnt = 0; @@ -154,163 +321,39 @@ __opendir_common(int fd, int flags) } if (unionstack) { - int len = 0; - int space = 0; - char *buf = 0; - char *ddptr = 0; - char *ddeptr; - int n; - struct dirent **dpv; - - /* - * The strategy here is to read all the directory - * entries into a buffer, sort the buffer, and - * remove duplicate entries by setting the inode - * number to zero. - * - * We reopen the directory because _getdirentries() - * on a MNT_UNION mount modifies the open directory, - * making it refer to the lower directory after the - * upper directory's entries are exhausted. - * This would otherwise break software that uses - * the directory descriptor for fchdir or *at - * functions, such as fts.c. - */ - if ((fd2 = _openat(fd, ".", O_RDONLY | O_CLOEXEC)) == -1) { - saved_errno = errno; - free(buf); - free(dirp); - errno = saved_errno; - return (NULL); - } - - do { - /* - * Always make at least DIRBLKSIZ bytes - * available to _getdirentries - */ - if (space < DIRBLKSIZ) { - space += incr; - len += incr; - buf = reallocf(buf, len); - if (buf == NULL) - goto fail; - ddptr = buf + (len - space); - } - - n = _getdirentries(fd2, ddptr, space, &dirp->dd_seek); - if (n > 0) { - ddptr += n; - space -= n; - } - } while (n > 0); - - ddeptr = ddptr; - flags |= __DTF_READALL; - - _close(fd2); - fd2 = -1; - - /* - * There is now a buffer full of (possibly) duplicate - * names. - */ - dirp->dd_buf = buf; - - /* - * Go round this loop twice... - * - * Scan through the buffer, counting entries. - * On the second pass, save pointers to each one. - * Then sort the pointers and remove duplicate names. - */ - for (dpv = 0;;) { - n = 0; - ddptr = buf; - while (ddptr < ddeptr) { - struct dirent *dp; - - dp = (struct dirent *) ddptr; - if ((long)dp & 03L) - break; - if ((dp->d_reclen <= 0) || - (dp->d_reclen > (ddeptr + 1 - ddptr))) - break; - ddptr += dp->d_reclen; - if (dp->d_fileno) { - if (dpv) - dpv[n] = dp; - n++; - } - } - - if (dpv) { - struct dirent *xp; - - /* - * This sort must be stable. - */ - mergesort(dpv, n, sizeof(*dpv), - opendir_compar); - - dpv[n] = NULL; - xp = NULL; - - /* - * Scan through the buffer in sort order, - * zapping the inode number of any - * duplicate names. - */ - for (n = 0; dpv[n]; n++) { - struct dirent *dp = dpv[n]; - - if ((xp == NULL) || - strcmp(dp->d_name, xp->d_name)) { - xp = dp; - } else { - dp->d_fileno = 0; - } - if (dp->d_type == DT_WHT && - (flags & DTF_HIDEW)) - dp->d_fileno = 0; - } - - free(dpv); - break; - } else { - dpv = malloc((n+1) * sizeof(struct dirent *)); - if (dpv == NULL) - break; - } - } - - dirp->dd_len = len; - dirp->dd_size = ddptr - dirp->dd_buf; + if (!_filldir(dirp, use_current_pos)) + goto fail; + dirp->dd_flags |= __DTF_READALL; } else { dirp->dd_len = incr; - dirp->dd_size = 0; dirp->dd_buf = malloc(dirp->dd_len); if (dirp->dd_buf == NULL) goto fail; - dirp->dd_seek = 0; + if (use_current_pos) { + /* + * Read the first batch of directory entries + * to prime dd_seek. This also checks if the + * fd passed to fdopendir() is a directory. + */ + dirp->dd_size = _getdirentries(dirp->dd_fd, + dirp->dd_buf, dirp->dd_len, &dirp->dd_seek); + if (dirp->dd_size < 0) { + if (errno == EINVAL) + errno = ENOTDIR; + goto fail; + } + dirp->dd_flags |= __DTF_SKIPREAD; + } else { + dirp->dd_size = 0; + dirp->dd_seek = 0; + } } - dirp->dd_loc = 0; - dirp->dd_fd = fd; - dirp->dd_flags = flags; - dirp->dd_lock = NULL; - - /* - * Set up seek point for rewinddir. - */ - dirp->dd_rewind = telldir(dirp); - return (dirp); fail: saved_errno = errno; - if (fd2 != -1) - _close(fd2); + free(dirp->dd_buf); free(dirp); errno = saved_errno; return (NULL); diff --git a/lib/libc/gen/readdir.c b/lib/libc/gen/readdir.c index 324870b39..69f59d160 100644 --- a/lib/libc/gen/readdir.c +++ b/lib/libc/gen/readdir.c @@ -61,12 +61,14 @@ _readdir_unlocked(dirp, skip) return (NULL); dirp->dd_loc = 0; } - if (dirp->dd_loc == 0 && !(dirp->dd_flags & __DTF_READALL)) { + if (dirp->dd_loc == 0 && + !(dirp->dd_flags & (__DTF_READALL | __DTF_SKIPREAD))) { dirp->dd_size = _getdirentries(dirp->dd_fd, dirp->dd_buf, dirp->dd_len, &dirp->dd_seek); if (dirp->dd_size <= 0) return (NULL); } + dirp->dd_flags &= ~__DTF_SKIPREAD; dp = (struct dirent *)(dirp->dd_buf + dirp->dd_loc); if ((long)dp & 03L) /* bogus pointer check */ return (NULL); diff --git a/lib/libc/gen/rewinddir.c b/lib/libc/gen/rewinddir.c index 0eb091a08..89e717cbf 100644 --- a/lib/libc/gen/rewinddir.c +++ b/lib/libc/gen/rewinddir.c @@ -33,9 +33,14 @@ static char sccsid[] = "@(#)rewinddir.c 8.1 (Berkeley) 6/8/93"; #include __FBSDID("$FreeBSD$"); +#include "namespace.h" #include #include +#include +#include +#include "un-namespace.h" +#include "libc_private.h" #include "gen-private.h" #include "telldir.h" @@ -44,6 +49,16 @@ rewinddir(dirp) DIR *dirp; { - _seekdir(dirp, dirp->dd_rewind); - dirp->dd_rewind = telldir(dirp); + if (__isthreaded) + _pthread_mutex_lock(&dirp->dd_lock); + if (dirp->dd_flags & __DTF_READALL) + _filldir(dirp, false); + else if (dirp->dd_seek != 0) { + (void) lseek(dirp->dd_fd, 0, SEEK_SET); + dirp->dd_seek = 0; + } + dirp->dd_loc = 0; + _reclaim_telldir(dirp); + if (__isthreaded) + _pthread_mutex_unlock(&dirp->dd_lock); } diff --git a/lib/libc/gen/telldir.c b/lib/libc/gen/telldir.c index 4954b973a..d72b500b0 100644 --- a/lib/libc/gen/telldir.c +++ b/lib/libc/gen/telldir.c @@ -46,13 +46,6 @@ __FBSDID("$FreeBSD$"); #include "gen-private.h" #include "telldir.h" -/* - * The option SINGLEUSE may be defined to say that a telldir - * cookie may be used only once before it is freed. This option - * is used to avoid having memory usage grow without bound. - */ -#define SINGLEUSE - /* * return a pointer into a directory */ @@ -61,18 +54,31 @@ telldir(dirp) DIR *dirp; { struct ddloc *lp; + long idx; - if ((lp = (struct ddloc *)malloc(sizeof(struct ddloc))) == NULL) - return (-1); if (__isthreaded) _pthread_mutex_lock(&dirp->dd_lock); - lp->loc_index = dirp->dd_td->td_loccnt++; - lp->loc_seek = dirp->dd_seek; - lp->loc_loc = dirp->dd_loc; - LIST_INSERT_HEAD(&dirp->dd_td->td_locq, lp, loc_lqe); + LIST_FOREACH(lp, &dirp->dd_td->td_locq, loc_lqe) { + if (lp->loc_seek == dirp->dd_seek && + lp->loc_loc == dirp->dd_loc) + break; + } + if (lp == NULL) { + lp = malloc(sizeof(struct ddloc)); + if (lp == NULL) { + if (__isthreaded) + _pthread_mutex_unlock(&dirp->dd_lock); + return (-1); + } + lp->loc_index = dirp->dd_td->td_loccnt++; + lp->loc_seek = dirp->dd_seek; + lp->loc_loc = dirp->dd_loc; + LIST_INSERT_HEAD(&dirp->dd_td->td_locq, lp, loc_lqe); + } + idx = lp->loc_index; if (__isthreaded) _pthread_mutex_unlock(&dirp->dd_lock); - return (lp->loc_index); + return (idx); } /* @@ -94,7 +100,7 @@ _seekdir(dirp, loc) if (lp == NULL) return; if (lp->loc_loc == dirp->dd_loc && lp->loc_seek == dirp->dd_seek) - goto found; + return; (void) lseek(dirp->dd_fd, (off_t)lp->loc_seek, SEEK_SET); dirp->dd_seek = lp->loc_seek; dirp->dd_loc = 0; @@ -103,11 +109,6 @@ _seekdir(dirp, loc) if (dp == NULL) break; } -found: -#ifdef SINGLEUSE - LIST_REMOVE(lp, loc_lqe); - free((caddr_t)lp); -#endif } /* diff --git a/lib/libc/gen/telldir.h b/lib/libc/gen/telldir.h index ef930d2b8..04989bb7b 100644 --- a/lib/libc/gen/telldir.h +++ b/lib/libc/gen/telldir.h @@ -36,6 +36,7 @@ #define _TELLDIR_H_ #include +#include /* * One of these structures is malloced to describe the current directory @@ -59,6 +60,7 @@ struct _telldir { long td_loccnt; /* index of entry for sequential readdir's */ }; +bool _filldir(DIR *, bool); struct dirent *_readdir_unlocked(DIR *, int); void _reclaim_telldir(DIR *); void _seekdir(DIR *, long); -- 2.45.0