From 48aaad5fbca3c278bcfee31de4f132a9a63867e1 Mon Sep 17 00:00:00 2001 From: Yaroslav Tykhiy Date: Sat, 26 Jan 2008 17:09:40 +0000 Subject: [PATCH] Our fts(3) API, as inherited from 4.4BSD, suffers from integer fields in FTS and FTSENT structs being too narrow. In addition, the narrow types creep from there into fts.c. As a result, fts(3) consumers, e.g., find(1) or rm(1), can't handle file trees an ordinary user can create, which can have security implications. To fix the historic implementation of fts(3), OpenBSD and NetBSD have already changed in somewhat incompatible ways, so we are free to do so, too. This change is a superset of changes from the other BSDs with a few more improvements. It doesn't touch fts(3) functionality; it just extends integer types used by it to match modern reality and the C standard. Here are its points: o For C 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.) o Avoid the short types because on modern platforms using them results in larger and slower code. Change shorts to ints as follows: - For variables than count simple, limited things like states, use plain vanilla `int' as it's the type of choice in C. - For a limited number of bit flags use `unsigned' because signed bit-wise operations are implementation-defined, i.e., unportable, in C. o For things that should be at least 64 bits wide, use long long and not int64_t, as the latter is an optional type. See FTSENT.fts_number aka FTS.fts_bignum. Extending fts_number `to satisfy future needs' is pointless because there is fts_pointer, which can be used to link to arbitrary data from an FTSENT. However, there already are fts(3) consumers that require fts_number, or fts_bignum, have at least 64 bits in it, so we must allow for them. o For the tree depth, use `long'. This is a trade-off between making this field too wide and allowing for 64-bit inode numbers and/or chain-mounted filesystems. On the one hand, `long' is almost enough for 32-bit filesystems on a 32-bit platform (our ino_t is uint32_t now). On the other hand, platforms with a 64-bit (or wider) `long' will be ready for 64-bit inode numbers, as well as for several 32-bit filesystems mounted one under another. Note that fts_level has to be signed because -1 is a magic value for it, FTS_ROOTPARENTLEVEL. o For the `nlinks' local var in fts_build(), use `long'. The logic in fts_build() requires that `nlinks' be signed, but our nlink_t currently is uint16_t. Therefore let's make the signed var wide enough to be able to represent 2^16-1 in pure C99, and even 2^32-1 on a 64-bit platform. Perhaps the logic should be changed just to use nlink_t, but it can be done later w/o breaking fts(3) ABI any more because `nlinks' is just a local var. This commit also inludes supporting stuff for the fts change: o Preserve the old versions of fts(3) functions through libc symbol versioning because the old versions appeared in all our former releases. o Bump __FreeBSD_version just in case. There is a small chance that some ill-written 3-rd party apps may fail to build or work correctly if compiled after this change. o Update the fts(3) manpage accordingly. In particular, remove references to fts_bignum, which was a FreeBSD-specific hack to work around the too narrow types of FTSENT members. Now fts_number is at least 64 bits wide (long long) and fts_bignum is an undocumented alias for fts_number kept around for compatibility reasons. According to Google Code Search, the only big consumers of fts_bignum are in our own source tree, so they can be fixed easily to use fts_number. o Mention the change in src/UPDATING. PR: bin/104458 Approved by: re (quite a while ago) Discussed with: deischen (the symbol versioning part) Reviewed by: -arch (mostly silence); das (generally OK, but we didn't agree on some types used; assuming that no objections on -arch let me to stick to my opinion) --- UPDATING | 14 ++++++++++ include/fts.h | 29 ++++++++------------- lib/libc/gen/Makefile.inc | 2 +- lib/libc/gen/Symbol.map | 19 ++++++++------ lib/libc/gen/fts-compat.c | 37 ++++++++++++++++++++------- lib/libc/gen/fts-compat.h | 13 ---------- lib/libc/gen/fts.3 | 27 +++++--------------- lib/libc/gen/fts.c | 54 ++++++++++----------------------------- sys/sys/param.h | 2 +- 9 files changed, 85 insertions(+), 112 deletions(-) diff --git a/UPDATING b/UPDATING index 1143e84965d..8cac2e8bdc6 100644 --- a/UPDATING +++ b/UPDATING @@ -22,6 +22,20 @@ NOTE TO PEOPLE WHO THINK THAT FreeBSD 8.x IS SLOW: to maximize performance. (To disable malloc debugging, run ln -s aj /etc/malloc.conf.) +20080126: + The fts(3) structures have been changed to use adequate + integer types for their members and so to be able to cope + with huge file trees. The old fts(3) ABI is preserved + through symbol versioning in libc, so third-party binaries + using fts(3) should still work, although they will not take + advantage of the extended types. At the same time, some + third-party software might fail to build after this change + due to unportable assumptions made in its source code about + fts(3) structure members. Such software should be fixed + by its vendor or, in the worst case, in the ports tree. + FreeBSD_version 800015 marks this change for the unlikely + case that a portable fix is impossible. + 20080123: To upgrade to -current after this date, you must be running FreeBSD not older than 6.0-RELEASE. Upgrading to -current diff --git a/include/fts.h b/include/fts.h index 7eb03a64994..cd3da87b679 100644 --- a/include/fts.h +++ b/include/fts.h @@ -44,8 +44,8 @@ typedef struct { 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 @@ typedef struct _ftsent { 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 @@ typedef struct _ftsent { #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 @@ typedef struct _ftsent { #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 */ + unsigned 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 */ diff --git a/lib/libc/gen/Makefile.inc b/lib/libc/gen/Makefile.inc index 10c6ceb123f..ea4366295a8 100644 --- a/lib/libc/gen/Makefile.inc +++ b/lib/libc/gen/Makefile.inc @@ -11,7 +11,7 @@ SRCS+= __getosreldate.c __xuname.c \ crypt.c ctermid.c daemon.c devname.c dirname.c disklabel.c \ dlfcn.c dlfunc.c drand48.c erand48.c err.c errlst.c errno.c \ exec.c feature_present.c fmtcheck.c fmtmsg.c fnmatch.c \ - fpclassify.c frexp.c fstab.c ftok.c fts.c ftw.c \ + fpclassify.c frexp.c fstab.c ftok.c fts.c fts-compat.c ftw.c \ getbootfile.c getbsize.c \ getcap.c getcwd.c getdomainname.c getgrent.c getgrouplist.c \ gethostname.c getloadavg.c getlogin.c getmntinfo.c getnetgrent.c \ diff --git a/lib/libc/gen/Symbol.map b/lib/libc/gen/Symbol.map index e2f62d643c6..8167aaac023 100644 --- a/lib/libc/gen/Symbol.map +++ b/lib/libc/gen/Symbol.map @@ -134,14 +134,6 @@ FBSD_1.0 { setfsent; endfsent; ftok; - fts_open; - fts_close; - fts_read; - fts_set; - fts_children; - fts_get_clientptr; - fts_get_stream; - fts_set_clientptr; ftw; glob; globfree; @@ -336,6 +328,17 @@ FBSD_1.0 { wordfree; }; +FBSD_1.1 { + fts_open; + fts_close; + fts_read; + fts_set; + fts_children; + fts_get_clientptr; + fts_get_stream; + fts_set_clientptr; +}; + FBSDprivate_1.0 { /* needed by thread libraries */ __thr_jtable; diff --git a/lib/libc/gen/fts-compat.c b/lib/libc/gen/fts-compat.c index 83f9c67a203..3afb87d7b73 100644 --- a/lib/libc/gen/fts-compat.c +++ b/lib/libc/gen/fts-compat.c @@ -46,12 +46,22 @@ __FBSDID("$FreeBSD$"); #include #include #include -#include #include #include #include +#include "fts-compat.h" #include "un-namespace.h" +FTSENT *__fts_children_44bsd(FTS *, int); +int __fts_close_44bsd(FTS *); +void *__fts_get_clientptr_44bsd(FTS *); +FTS *__fts_get_stream_44bsd(FTSENT *); +FTS *__fts_open_44bsd(char * const *, int, + int (*)(const FTSENT * const *, const FTSENT * const *)); +FTSENT *__fts_read_44bsd(FTS *); +int __fts_set_44bsd(FTS *, FTSENT *, int); +void __fts_set_clientptr_44bsd(FTS *, void *); + static FTSENT *fts_alloc(FTS *, char *, int); static FTSENT *fts_build(FTS *, int); static void fts_lfree(FTSENT *); @@ -107,7 +117,7 @@ static const char *ufslike_filesystems[] = { }; FTS * -fts_open(argv, options, compar) +__fts_open_44bsd(argv, options, compar) char * const *argv; int options; int (*compar)(const FTSENT * const *, const FTSENT * const *); @@ -246,7 +256,7 @@ fts_load(sp, p) } int -fts_close(sp) +__fts_close_44bsd(sp) FTS *sp; { FTSENT *freep, *p; @@ -301,7 +311,7 @@ fts_close(sp) ? p->fts_pathlen - 1 : p->fts_pathlen) FTSENT * -fts_read(sp) +__fts_read_44bsd(sp) FTS *sp; { FTSENT *p, *tmp; @@ -495,7 +505,7 @@ name: t = sp->fts_path + NAPPEND(p->fts_parent); */ /* ARGSUSED */ int -fts_set(sp, p, instr) +__fts_set_44bsd(sp, p, instr) FTS *sp; FTSENT *p; int instr; @@ -510,7 +520,7 @@ fts_set(sp, p, instr) } FTSENT * -fts_children(sp, instr) +__fts_children_44bsd(sp, instr) FTS *sp; int instr; { @@ -582,7 +592,7 @@ fts_children(sp, instr) #endif void * -(fts_get_clientptr)(FTS *sp) +(__fts_get_clientptr_44bsd)(FTS *sp) { return (fts_get_clientptr(sp)); @@ -593,13 +603,13 @@ void * #endif FTS * -(fts_get_stream)(FTSENT *p) +(__fts_get_stream_44bsd)(FTSENT *p) { return (fts_get_stream(p)); } void -fts_set_clientptr(FTS *sp, void *clientptr) +__fts_set_clientptr_44bsd(FTS *sp, void *clientptr) { sp->fts_clientptr = clientptr; @@ -1220,3 +1230,12 @@ fts_ufslinks(FTS *sp, const FTSENT *ent) } return (priv->ftsp_linksreliable); } + +__sym_compat(fts_open, __fts_open_44bsd, FBSD_1.0); +__sym_compat(fts_close, __fts_close_44bsd, FBSD_1.0); +__sym_compat(fts_read, __fts_read_44bsd, FBSD_1.0); +__sym_compat(fts_set, __fts_set_44bsd, FBSD_1.0); +__sym_compat(fts_children, __fts_children_44bsd, FBSD_1.0); +__sym_compat(fts_get_clientptr, __fts_get_clientptr_44bsd, FBSD_1.0); +__sym_compat(fts_get_stream, __fts_get_stream_44bsd, FBSD_1.0); +__sym_compat(fts_set_clientptr, __fts_set_clientptr_44bsd, FBSD_1.0); diff --git a/lib/libc/gen/fts-compat.h b/lib/libc/gen/fts-compat.h index 7eb03a64994..dfdd53bfb65 100644 --- a/lib/libc/gen/fts-compat.h +++ b/lib/libc/gen/fts-compat.h @@ -126,20 +126,7 @@ typedef struct _ftsent { FTS *fts_fts; /* back pointer to main FTS */ } FTSENT; -#include - -__BEGIN_DECLS -FTSENT *fts_children(FTS *, int); -int fts_close(FTS *); -void *fts_get_clientptr(FTS *); #define fts_get_clientptr(fts) ((fts)->fts_clientptr) -FTS *fts_get_stream(FTSENT *); #define fts_get_stream(ftsent) ((ftsent)->fts_fts) -FTS *fts_open(char * const *, int, - int (*)(const FTSENT * const *, const FTSENT * const *)); -FTSENT *fts_read(FTS *); -int fts_set(FTS *, FTSENT *, int); -void fts_set_clientptr(FTS *, void *); -__END_DECLS #endif /* !_FTS_H_ */ diff --git a/lib/libc/gen/fts.3 b/lib/libc/gen/fts.3 index ad6e513b5d2..8e1a1e42269 100644 --- a/lib/libc/gen/fts.3 +++ b/lib/libc/gen/fts.3 @@ -28,7 +28,7 @@ .\" @(#)fts.3 8.5 (Berkeley) 4/16/94 .\" $FreeBSD$ .\" -.Dd January 7, 2005 +.Dd January 26, 2008 .Dt FTS 3 .Os .Sh NAME @@ -133,17 +133,16 @@ structure contains at least the following fields, which are described in greater detail below: .Bd -literal typedef struct _ftsent { - u_short fts_info; /* flags for FTSENT structure */ + int fts_info; /* status for FTSENT structure */ char *fts_accpath; /* access path */ char *fts_path; /* root path */ - u_short fts_pathlen; /* strlen(fts_path) */ + size_t fts_pathlen; /* strlen(fts_path) */ char *fts_name; /* file name */ - u_short fts_namelen; /* strlen(fts_name) */ - short fts_level; /* depth (\-1 to N) */ + size_t fts_namelen; /* strlen(fts_name) */ + long fts_level; /* depth (\-1 to N) */ int fts_errno; /* file errno */ - long fts_number; /* local numeric value */ + long long fts_number; /* local numeric value */ void *fts_pointer; /* local address value */ - int64_t fts_bignum; /* local 64-bit numeric value */ struct ftsent *fts_parent; /* parent directory */ struct ftsent *fts_link; /* next file structure */ struct ftsent *fts_cycle; /* cycle structure */ @@ -292,8 +291,6 @@ not modified by the .Nm functions. It is initialized to 0. -Note that this field is overlaid by -.Fa fts_bignum . .It Fa fts_pointer This field is provided for the use of the application program and is not modified by the @@ -301,18 +298,6 @@ not modified by the functions. It is initialized to .Dv NULL . -Note that this field is overlaid by -.Fa fts_bignum . -.It Fa fts_bignum -This field is provided for the use of the application program and is -not modified by the -.Nm -functions. -It is initialized to 0. -Note that this field overlays -.Fa fts_number -and -.Fa fts_pointer . .It Fa fts_parent A pointer to the .Vt FTSENT diff --git a/lib/libc/gen/fts.c b/lib/libc/gen/fts.c index 83f9c67a203..ced205b04a5 100644 --- a/lib/libc/gen/fts.c +++ b/lib/libc/gen/fts.c @@ -52,15 +52,15 @@ __FBSDID("$FreeBSD$"); #include #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 @@ fts_open(argv, options, compar) 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_load(sp, p) FTS *sp; FTSENT *p; { - int len; + size_t len; char *cp; /* @@ -626,14 +625,14 @@ fts_build(sp, type) { 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 @@ fts_build(sp, type) 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 @@ mem1: saved_errno = errno; 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 @@ mem1: saved_errno = errno; return (head); } -static u_short +static int fts_stat(sp, p, follow) FTS *sp; FTSENT *p; @@ -987,7 +971,7 @@ static FTSENT * fts_sort(sp, head, nitems) FTS *sp; FTSENT *head; - int nitems; + size_t nitems; { FTSENT **ap, *p; @@ -1019,7 +1003,7 @@ static FTSENT * fts_alloc(sp, name, namelen) FTS *sp; char *name; - int namelen; + size_t namelen; { FTSENT *p; size_t len; @@ -1091,18 +1075,6 @@ fts_palloc(sp, more) { 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); } diff --git a/sys/sys/param.h b/sys/sys/param.h index d3f81c90642..385571a6f64 100644 --- a/sys/sys/param.h +++ b/sys/sys/param.h @@ -57,7 +57,7 @@ * is created, otherwise 1. */ #undef __FreeBSD_version -#define __FreeBSD_version 800014 /* Master, propagated to newvers */ +#define __FreeBSD_version 800015 /* Master, propagated to newvers */ #ifndef LOCORE #include