svn commit: r268376 - head/bin/rm

[Jilles Tjoelker]

On Thu, Jul 10, 2014 at 12:34:17AM +1000, Bruce Evans wrote:
> On Mon, 7 Jul 2014, Warner Losh wrote:

> > Log:
> >  rm -rf can fail sometimes with an error from fts_read. Make it honor
> >  fflag to ignore fts_read errors, but stop deleting from that directory
> >  because no further progress can be made.

> >  When building a kernel with a high -j value on a high core count
> >  machine, during the cleanobj phase we can wind up doing multiple rm
> >  -rf at the same time for modules that have subdirectories. This
> >  exposed this race (sometimes) as fts_read can return an error if the
> >  directory is removed by another rm -rf. Since the intent of the -f
> >  flag was to ignore errors, even if this was a bug in fts_read, we
> >  should ignore the error like we've been instructed to do.

> This seems to be more broken than before.  Although the intent of using
> -f is often to ignore errors, -f is supposed to only ignore ENOENT
> errors except in some cases for complicated interactions with -i.

Yes, it looks like POSIX requires ignoring non-existent files only
before recursing (for directory files). Hence, in particular, if a file
is deleted by another process between being read from the directory and
being checked for existence/type, -f shall cause the error to be
ignored, but if a directory is deleted by another process while files
are being deleted from it, the [ENOENT] error from rmdir() shall not be
ignored.

This probably does not make much sense and may be a defect in the
standard.

In this case, the error is probably because fts tried to change
directory to ".." which no longer exists in the deleted directory. This
causes fts to abort and rm may not do all required work. In the case
that two instances rm attempt to remove the same directory tree in
parallel, hiding the error message is indeed correct.

> > Modified: head/bin/rm/rm.c
> > ==============================================================================
> > --- head/bin/rm/rm.c	Mon Jul  7 23:21:15 2014	(r268375)
> > +++ head/bin/rm/rm.c	Mon Jul  7 23:21:20 2014	(r268376)
> > @@ -335,7 +335,7 @@ err:
> > 		warn("%s", p->fts_path);
> > 		eval = 1;
> > 	}
> > -	if (errno)
> > +	if (!fflag && errno)
> > 		err(1, "fts_read");
> > 	fts_close(fts);
> > }

> The old code was broken too.  Apart from its style bug (boolean test for
> non-boolean), it tests errno in all cases after leaving the loop, but
> most of the loop exits are not for errors so the test is of the garbage
> value last written to errno.

> So the old and new code both defeat the more careful tests of fflag combined
> with errno the loop.  Most are of the form (!fflag || p->fts_errno != ENOENT)
> (then at least a warning if this fails).  It also seems wrong to use
> errno outside the loop and p->fts_errno in the loop.  Of course, outside
> the loop, p is usually invalid, but that means that errno is very stale.

> % 	while ((p = fts_read(fts)) != NULL) {

> Normal loop exit is when this is NULL.  Then there is no error, and the
> errno that is tested is pure garbage.   Fixing this alone might fix the
> problem.  Otherwise, hopefully any error from things going away causes
> an ENOENT error, so checking for that alone would fix the error handling.
> That's a bit much to ask for -- if a whole subtree is removed then there
> should be multiple ENOENT errors and probably other errors, and fts_read()
> would find it hard to reduce to a single ENOENT without missing a more
> serious error.

Various fts(3) functions such as fts_read() violate the C standard rules
for library functions in that they may set errno to 0. If fts_read()
reaches the end, it sets errno to 0.

I changed find(1) to set errno to 0 before calling fts_read() but this
is apparently unnecessary.

> % 		switch (p->fts_info) {
> % 		case FTS_DNR:
> % 			if (!fflag || p->fts_errno != ENOENT) {
> % 				warnx("%s: %s",
> % 				    p->fts_path, strerror(p->fts_errno));
> % 				eval = 1;
> % 			}
> % 			continue;

> Normal error handling.  We check for ENOENT and handle the error completely
> and don't break out of the loop.

> % 		case FTS_ERR:
> % 			errx(1, "%s: %s", p->fts_path, strerror(p->fts_errno));

> Exit for fatal errors.  I think POSIX doesn't allow this, but requires
> contining to the next arg.  Exiting the loop should give that, unless
> fts_close() fails.

It looks like FTS_ERR does not require an immediate exit. find(1) does
not exit for FTS_ERR. It is a rare error though.

> % 		case FTS_NS:
> % 			/*
> % 			 * Assume that since fts_read() couldn't stat the
> % 			 * file, it can't be unlinked.
> % 			 */
> % 			if (!needstat)
> % 				break;

> Here we exit the loop for an assumed stat() error.  If this error is
> anything other than ENOENT, then POSIX doesn't allow ignoring it but
> it might allow quitting for the current arg.  If this error is
> ENOENT, then the error handling seems to be even more broken.  We
> shouldn't quit for the current arg on ENOENT, irrespective of fflag.

> Permissions 000 on a file don't prevent stat'ing it, and I couldn't
> find a simple example where this code is reached.

The break exits from the switch, not the loop, and therefore continues
with the next file.

Also note that there are two switches on p->fts_info here. The first
one prints an error message for FTS_NS (except for [ENOENT] if -f is
active, or if stat information is not needed). The latter part looks
wrong: the second switch also ignores everything if -f is active, so
even non-[ENOENT] errors are not written.

stat() can fail if the pathname is too long in FTS_NOCHDIR mode (not
after r260571, but that's only in 11-current) or if MAC denies it.

> % ...
> % 		case FTS_DP:
> % 			/* Post-order: see if user skipped. */
> % 			if (p->fts_number == SKIPPED)
> % 				continue;
> % 			break;

> Most other cases don't exit the loop.

> % ...
> % 	}
> % 	if (!fflag && errno)

> More broken than before.

> % 		err(1, "fts_read");

> fts without FTS_NOCHDIR is fundamentally broken if the directory tree
> can change underneath it, since (without more work by the caller) it
> is impossible to get back to the original directory in fts_close()
> or just up a level in the tree walk for small changes in the tree
> (the caller cannot handle this with any reasonable anount of work).
> FTS_NOCHDIR is only used by the following utilities in /usr/src/*bin:
> cp, pax, gxip, find, grep, asf.  It is not used in chmod, rm, ls,
> chflags, du, chown, ckdist, ctm, mtree, kldxref, setfmac.  Mixing
> this flag gives the worse possible combination -- e.g., rm can handle
> deeper trees by not using FTS_NOCHDIR, but cp cannot duplicate deep
> trees.  POSIX requires handling unboundedly deep trees in bounded
> memory for at last critical utilities like rm.  This is impossible,
> and not using FTS_NOCHDIR in FreeBSD rm doesn't give it.

Using FTS_NOCHDIR (either explicitly or implicitly by denying read
access to ".") allows following symlinks via race conditions -- very
dangerous when removing a directory tree as root that other users have
write access to. It also imposes the {PATH_MAX} limit.

When not using FTS_NOCHDIR, fts verifies everything in a paranoid manner
and refuses to "return" from a directory that was moved during the
traversal. At the end, it returns to the original directory ("." was
opened).

I don't think the requirement to "descend to arbitrary depths in the
file hierarchy" is intended to require the impossible. Rather, there
shall not be a fixed limit such as {PATH_MAX} or a hard-coded number of
file descriptors.

The requirement is probably on rm and not on cp because it is more
important for rm and is harder for cp (O_SEARCH is required, *at
functions are very desirable and neither used to exist).

-- 
Jilles Tjoelker