Date: Mon, 30 Aug 2010 22:44:13 +0200 From: David Naylor <naylor.b.david@gmail.com> To: freebsd-hackers@freebsd.org Cc: Garrett Cooper <yanefbsd@gmail.com> Subject: Re: [RFR] mtree improvements (take 2) Message-ID: <201008302244.18208.naylor.b.david@gmail.com> In-Reply-To: <201006252212.46301.naylor.b.david@gmail.com> References: <201006251852.30302.naylor.b.david@gmail.com> <AANLkTilWRu63O48AotRODxnwo8EtnSHB27UucXzSJDf9@mail.gmail.com> <201006252212.46301.naylor.b.david@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--nextPart1409236.Jd7gOkigLW Content-Type: multipart/mixed; boundary="Boundary-01=_dgBfMjvoRdsPhiC" Content-Transfer-Encoding: 7bit --Boundary-01=_dgBfMjvoRdsPhiC Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline 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 <naylor.b.david@gmail.com> >=20 > wrote: > > > Hi, > > >=20 > > > 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]. > > >=20 > > > For a full discussion see PR bin/143732. This arose out of [2] where= I > > > experienced the increase. > > >=20 > > > For your convenience I have attached the patch. > > >=20 > > > Please review this patch and if it is acceptable, commit it. > > >=20 > > > Regards, > > >=20 > > > David > > >=20 > > > 1] http://markmail.org/message/iju3l6hyv7s7emrb > > > 2] http://markmail.org/message/gfztjpszl5dozzii >=20 > I've included some more changes. I do not think -[uU] needs to be set wh= en > skipping vwalk. I also move sflag so it will get called even if eflag is > set (bug in my patch). >=20 > I'm not sure sflag will produce the same results as check() and vwalk() m= ay > 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 m= tree=20 file that "triggers" the bug. Nothing interesting happens. =20 All comments are welcome. =20 David P.S. does anyone know of a test suite for mtree? --Boundary-01=_dgBfMjvoRdsPhiC Content-Type: text/plain; charset="ISO-8859-1"; name="big_path.mtree" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="big_path.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 .. .. .. .. .. .. .. .. .. --Boundary-01=_dgBfMjvoRdsPhiC Content-Type: text/x-patch; charset="ISO-8859-1"; name="mtree.diff" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="mtree.diff" =2D-- /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" =20 +/* + * Error returned when buffer overflows, cannot be 2 as that is reserved f= or + * MISMATCHEXIT. + */ +#define BUFFEROVERFLOWEXIT 3 + static NODE *root; static char path[MAXPATHLEN]; =20 =2Dstatic void miss(NODE *, char *); +static int miss(NODE *, char *, size_t); +static int check(NODE *, char *, size_t); static int vwalk(void); =20 int mtree_verifyspec(FILE *fi) { =2D int rval; + int rval =3D 0; =20 root =3D mtree_readspec(fi); =2D rval =3D vwalk(); =2D miss(root, path); + /* No need to walk tree if we are ignoring extra files. */ + if (!eflag) + rval =3D vwalk(); + *path =3D '\0'; + rval |=3D 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); } =20 @@ -135,40 +150,96 @@ if (ep) continue; extra: =2D if (!eflag) { =2D (void)printf("%s extra", RP(p)); =2D if (rflag) { =2D if ((S_ISDIR(p->fts_statp->st_mode) =2D ? rmdir : unlink)(p->fts_accpath)) { =2D (void)printf(", not removed: %s", =2D strerror(errno)); =2D } else =2D (void)printf(", removed"); =2D } =2D (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); =2D if (sflag) =2D warnx("%s checksum: %lu", fullpath, (unsigned long)crc_total); return (rval); } =20 =2Dstatic void =2Dmiss(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) >=3D tail_len) + return (BUFFEROVERFLOWEXIT); + + /* + * It is assumed that compare() only requires fts_accpath and fts_statp + * fields in the FTSENT structure. + */ + fts.fts_accpath =3D path; + fts.fts_statp =3D &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 |=3D F_VISIT; + if ((p->flags & F_NOCHANGE) =3D=3D 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 =3D 0; int serr; + size_t name_len; =20 for (; p; p =3D 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 >=3D path + MAXPATHLEN) + (void)printf("!!!max path len exceeded!!!"); +#endif + if (eflag) { + if (dflag && (p->type !=3D F_DIR)) + p->flags |=3D F_VISIT; + else + rval |=3D 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 !=3D F_DIR && (dflag || p->flags & F_VISIT)) continue; =2D (void)strcpy(tail, p->name); + if ((name_len =3D strlcpy(tail, p->name, tail_len)) >=3D tail_len) { + (void)printf("%s%s (skipped, buffer overflow)\n", path, p->name); + rval |=3D 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'); =20 =2D for (tp =3D tail; *tp; ++tp); =2D *tp =3D '/'; =2D miss(p->child, tp + 1); =2D *tp =3D '\0'; + if (tail_len - name_len > 1) { + tp =3D tail + name_len; + *tp =3D '/'; + rval |=3D miss(p->child, tp + 1, tail_len - name_len - 1); + *tp =3D '\0'; + } else if (p->child) { + (void)printf("%s (children skipped, buffer overflow)\n", path); + rval |=3D BUFFEROVERFLOWEXIT; + } =20 if (!create) continue; @@ -256,4 +332,5 @@ (void)printf("%s: file flags not set: %s\n", path, strerror(errno)); } + return (rval); } --Boundary-01=_dgBfMjvoRdsPhiC-- --nextPart1409236.Jd7gOkigLW Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.15 (FreeBSD) iEYEABECAAYFAkx8GCIACgkQUaaFgP9pFrJKeACdE8RcODZ018B4aM9UovipKDhj KLcAoIOJhC+m5bKkCH6vgb9F7Whh7a3M =MI0n -----END PGP SIGNATURE----- --nextPart1409236.Jd7gOkigLW--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201008302244.18208.naylor.b.david>