Date: Tue, 19 Jun 2007 00:27:58 +0400 From: Yar Tikhiy <yar@comp.chem.msu.su> To: current@freebsd.org Subject: fts(3) patch for review Message-ID: <20070618202758.GA16711@comp.chem.msu.su>
next in thread | raw e-mail | index | archive | help
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
@@ -44,8 +44,8 @@
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 *);
@@ -69,22 +69,15 @@
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 */
@@ -92,7 +85,7 @@
#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 */
@@ -108,18 +101,18 @@
#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
@@ -52,15 +52,15 @@
#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 *);
@@ -115,9 +115,8 @@
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) {
@@ -224,7 +223,7 @@
FTS *sp;
FTSENT *p;
{
- int len;
+ size_t len;
char *cp;
/*
@@ -626,14 +625,14 @@
{
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;
@@ -741,7 +740,7 @@
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;
@@ -770,21 +769,6 @@
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;
@@ -885,7 +869,7 @@
return (head);
}
-static u_short
+static int
fts_stat(sp, p, follow)
FTS *sp;
FTSENT *p;
@@ -987,7 +971,7 @@
fts_sort(sp, head, nitems)
FTS *sp;
FTSENT *head;
- int nitems;
+ size_t nitems;
{
FTSENT **ap, *p;
@@ -1019,7 +1003,7 @@
fts_alloc(sp, name, namelen)
FTS *sp;
char *name;
- int namelen;
+ size_t namelen;
{
FTSENT *p;
size_t len;
@@ -1091,18 +1075,6 @@
{
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070618202758.GA16711>