From owner-freebsd-bugs@FreeBSD.ORG Wed Sep 1 07:50:03 2010 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 77E8010656A8 for ; Wed, 1 Sep 2010 07:50:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 4B1CD8FC13 for ; Wed, 1 Sep 2010 07:50:03 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id o817o3mq082516 for ; Wed, 1 Sep 2010 07:50:03 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id o817o344082508; Wed, 1 Sep 2010 07:50:03 GMT (envelope-from gnats) Date: Wed, 1 Sep 2010 07:50:03 GMT Message-Id: <201009010750.o817o344082508@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: David Naylor Cc: Subject: Re: bin/143732: [patch] mtree(8) does a full hierarchy walk when requested to just update structure (-u -e) X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: David Naylor List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Sep 2010 07:50:03 -0000 The following reply was made to PR bin/143732; it has been noted by GNATS. From: David Naylor 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--