fts(3) patch for review

Hi all,

Our fts(3) functions and data structures suffer from too narrow
integer types, which apparently were selected in ancient days when
RAM was costlier than gold.  The consequences of that choice are
illustrated by PR bin/104458.  In short, find(1) can't walk and
rm(1) can't remove file trees an ordinary user can create.

To fix the problem, structures in <fts.h> have to be changed.  For
my change (attached below), I chose new types using the following
principles I believe to be well-known in the C world:

- avoid `short' unless there is a very grave reason to try to
  save RAM -- on modern platforms using `short' results in larger
  and slower code;
- for object sizes, use size_t unless it's 100% certain that
  the object will be really small (note that fts(3) can construct
  pathnames _much_ longer than PATH_MAX for its consumers);
- for variables than count simple, limited things like states,
  use plain vanilla `int' as it's the type of choice in C;
- for bit flags use u_int because signed bit-wise operations
  are unportable in C;
- for things that should be at least 64 bits wide, use long long
  and not int64_t, as the latter is an optional type.

An open question is what type to use for the level.  Since one can
chain-mount several filesystems, theoretically the level can be
greater than the maximum value of ino_t, which is 2^32-1.  OTOH, I
doubt that the limit can be hit in practice, especially on 32-bit
systems, so `long' can be a fair compromise for the level.

Comments are welcome.  Thanks!

P.S. According to my tests, the stock system tools happily build
and run with the modified fts(3).

-- 
Yar

--- //depot/vendor/freebsd/src/include/fts.h	2005/01/07 00:37:13
+++ //depot/user/yar/hack/include/fts.h	2007/06/18 08:58:52
_at__at_ -44,8 +44,8 _at__at_
 	dev_t fts_dev;			/* starting device # */
 	char *fts_path;			/* path for this descent */
 	int fts_rfd;			/* fd for root */
-	int fts_pathlen;		/* sizeof(path) */
-	int fts_nitems;			/* elements in the sort array */
+	size_t fts_pathlen;		/* sizeof(path) */
+	size_t fts_nitems;		/* elements in the sort array */
 	int (*fts_compar)		/* compare function */
 	    (const struct _ftsent * const *, const struct _ftsent * const *);
 
_at__at_ -69,22 +69,15 _at__at_
 	struct _ftsent *fts_cycle;	/* cycle node */
 	struct _ftsent *fts_parent;	/* parent directory */
 	struct _ftsent *fts_link;	/* next file in directory */
-	union {
-		struct {
-			long __fts_number;	/* local numeric value */
-			void *__fts_pointer;	/* local address value */
-		} __struct_ftsent;
-		int64_t __fts_bignum;
-	} __union_ftsent;
-#define	fts_number	__union_ftsent.__struct_ftsent.__fts_number
-#define	fts_pointer	__union_ftsent.__struct_ftsent.__fts_pointer
-#define	fts_bignum	__union_ftsent.__fts_bignum
+	long long fts_number;		/* local numeric value */
+#define	fts_bignum	fts_number	/* XXX non-std, should go away */
+	void *fts_pointer;		/* local address value */
 	char *fts_accpath;		/* access path */
 	char *fts_path;			/* root path */
 	int fts_errno;			/* errno for this node */
 	int fts_symfd;			/* fd for symlink */
-	u_short fts_pathlen;		/* strlen(fts_path) */
-	u_short fts_namelen;		/* strlen(fts_name) */
+	size_t fts_pathlen;		/* strlen(fts_path) */
+	size_t fts_namelen;		/* strlen(fts_name) */
 
 	ino_t fts_ino;			/* inode */
 	dev_t fts_dev;			/* device */
_at__at_ -92,7 +85,7 _at__at_
 
 #define	FTS_ROOTPARENTLEVEL	-1
 #define	FTS_ROOTLEVEL		 0
-	short fts_level;		/* depth (-1 to N) */
+	long fts_level;			/* depth (-1 to N) */
 
 #define	FTS_D		 1		/* preorder directory */
 #define	FTS_DC		 2		/* directory that causes cycles */
_at__at_ -108,18 +101,18 _at__at_
 #define	FTS_SL		12		/* symbolic link */
 #define	FTS_SLNONE	13		/* symbolic link without target */
 #define	FTS_W		14		/* whiteout object */
-	u_short fts_info;		/* user flags for FTSENT structure */
+	int fts_info;			/* user status for FTSENT structure */
 
 #define	FTS_DONTCHDIR	 0x01		/* don't chdir .. to the parent */
 #define	FTS_SYMFOLLOW	 0x02		/* followed a symlink to get here */
 #define	FTS_ISW		 0x04		/* this is a whiteout object */
-	u_short fts_flags;		/* private flags for FTSENT structure */
+	u_int fts_flags;		/* private flags for FTSENT structure */
 
 #define	FTS_AGAIN	 1		/* read node again */
 #define	FTS_FOLLOW	 2		/* follow symbolic link */
 #define	FTS_NOINSTR	 3		/* no instructions */
 #define	FTS_SKIP	 4		/* discard node */
-	u_short fts_instr;		/* fts_set() instructions */
+	int fts_instr;			/* fts_set() instructions */
 
 	struct stat *fts_statp;		/* stat(2) information */
 	char *fts_name;			/* file name */
--- //depot/vendor/freebsd/src/lib/libc/gen/fts.c	2007/01/10 16:42:27
+++ //depot/user/yar/hack/lib/libc/gen/fts.c	2007/06/18 08:58:52
_at__at_ -52,15 +52,15 _at__at_
 #include <unistd.h>
 #include "un-namespace.h"
 
-static FTSENT	*fts_alloc(FTS *, char *, int);
+static FTSENT	*fts_alloc(FTS *, char *, size_t);
 static FTSENT	*fts_build(FTS *, int);
 static void	 fts_lfree(FTSENT *);
 static void	 fts_load(FTS *, FTSENT *);
 static size_t	 fts_maxarglen(char * const *);
 static void	 fts_padjust(FTS *, FTSENT *);
 static int	 fts_palloc(FTS *, size_t);
-static FTSENT	*fts_sort(FTS *, FTSENT *, int);
-static u_short	 fts_stat(FTS *, FTSENT *, int);
+static FTSENT	*fts_sort(FTS *, FTSENT *, size_t);
+static int	 fts_stat(FTS *, FTSENT *, int);
 static int	 fts_safe_changedir(FTS *, FTSENT *, int, char *);
 static int	 fts_ufslinks(FTS *, const FTSENT *);
 
_at__at_ -115,9 +115,8 _at__at_
 	struct _fts_private *priv;
 	FTS *sp;
 	FTSENT *p, *root;
-	int nitems;
 	FTSENT *parent, *tmp;
-	int len;
+	size_t len, nitems;
 
 	/* Options check. */
 	if (options & ~FTS_OPTIONMASK) {
_at__at_ -224,7 +223,7 _at__at_
 	FTS *sp;
 	FTSENT *p;
 {
-	int len;
+	size_t len;
 	char *cp;
 
 	/*
_at__at_ -626,14 +625,14 _at__at_
 {
 	struct dirent *dp;
 	FTSENT *p, *head;
-	int nitems;
 	FTSENT *cur, *tail;
 	DIR *dirp;
 	void *oldaddr;
-	size_t dnamlen;
-	int cderrno, descend, len, level, maxlen, nlinks, oflag, saved_errno,
-	    nostat, doadjust;
 	char *cp;
+	int cderrno, descend, oflag, saved_errno, nostat, doadjust;
+	long level;
+	long nlinks;	/* has to be signed because -1 is a magic value */
+	size_t dnamlen, len, maxlen, nitems;
 
 	/* Set current node pointer. */
 	cur = sp->fts_cur;
_at__at_ -741,7 +740,7 _at__at_
 		if (!ISSET(FTS_SEEDOT) && ISDOT(dp->d_name))
 			continue;
 
-		if ((p = fts_alloc(sp, dp->d_name, (int)dnamlen)) == NULL)
+		if ((p = fts_alloc(sp, dp->d_name, dnamlen)) == NULL)
 			goto mem1;
 		if (dnamlen >= maxlen) {	/* include space for NUL */
 			oldaddr = sp->fts_path;
_at__at_ -770,21 +769,6 _at__at_
 			maxlen = sp->fts_pathlen - len;
 		}
 
-		if (len + dnamlen >= USHRT_MAX) {
-			/*
-			 * In an FTSENT, fts_pathlen is a u_short so it is
-			 * possible to wraparound here.  If we do, free up
-			 * the current structure and the structures already
-			 * allocated, then error out with ENAMETOOLONG.
-			 */
-			free(p);
-			fts_lfree(head);
-			(void)closedir(dirp);
-			cur->fts_info = FTS_ERR;
-			SET(FTS_STOP);
-			errno = ENAMETOOLONG;
-			return (NULL);
-		}
 		p->fts_level = level;
 		p->fts_parent = sp->fts_cur;
 		p->fts_pathlen = len + dnamlen;
_at__at_ -885,7 +869,7 _at__at_
 	return (head);
 }
 
-static u_short
+static int
 fts_stat(sp, p, follow)
 	FTS *sp;
 	FTSENT *p;
_at__at_ -987,7 +971,7 _at__at_
 fts_sort(sp, head, nitems)
 	FTS *sp;
 	FTSENT *head;
-	int nitems;
+	size_t nitems;
 {
 	FTSENT **ap, *p;
 
_at__at_ -1019,7 +1003,7 _at__at_
 fts_alloc(sp, name, namelen)
 	FTS *sp;
 	char *name;
-	int namelen;
+	size_t namelen;
 {
 	FTSENT *p;
 	size_t len;
_at__at_ -1091,18 +1075,6 _at__at_
 {
 
 	sp->fts_pathlen += more + 256;
-	/*
-	 * Check for possible wraparound.  In an FTS, fts_pathlen is
-	 * a signed int but in an FTSENT it is an unsigned short.
-	 * We limit fts_pathlen to USHRT_MAX to be safe in both cases.
-	 */
-	if (sp->fts_pathlen < 0 || sp->fts_pathlen >= USHRT_MAX) {
-		if (sp->fts_path)
-			free(sp->fts_path);
-		sp->fts_path = NULL;
-		errno = ENAMETOOLONG;
-		return (1);
-	}
 	sp->fts_path = reallocf(sp->fts_path, sp->fts_pathlen);
 	return (sp->fts_path == NULL);
 }