[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: [PATCH] find: fix -xdev -depth (and -delete)
From: Natanael Copa <ncopa () alpinelinux ! org>
Date: 2023-10-06 15:07:13
Message-ID: 20231006170713.6231bde7 () ncopa-desktop ! lan
[Download RAW message or body]
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 <asmadeus@codewreck.org> wrote:
> From: Dominique Martinet <dominique.martinet@atmark-techno.com>
>
> 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
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic