Date: Sun, 29 Aug 2010 03:54:29 -0700 From: Garrett Cooper <gcooper@FreeBSD.org> To: Garrett Cooper <gcooper@freebsd.org> Cc: freebsd-bugs@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: bin/144411: [patch] mtree(8) doesn't reject non-regular files for -X Message-ID: <AANLkTi=C7nshKdKWdhTL7qK2dm=o3M83NyyfF6UHFVe9@mail.gmail.com> In-Reply-To: <364299f41003301740m4ca73398v9aadcc87e53a4628@mail.gmail.com> References: <201003300830.o2U8U93Y096013@freefall.freebsd.org> <20100331034500.O1425@besplex.bde.org> <20100331060503.G1425@besplex.bde.org> <364299f41003301740m4ca73398v9aadcc87e53a4628@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Tue, Mar 30, 2010 at 5:40 PM, Garrett Cooper <gcooper@freebsd.org> wrote:
> On Tue, Mar 30, 2010 at 12:12 PM, Bruce Evans <brde@optusnet.com.au> wrote:
>> On Wed, 31 Mar 2010, Bruce Evans wrote:
>>
>>> On Tue, 30 Mar 2010, Garrett Cooper wrote:
>>>
>>>> Hi,
>>>> I'm not 100% satisfied with this patch now. Looking back it fails
>>>> the following case:
>>>>
>>>> -P Do not follow symbolic links in the file hierarchy, instead
>>>> con-
>>>> sider the symbolic link itself in any comparisons. This is the
>>>> default.
>>>
>>> -P should have the same semantics and description in all utilities. The
>>> description should not have grammar errors like the above (comma splice).
>>> ...
>>> I now see that the grammar error is from the original version of mtree(1),
>>> and is probably one of the things you don't like. mtree also has -L, but
>>> not -R or -P or -h. It is not clear how any utility that traverses trees
>>> can work without a full complement of -[HLPR] or how any utility that
>>> ...
>>
>> Looking at the actual patch, I now see that it is about a completely
>> different problem. You would only need to understand the amount of
>> brokenness of -P to see if you need to use lstat(). I think -P is so
>> broken that mtree on symlinks doesn't work at all and not using lstat()
>> would be safest.
>
> Hmmm... so I take it that this is actually the first step in many to
> fixing this underlying problem? I suppose I should be opening bugs for
> all of the itemized issues that you see in mtree(8) so someone can
> submit patches to fix the utility?
>
>> The patch has some style bugs.
>
> Please expound on this -- I want to improve my style (without having
> to rewrite the entire program of course) -- so that it conforms more
> to the projects overall style rules; of course there are some cases
> where I can't readily do that (like pkg_install -- ugh), but I'll do
> my best to make sure that the rules are withheld.
Just for the record, here's the latest patch that I submitted to
Bruce for this PR.
Thanks,
-Garrett
[-- Attachment #2 --]
Index: parse.c
===================================================================
--- parse.c (revision 205137)
+++ parse.c (working copy)
@@ -94,58 +94,112 @@
nextfs = &tfs->nextfs;
nextfu = &tfs->nextfu;
- /* take the format string and break it up into format units */
- for (p = fmt;;) {
- /* skip leading white space */
- for (; isspace(*p); ++p);
- if (!*p)
- break;
+ /*
+ * Take the format string and break it up into format units.
+ *
+ * The structure of each format unit is as follows:
+ *
+ * iteration_count/byte_count
+ *
+ * iteration_count and byte_count are optional, but either one or the
+ * other must be present.
+ *
+ * Iteration count defaults to 1, and byte count defaults vary
+ * depending upon the format string specified. See size for more
+ * details.
+ */
+ for (p = fmt; *p; ) {
- /* allocate a new format unit and link it in */
- if ((tfu = calloc(1, sizeof(FU))) == NULL)
- err(1, NULL);
- *nextfu = tfu;
- nextfu = &tfu->nextfu;
- tfu->reps = 1;
+ /* Skip any and all leading white space. */
+ for (; isspace(*p); p++) ;
- /* if leading digit, repetition count */
- if (isdigit(*p)) {
- for (savep = p; isdigit(*p); ++p);
- if (!isspace(*p) && *p != '/')
+ /* There's more information on the line to scan. */
+ if (*p) {
+
+ /* allocate a new format unit and link it in */
+ if ((tfu = calloc(1, sizeof(FU))) == NULL)
+ err(1, "calloc");
+ *nextfu = tfu;
+ nextfu = &tfu->nextfu;
+ /* Default to one iteration count. */
+ tfu->reps = 1;
+
+ /*
+ * Scan the leading digit -- it's the repetition count.
+ */
+ if (isdigit(*p)) {
+
+ for (savep = p; isdigit(*p); ++p);
+ /*
+ * Next character scanned wasn't '/' -- the
+ * iteration count is invalid.
+ */
+ if (!isspace(*p) && *p != '/')
+ badfmt(fmt);
+ /* May overwrite either white space or slash */
+ tfu->reps = (int) strtol(savep, NULL, 10);
+ /*
+ * We only want non-zero numbers. All negative
+ * numbers would be caught below at the == '"'
+ * check because the leading character is `-'.
+ */
+ if (tfu->reps == 0)
+ badfmt(fmt);
+ tfu->flags = F_SETREP;
+ /* skip trailing white space */
+ for (++p; isspace(*p); ++p) ;
+
+ }
+
+ /* Skip the slash and trailing white space */
+ if (*p == '/')
+ while (isspace(*++p));
+
+ /* Scan the trailing digit -- it's the byte count. */
+ if (isdigit(*p)) {
+
+ for (savep = p; isdigit(*p); ++p);
+ if (!isspace(*p))
+ badfmt(fmt);
+ tfu->bcnt = (int) strtol(savep, NULL, 10);
+ /*
+ * We only want non-zero numbers. All negative
+ * numbers would be caught below at the == '"'
+ * check because the leading character is `-'.
+ */
+ if (tfu->bcnt == 0)
+ badfmt(fmt);
+ /* skip trailing white space */
+ for (++p; isspace(*p); ++p) ;
+
+ }
+
+ /* This wasn't a valid format string */
+ if (*p++ != '"')
badfmt(fmt);
- /* may overwrite either white space or slash */
- tfu->reps = atoi(savep);
- tfu->flags = F_SETREP;
- /* skip trailing white space */
- for (++p; isspace(*p); ++p);
- }
+ /*
+ * Copy the format string between the '"'
+ * terminators.
+ */
+ for (savep = p; *p != '"'; p++) {
- /* skip slash and trailing white space */
- if (*p == '/')
- while (isspace(*++p));
+ /* The format string isn't valid (couldn't find
+ * a '"' terminator) */
+ if (*p == '\0')
+ badfmt(fmt);
- /* byte count */
- if (isdigit(*p)) {
- for (savep = p; isdigit(*p); ++p);
- if (!isspace(*p))
- badfmt(fmt);
- tfu->bcnt = atoi(savep);
- /* skip trailing white space */
- for (++p; isspace(*p); ++p);
+ }
+ if (!(tfu->fmt = malloc(p - savep + 1)))
+ err(1, "malloc");
+ (void) strlcpy(tfu->fmt, savep, p - savep + 1);
+ /* End format string copy. */
+ escape(tfu->fmt);
+ p++;
+
}
- /* format */
- if (*p != '"')
- badfmt(fmt);
- for (savep = ++p; *p != '"';)
- if (*p++ == 0)
- badfmt(fmt);
- if (!(tfu->fmt = malloc(p - savep + 1)))
- err(1, NULL);
- (void) strlcpy(tfu->fmt, savep, p - savep + 1);
- escape(tfu->fmt);
- p++;
}
+
}
static const char *spec = ".#-+ 0123456789";
@@ -160,47 +214,68 @@
/* figure out the data block size needed for each format unit */
for (cursize = 0, fu = fs->nextfu; fu; fu = fu->nextfu) {
- if (fu->bcnt) {
- cursize += fu->bcnt * fu->reps;
- continue;
- }
- for (bcnt = prec = 0, fmt = fu->fmt; *fmt; ++fmt) {
- if (*fmt != '%')
- continue;
- /*
- * skip any special chars -- save precision in
- * case it's a %s format.
- */
- while (index(spec + 1, *++fmt));
- if (*fmt == '.' && isdigit(*++fmt)) {
- prec = atoi(fmt);
- while (isdigit(*++fmt));
- }
- switch(*fmt) {
- case 'c':
- bcnt += 1;
- break;
- case 'd': case 'i': case 'o': case 'u':
- case 'x': case 'X':
- bcnt += 4;
- break;
- case 'e': case 'E': case 'f': case 'g': case 'G':
- bcnt += 8;
- break;
- case 's':
- bcnt += prec;
- break;
- case '_':
- switch(*++fmt) {
- case 'c': case 'p': case 'u':
- bcnt += 1;
- break;
+
+ bcnt = fu->bcnt;
+
+ if (bcnt == 0) {
+
+ for (prec = 0, fmt = fu->fmt; *fmt; ++fmt) {
+
+ if (*fmt == '%') {
+
+ /*
+ * Skip any special chars minus `.'.
+ * Save precision in case it's a %s
+ * format.
+ */
+ while (*fmt != '\0' &&
+ index(spec+1, *++fmt) != NULL) ;
+
+ if (*fmt == '.' && isdigit(*++fmt)) {
+ prec = strtol(fmt, NULL, 10);
+ if (prec == 0) {
+ errx(1, "Bad precision "
+ "value: %s",
+ fmt);
+ }
+ while (isdigit(*++fmt)) ;
+ }
+ switch(*fmt) {
+ case 'c':
+ bcnt += 1;
+ break;
+ case 'd': case 'i': case 'o': case 'u':
+ case 'x': case 'X':
+ bcnt += 4;
+ break;
+ case 'e': case 'E': case 'f': case 'g':
+ case 'G':
+ bcnt += 8;
+ break;
+ case 's':
+ bcnt += prec;
+ break;
+ case '_':
+ switch(*++fmt) {
+ case 'c': case 'p': case 'u':
+ bcnt += 1;
+ break;
+ }
+
+ }
+
}
+
}
+
}
+
cursize += bcnt * fu->reps;
+
}
+
return (cursize);
+
}
void
@@ -247,10 +322,10 @@
if (fu->bcnt) {
sokay = USEBCNT;
/* Skip to conversion character. */
- for (++p1; index(spec, *p1); ++p1);
+ for (++p1; *p1 != '\0' && index(spec, *p1); ++p1);
} else {
/* Skip any special chars, field width. */
- while (index(spec + 1, *++p1));
+ while (*p1 != '\0' && index(spec + 1, *++p1));
if (*p1 == '.' && isdigit(*++p1)) {
sokay = USEPREC;
prec = atoi(p1);
@@ -396,7 +471,7 @@
p1[0] = '\0';
len = strlen(fmtp) + strlen(cs) + 1;
if ((pr->fmt = calloc(1, len)) == NULL)
- err(1, NULL);
+ err(1, "calloc");
snprintf(pr->fmt, len, "%s%s", fmtp, cs);
*p2 = savech;
pr->cchar = pr->fmt + (p1 - fmtp);
@@ -425,16 +500,20 @@
*/
for (fu = fs->nextfu; fu; fu = fu->nextfu) {
if (!fu->nextfu && fs->bcnt < blocksize &&
- !(fu->flags&F_SETREP) && fu->bcnt)
+ !(fu->flags & F_SETREP) && fu->bcnt)
fu->reps += (blocksize - fs->bcnt) / fu->bcnt;
if (fu->reps > 1) {
- for (pr = fu->nextpr;; pr = pr->nextpr)
- if (!pr->nextpr)
- break;
- for (p1 = pr->fmt, p2 = NULL; *p1; ++p1)
- p2 = isspace(*p1) ? p1 : NULL;
- if (p2)
- pr->nospace = p2;
+
+ for (pr = fu->nextpr; pr && pr->nextpr; pr = pr->nextpr) ;
+
+ /* Avoid a NULL pointer. */
+ if (pr != NULL) {
+ for (p1 = pr->fmt, p2 = NULL; *p1; ++p1)
+ p2 = isspace(*p1) ? p1 : NULL;
+ if (p2)
+ pr->nospace = p2;
+ }
+
}
}
#ifdef DEBUG
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=C7nshKdKWdhTL7qK2dm=o3M83NyyfF6UHFVe9>