Hi! I think this bug looks pretty bad, as it deletes stuff unintentionally.
And appears to reduce size, so I'd think this would be nice! Denys, what do you think? Thanks! -nc On Tue, 19 Sep 2023 17:17:47 +0900 Dominique Martinet <[email protected]> wrote: > From: Dominique Martinet <[email protected]> > > find -xdev with -depth would check for same_fs after the subdirectory > has been processed (because the check is done in the file/dir action, > which is evaluated too late in the -depth case) > This renders `find -xdev -delete` useless, as reported in 2012 here: > https://bugs.busybox.net/show_bug.cgi?id=5756 > > The bug report suggested adding an extra hook, which would be required > if we were to keep the current xdev approach that allows all filesystems > given in argument, but GNU findutils and OpenBSD find actually stop on > the first filesystem boundary e.g. for the following tree: > > $ find test -exec stat --format "%d %n" {} + > 27 test > 27 test/file > 59 test/tmpfs > 27 test/tmpfs/bind > 27 test/tmpfs/bind/file > 59 test/tmpfs/file > (Where 'test/tmpfs' is a tmpfs, and 'test/tmpfs/bind' is a bind mount > to a neighboring directory in the same filesystem as 'test' -- also > tested with a symlink and -follow for openbsd which has no bind mount) > > Then `find test test/tmpfs -xdev` does not print test/tmpfs/bind/file. > > This makes the implementation much simpler (although it's a bit ugly to > carry the parent st_dev as an argument to the function) and smaller > code, and would allow for easy addition of rm/cp --one-file-system if > we want to do that later. > > function old new delta > recursive_action1 398 425 +27 > parse_params 1660 1670 +10 > recursive_action 70 72 +2 > fileAction 216 127 -89 > find_main 540 442 -98 > ------------------------------------------------------------------------------ > (add/remove: 0/0 grow/shrink: 3/2 up/down: 39/-187) Total: -148 bytes > text data bss dec hex filename > 75774 2510 1552 79836 137dc busybox_old > 75626 2510 1552 79688 13748 busybox_unstripped > --- > This has biten me recently and seems to be a known issue for a while. > > I don't recall seeing any patch for this since I've been on the list so > I took a quick stab, please take a look when you have time. > > file system loops aside, this can be tested on linux with the following > "script": > ++++++++++++++ > cd $(mktemp -d bb-find-xdev.XXXXXX) || exit > mkdir tmpfs > mount -t tmpfs tmpfs tmpfs > mkdir -p a/b/c > touch file tmpfs/file a/b/c/file > find $PWD -xdev -delete > # ^ errors about tmpfs busy and pwd not empty, > # but tmpfs/file is left intact and everyting else is gone > umount tmpfs > find $PWD -xdev -delete > +++++++++++++ > > findutils/find.c | 44 ++-------------------------------------- > include/libbb.h | 1 + > libbb/recursive_action.c | 13 +++++++++--- > 3 files changed, 13 insertions(+), 45 deletions(-) > > diff --git a/findutils/find.c b/findutils/find.c > index 31c9969886f6..a4a6bbc2df91 100644 > --- a/findutils/find.c > +++ b/findutils/find.c > @@ -501,7 +501,6 @@ struct globals { > #endif > action ***actions; > smallint need_print; > - smallint xdev_on; > smalluint exitstatus; > recurse_flags_t recurse_flags; > IF_FEATURE_FIND_EXEC_PLUS(unsigned max_argv_len;) > @@ -1015,26 +1014,10 @@ static int FAST_FUNC fileAction( > struct stat *statbuf) > { > int r; > - int same_fs = 1; > - > -#if ENABLE_FEATURE_FIND_XDEV > - if (S_ISDIR(statbuf->st_mode) && G.xdev_count) { > - int i; > - for (i = 0; i < G.xdev_count; i++) { > - if (G.xdev_dev[i] == statbuf->st_dev) > - goto found; > - } > - //bb_error_msg("'%s': not same fs", fileName); > - same_fs = 0; > - found: ; > - } > -#endif > > #if ENABLE_FEATURE_FIND_MAXDEPTH > if (state->depth < G.minmaxdepth[0]) { > - if (same_fs) > - return TRUE; /* skip this, continue recursing */ > - return SKIP; /* stop recursing */ > + return TRUE; /* skip this, continue recursing */ > } > if (state->depth > G.minmaxdepth[1]) > return SKIP; /* stop recursing */ > @@ -1051,11 +1034,6 @@ static int FAST_FUNC fileAction( > return SKIP; > } > #endif > - /* -xdev stops on mountpoints, but AFTER mountpoit itself > - * is processed as usual */ > - if (!same_fs) { > - return SKIP; > - } > > /* Cannot return 0: our caller, recursive_action(), > * will perror() and skip dirs (if called on dir) */ > @@ -1295,7 +1273,7 @@ static action*** parse_params(char **argv) > #if ENABLE_FEATURE_FIND_XDEV > else if (parm == OPT_XDEV) { > dbg("%d", __LINE__); > - G.xdev_on = 1; > + G.recurse_flags |= ACTION_XDEV; > } > #endif > #if ENABLE_FEATURE_FIND_MAXDEPTH > @@ -1718,24 +1696,6 @@ int find_main(int argc UNUSED_PARAM, char **argv) > G.actions = parse_params(&argv[firstopt]); > argv[firstopt] = NULL; > > -#if ENABLE_FEATURE_FIND_XDEV > - if (G.xdev_on) { > - struct stat stbuf; > - > - G.xdev_count = firstopt; > - G.xdev_dev = xzalloc(G.xdev_count * sizeof(G.xdev_dev[0])); > - for (i = 0; argv[i]; i++) { > - /* not xstat(): shouldn't bomb out on > - * "find not_exist exist -xdev" */ > - if (stat(argv[i], &stbuf) == 0) > - G.xdev_dev[i] = stbuf.st_dev; > - /* else G.xdev_dev[i] stays 0 and > - * won't match any real device dev_t > - */ > - } > - } > -#endif > - > for (i = 0; argv[i]; i++) { > if (!recursive_action(argv[i], > G.recurse_flags,/* flags */ > diff --git a/include/libbb.h b/include/libbb.h > index eb97a988045d..e8a7b449aef6 100644 > --- a/include/libbb.h > +++ b/include/libbb.h > @@ -514,6 +514,7 @@ enum { > ACTION_DEPTHFIRST = (1 << 3), > ACTION_QUIET = (1 << 4), > ACTION_DANGLING_OK = (1 << 5), > + ACTION_XDEV = (1 << 6), > }; > typedef uint8_t recurse_flags_t; > typedef struct recursive_state { > diff --git a/libbb/recursive_action.c b/libbb/recursive_action.c > index b1c4bfad7ccf..76dd664369a2 100644 > --- a/libbb/recursive_action.c > +++ b/libbb/recursive_action.c > @@ -62,9 +62,11 @@ static int FAST_FUNC true_action(struct recursive_state > *state UNUSED_PARAM, > * ACTION_FOLLOWLINKS mainly controls handling of links to dirs. > * 0: lstat(statbuf). Calls fileAction on link name even if points to dir. > * 1: stat(statbuf). Calls dirAction and optionally recurse on link to dir. > + * > + * If ACTION_XDEV, stop on different filesystem _after_ it has been processed > */ > > -static int recursive_action1(recursive_state_t *state, const char *fileName) > +static int recursive_action1(recursive_state_t *state, const char *fileName, > dev_t parentDev) > { > struct stat statbuf; > unsigned follow; > @@ -114,6 +116,10 @@ static int recursive_action1(recursive_state_t *state, > const char *fileName) > return TRUE; > } > > + /* skip cross devices -- we still need to process action */ > + if ((state->flags & ACTION_XDEV) && parentDev != 0 && statbuf.st_dev != > parentDev) > + goto skip_recurse; > + > dir = opendir(fileName); > if (!dir) { > /* findutils-4.1.20 reports this */ > @@ -132,7 +138,7 @@ static int recursive_action1(recursive_state_t *state, > const char *fileName) > > /* process every file (NB: ACTION_RECURSE is set in flags) */ > state->depth++; > - s = recursive_action1(state, nextFile); > + s = recursive_action1(state, nextFile, statbuf.st_dev); > if (s == FALSE) > status = FALSE; > free(nextFile); > @@ -146,6 +152,7 @@ static int recursive_action1(recursive_state_t *state, > const char *fileName) > } > closedir(dir); > > +skip_recurse: > if (state->flags & ACTION_DEPTHFIRST) { > if (!state->dirAction(state, fileName, &statbuf)) > goto done_nak_warn; > @@ -177,5 +184,5 @@ int FAST_FUNC recursive_action(const char *fileName, > state.fileAction = fileAction ? fileAction : true_action; > state.dirAction = dirAction ? dirAction : true_action; > > - return recursive_action1(&state, fileName); > + return recursive_action1(&state, fileName, 0); > } _______________________________________________ busybox mailing list [email protected] http://lists.busybox.net/mailman/listinfo/busybox
