On Friday 25 June 2010 22:12:42 David Naylor wrote: > On Friday 25 June 2010 20:01:42 Garrett Cooper wrote: > > On Fri, Jun 25, 2010 at 9:52 AM, David Naylor <[email protected]> > > wrote: > > > Hi, > > > > > > I've created a patch that increases the performance of mtree. This is > > > of particular use during a port install. In an extreme case I have > > > experienced a ~20% increase [1]. > > > > > > For a full discussion see PR bin/143732. This arose out of [2] where I > > > experienced the increase. > > > > > > For your convenience I have attached the patch. > > > > > > Please review this patch and if it is acceptable, commit it. > > > > > > Regards, > > > > > > David > > > > > > 1] http://markmail.org/message/iju3l6hyv7s7emrb > > > 2] http://markmail.org/message/gfztjpszl5dozzii > > I've included some more changes. I do not think -[uU] needs to be set when > skipping vwalk. I also move sflag so it will get called even if eflag is > set (bug in my patch). > > I'm not sure sflag will produce the same results as check() and vwalk() may > call compare() in different orders? Will this impact on crc?
I've also fixed a buffer overflow bug in mtree, there is an example of an mtree file that "triggers" the bug. Nothing interesting happens. All comments are welcome. David P.S. does anyone know of a test suite for mtree?
/set uid=1001 gid=1001 mode=0755
. type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
this_is_a_very_long_directory_name_that_is_exactly_128_characts_long_requiring_8_to_exceed_the_maximum_path_length_of_1024_byte
type=dir
and_a_file_to_tip_the_scale
..
..
..
..
..
..
..
..
..
--- /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);
}
signature.asc
Description: This is a digitally signed message part.
