Date: Fri, 11 Jul 2014 16:16:26 +0000 (UTC) From: John Baldwin <jhb@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r268531 - in head: include lib/libc/gen Message-ID: <201407111616.s6BGGQFW060195@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: jhb Date: Fri Jul 11 16:16:26 2014 New Revision: 268531 URL: http://svnweb.freebsd.org/changeset/base/268531 Log: Fix some edge cases with rewinddir(): - 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. While here, add missing locking to rewinddir(). CR: https://phabric.freebsd.org/D312 Reviewed by: jilles MFC after: 1 week Modified: head/include/dirent.h head/lib/libc/gen/gen-private.h head/lib/libc/gen/opendir.c head/lib/libc/gen/readdir.c head/lib/libc/gen/rewinddir.c head/lib/libc/gen/telldir.h Modified: head/include/dirent.h ============================================================================== --- head/include/dirent.h Fri Jul 11 14:34:29 2014 (r268530) +++ head/include/dirent.h Fri Jul 11 16:16:26 2014 (r268531) @@ -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 */ Modified: head/lib/libc/gen/gen-private.h ============================================================================== --- head/lib/libc/gen/gen-private.h Fri Jul 11 14:34:29 2014 (r268530) +++ head/lib/libc/gen/gen-private.h Fri Jul 11 16:16:26 2014 (r268531) @@ -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 */ Modified: head/lib/libc/gen/opendir.c ============================================================================== --- head/lib/libc/gen/opendir.c Fri Jul 11 14:34:29 2014 (r268530) +++ head/lib/libc/gen/opendir.c Fri Jul 11 16:16:26 2014 (r268531) @@ -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); @@ -110,22 +104,195 @@ opendir_compar(const void *p1, const voi } /* + * 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); Modified: head/lib/libc/gen/readdir.c ============================================================================== --- head/lib/libc/gen/readdir.c Fri Jul 11 14:34:29 2014 (r268530) +++ head/lib/libc/gen/readdir.c Fri Jul 11 16:16:26 2014 (r268531) @@ -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); Modified: head/lib/libc/gen/rewinddir.c ============================================================================== --- head/lib/libc/gen/rewinddir.c Fri Jul 11 14:34:29 2014 (r268530) +++ head/lib/libc/gen/rewinddir.c Fri Jul 11 16:16:26 2014 (r268531) @@ -33,9 +33,14 @@ static char sccsid[] = "@(#)rewinddir.c #include <sys/cdefs.h> __FBSDID("$FreeBSD$"); +#include "namespace.h" #include <sys/types.h> #include <dirent.h> +#include <pthread.h> +#include <unistd.h> +#include "un-namespace.h" +#include "libc_private.h" #include "gen-private.h" #include "telldir.h" @@ -44,6 +49,15 @@ 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; + if (__isthreaded) + _pthread_mutex_unlock(&dirp->dd_lock); } Modified: head/lib/libc/gen/telldir.h ============================================================================== --- head/lib/libc/gen/telldir.h Fri Jul 11 14:34:29 2014 (r268530) +++ head/lib/libc/gen/telldir.h Fri Jul 11 16:16:26 2014 (r268531) @@ -36,6 +36,7 @@ #define _TELLDIR_H_ #include <sys/queue.h> +#include <stdbool.h> /* * 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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201407111616.s6BGGQFW060195>