Date: Wed, 1 Sep 2010 07:50:03 GMT From: David Naylor <naylor.b.david@gmail.com> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/143732: [patch] mtree(8) does a full hierarchy walk when requested to just update structure (-u -e) Message-ID: <201009010750.o817o344082508@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/143732; it has been noted by GNATS.
From: David Naylor <naylor.b.david@gmail.com>
To: bug-followup@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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201009010750.o817o344082508>