bin/143732: [patch] mtree(8) does a full hierarchy walk when requested to just update structure (-u -e)

[David Naylor]

The following reply was made to PR bin/143732; it has been noted by GNATS.

From: David Naylor <naylor.b.david at gmail.com>
To: bug-followup at freebsd.org
Cc:  
Subject: Re: bin/143732: [patch] mtree(8) does a full hierarchy walk when requested to just update structure (-u -e)
Date: Wed, 1 Sep 2010 09:43:53 +0200

 --Boundary-00=_8QgfMw2WK9C4H4q
 Content-Type: Text/Plain;
   charset="us-ascii"
 Content-Transfer-Encoding: 7bit
 
 Thanks to the feedback from the hackers ML I've updated the patch.  It now is 
 more aggressive in using the optimisation and handles buffer overflows (that 
 previously went unhandled by mtree).  
 
 Please see attached for the patch.
 
 --Boundary-00=_8QgfMw2WK9C4H4q
 Content-Type: text/x-patch;
   charset="ISO-8859-1";
   name="mtree.diff"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
 	filename="mtree.diff"
 
 --- /usr/src/usr.sbin/mtree/verify.c	2007-12-07 14:22:38.000000000 +0200
 +++ verify.c	2010-08-30 22:32:32.000000000 +0200
 @@ -47,20 +47,35 @@
  #include "mtree.h"
  #include "extern.h"
  
 +/*
 + * Error returned when buffer overflows, cannot be 2 as that is reserved for
 + * MISMATCHEXIT.
 + */
 +#define BUFFEROVERFLOWEXIT 3
 +
  static NODE *root;
  static char path[MAXPATHLEN];
  
 -static void	miss(NODE *, char *);
 +static int	miss(NODE *, char *, size_t);
 +static int	check(NODE *, char *, size_t);
  static int	vwalk(void);
  
  int
  mtree_verifyspec(FILE *fi)
  {
 -	int rval;
 +	int rval = 0;
  
  	root = mtree_readspec(fi);
 -	rval = vwalk();
 -	miss(root, path);
 +	/* No need to walk tree if we are ignoring extra files. */
 +	if (!eflag)
 +		rval = vwalk();
 +	*path = '\0';
 +	rval |= miss(root, path, MAXPATHLEN);
 +
 +	/* Called here to make sure vwalk() and check() have been called */
 +	if (sflag)
 +		warnx("%s checksum: %lu", fullpath, (unsigned long)crc_total);
 +
  	return (rval);
  }
  
 @@ -135,40 +150,96 @@
  		if (ep)
  			continue;
  extra:
 -		if (!eflag) {
 -			(void)printf("%s extra", RP(p));
 -			if (rflag) {
 -				if ((S_ISDIR(p->fts_statp->st_mode)
 -				    ? rmdir : unlink)(p->fts_accpath)) {
 -					(void)printf(", not removed: %s",
 -					    strerror(errno));
 -				} else
 -					(void)printf(", removed");
 -			}
 -			(void)putchar('\n');
 +		(void)printf("%s extra", RP(p));
 +		if (rflag) {
 +			if ((S_ISDIR(p->fts_statp->st_mode)
 +			    ? rmdir : unlink)(p->fts_accpath)) {
 +				(void)printf(", not removed: %s",
 +				    strerror(errno));
 +			} else
 +				(void)printf(", removed");
  		}
 +		(void)putchar('\n');
  		(void)fts_set(t, p, FTS_SKIP);
  	}
  	(void)fts_close(t);
 -	if (sflag)
 -		warnx("%s checksum: %lu", fullpath, (unsigned long)crc_total);
  	return (rval);
  }
  
 -static void
 -miss(NODE *p, char *tail)
 +static int
 +check(NODE *p, char *tail, size_t tail_len)
 +{
 +	FTSENT fts;
 +	struct stat fts_stat;
 +
 +	if (strlcpy(tail, p->name, tail_len) >= tail_len)
 +		return (BUFFEROVERFLOWEXIT);
 +
 +	/*
 +	 * It is assumed that compare() only requires fts_accpath and fts_statp
 +	 * fields in the FTSENT structure.
 +	 */
 +	fts.fts_accpath = path;
 +	fts.fts_statp = &fts_stat;
 +
 +	if (ftsoptions & FTS_LOGICAL) {
 +		if (stat(path, fts.fts_statp) || lstat(path, fts.fts_statp))
 +			return (0);
 +	} else if (lstat(path, fts.fts_statp))
 +		return (0);
 +
 +	p->flags |= F_VISIT;
 +	if ((p->flags & F_NOCHANGE) == 0 && compare(p->name, p, &fts))
 +		return (MISMATCHEXIT);
 +	else
 +		return (0);
 +
 +	/*
 +	 * tail is not restored to '\0' as the next time tail (or path) is used
 +	 * is with a strlcpy (thus overriding the '\0').
 +	 */
 +}
 +
 +static int
 +miss(NODE *p, char *tail, size_t tail_len)
  {
  	int create;
  	char *tp;
  	const char *type, *what;
 +	int rval = 0;
  	int serr;
 +	size_t name_len;
  
  	for (; p; p = p->next) {
 +		/*
 +		 * if check() needs to be called (eflag set) then directly
 +		 * update nodes if they are not directories and only
 +		 * directories are being checked otherwise check().
 +		 */
 +#if 1
 +		if (tail >= path + MAXPATHLEN)
 +			(void)printf("!!!max path len exceeded!!!");
 +#endif
 +		if (eflag) {
 +			if (dflag && (p->type != F_DIR))
 +				p->flags |= F_VISIT;
 +			else
 +				rval |= check(p, tail, tail_len);
 +		}
 +		/*
 +		 * if check() needs to be called and fails with buffer overflow
 +		 * it is assumed the node cannot be visited as it also cannot
 +		 * exist (due to MAXPATHLEN being exceeded).
 +		 */
  		if (p->flags & F_OPT && !(p->flags & F_VISIT))
  			continue;
  		if (p->type != F_DIR && (dflag || p->flags & F_VISIT))
  			continue;
 -		(void)strcpy(tail, p->name);
 +		if ((name_len = strlcpy(tail, p->name, tail_len)) >= tail_len) {
 +			(void)printf("%s%s (skipped, buffer overflow)\n", path, p->name);
 +			rval |= BUFFEROVERFLOWEXIT;
 +			continue;
 +		}
  		if (!(p->flags & F_VISIT)) {
  			/* Don't print missing message if file exists as a
  			   symbolic link and the -q flag is set. */
 @@ -228,10 +299,15 @@
  		if (!(p->flags & F_VISIT))
  			(void)putchar('\n');
  
 -		for (tp = tail; *tp; ++tp);
 -		*tp = '/';
 -		miss(p->child, tp + 1);
 -		*tp = '\0';
 +		if (tail_len - name_len > 1) {
 +			tp = tail + name_len;
 +			*tp = '/';
 +			rval |= miss(p->child, tp + 1, tail_len - name_len - 1);
 +			*tp = '\0';
 +		} else if (p->child) {
 +			(void)printf("%s (children skipped, buffer overflow)\n", path);
 +			rval |= BUFFEROVERFLOWEXIT;
 +		}
  
  		if (!create)
  			continue;
 @@ -256,4 +332,5 @@
  			(void)printf("%s: file flags not set: %s\n",
  			    path, strerror(errno));
  	}
 +	return (rval);
  }
 
 --Boundary-00=_8QgfMw2WK9C4H4q--