Date: Thu, 14 Aug 2014 20:20:22 +0000 (UTC) From: John Baldwin <jhb@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-10@freebsd.org Subject: svn commit: r270002 - in stable/10: include lib/libc/gen Message-ID: <201408142020.s7EKKMP5085498@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: jhb Date: Thu Aug 14 20:20:21 2014 New Revision: 270002 URL: http://svnweb.freebsd.org/changeset/base/270002 Log: 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 Modified: stable/10/include/dirent.h stable/10/lib/libc/gen/directory.3 stable/10/lib/libc/gen/gen-private.h stable/10/lib/libc/gen/opendir.c stable/10/lib/libc/gen/readdir.c stable/10/lib/libc/gen/rewinddir.c stable/10/lib/libc/gen/telldir.c stable/10/lib/libc/gen/telldir.h Directory Properties: stable/10/ (props changed) Modified: stable/10/include/dirent.h ============================================================================== --- stable/10/include/dirent.h Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/include/dirent.h Thu Aug 14 20:20:21 2014 (r270002) @@ -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: stable/10/lib/libc/gen/directory.3 ============================================================================== --- stable/10/lib/libc/gen/directory.3 Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/lib/libc/gen/directory.3 Thu Aug 14 20:20:21 2014 (r270002) @@ -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 asso 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 Modified: stable/10/lib/libc/gen/gen-private.h ============================================================================== --- stable/10/lib/libc/gen/gen-private.h Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/lib/libc/gen/gen-private.h Thu Aug 14 20:20:21 2014 (r270002) @@ -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: stable/10/lib/libc/gen/opendir.c ============================================================================== --- stable/10/lib/libc/gen/opendir.c Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/lib/libc/gen/opendir.c Thu Aug 14 20:20:21 2014 (r270002) @@ -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: stable/10/lib/libc/gen/readdir.c ============================================================================== --- stable/10/lib/libc/gen/readdir.c Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/lib/libc/gen/readdir.c Thu Aug 14 20:20:21 2014 (r270002) @@ -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: stable/10/lib/libc/gen/rewinddir.c ============================================================================== --- stable/10/lib/libc/gen/rewinddir.c Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/lib/libc/gen/rewinddir.c Thu Aug 14 20:20:21 2014 (r270002) @@ -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,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); } Modified: stable/10/lib/libc/gen/telldir.c ============================================================================== --- stable/10/lib/libc/gen/telldir.c Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/lib/libc/gen/telldir.c Thu Aug 14 20:20:21 2014 (r270002) @@ -47,13 +47,6 @@ __FBSDID("$FreeBSD$"); #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 */ long @@ -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 } /* Modified: stable/10/lib/libc/gen/telldir.h ============================================================================== --- stable/10/lib/libc/gen/telldir.h Thu Aug 14 20:17:23 2014 (r270001) +++ stable/10/lib/libc/gen/telldir.h Thu Aug 14 20:20:21 2014 (r270002) @@ -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?201408142020.s7EKKMP5085498>