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

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 */
 

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 */

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);

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);

lib/libc/gen/rewinddir.c

@@ -33,9 +33,14 @@ static char sccsid[] = "@(#)rewinddir.c	8.1 (Berkeley) 6/8/93";
 #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);
 }

lib/libc/gen/telldir.h

@@ -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);