bug#60989: [PATCH] rm: fail on duplicate input if force not enabled
When the input files contain duplicates, then the rm fails. Because duplicates occur most often when the * is used and the shell unwraps it. There is a very common scenario when a user accidentally enters space after a filename, or enters space instead of forward slash. Example: rm prefix_ * The user intended to remove all files with a `prefix_` but removed all of the files in cwd. The program quits immediately when a duplicate is detected, to prevent pressing `y` because user expected a prompt regarding removing multiple files. The force option disables this function to enable scripts to work without modifying them. ``` diff --git a/src/rm.c b/src/rm.c index 354e2b0df..e4f9949f0 100644 --- a/src/rm.c +++ b/src/rm.c @@ -123,6 +123,16 @@ diagnose_leading_hyphen (int argc, char **argv) } } +static bool +find_duplicates (int n_files, char **files) +{ + for (int l = 0; l < n_files-1; l++) +for (int r = l+1; r < n_files; r++) + if (strcmp(files[l], files[r]) == 0) + return true; + return false; +} + void usage (int status) { @@ -211,6 +221,7 @@ main (int argc, char **argv) bool preserve_root = true; struct rm_options x; bool prompt_once = false; + bool force_rm = false; int c; initialize_main (&argc, &argv); @@ -238,6 +249,7 @@ main (int argc, char **argv) x.interactive = RMI_NEVER; x.ignore_missing_files = true; prompt_once = false; + force_rm = true; break; case 'i': @@ -352,6 +364,17 @@ main (int argc, char **argv) uintmax_t n_files = argc - optind; char **file = argv + optind; + if (!force_rm && find_duplicates(n_files, file)) +{ + /* Because usually when the input files are duplicated it means that the user + sumbitted both a directory and an * as separate arguments, probably by accident */ + fprintf (stderr, + "%s: input contains duplicates, most likely you've put " + "both * and a file from the same directory.\n", + program_name); + return EXIT_FAILURE; +} + if (prompt_once && (x.recursive || 3 < n_files)) { fprintf (stderr, ```
bug#60989: [PATCH] rm: fail on duplicate input if force not enabled
On 21/01/2023 13:05, Łukasz Sroka wrote: When the input files contain duplicates, then the rm fails. Because duplicates occur most often when the * is used and the shell unwraps it. There is a very common scenario when a user accidentally enters space after a filename, or enters space instead of forward slash. Example: rm prefix_ * The user intended to remove all files with a `prefix_` but removed all of the files in cwd. The program quits immediately when a duplicate is detected, to prevent pressing `y` because user expected a prompt regarding removing multiple files. The force option disables this function to enable scripts to work without modifying them. ``` diff --git a/src/rm.c b/src/rm.c index 354e2b0df..e4f9949f0 100644 --- a/src/rm.c +++ b/src/rm.c @@ -123,6 +123,16 @@ diagnose_leading_hyphen (int argc, char **argv) } } +static bool +find_duplicates (int n_files, char **files) +{ + for (int l = 0; l < n_files-1; l++) +for (int r = l+1; r < n_files; r++) + if (strcmp(files[l], files[r]) == 0) + return true; + return false; +} + void usage (int status) { @@ -211,6 +221,7 @@ main (int argc, char **argv) bool preserve_root = true; struct rm_options x; bool prompt_once = false; + bool force_rm = false; int c; initialize_main (&argc, &argv); @@ -238,6 +249,7 @@ main (int argc, char **argv) x.interactive = RMI_NEVER; x.ignore_missing_files = true; prompt_once = false; + force_rm = true; break; case 'i': @@ -352,6 +364,17 @@ main (int argc, char **argv) uintmax_t n_files = argc - optind; char **file = argv + optind; + if (!force_rm && find_duplicates(n_files, file)) +{ + /* Because usually when the input files are duplicated it means that the user + sumbitted both a directory and an * as separate arguments, probably by accident */ + fprintf (stderr, + "%s: input contains duplicates, most likely you've put " + "both * and a file from the same directory.\n", + program_name); + return EXIT_FAILURE; +} + if (prompt_once && (x.recursive || 3 < n_files)) { fprintf (stderr, ``` An interesting proposal. The main protection would be for `dir/ *` rather than `file_prefix_ *`. The former would be unusual for a user to type, while the latter more usual, but wouldn't trigger the protection AFAICS. This ads O(N^2) on each interaction, so if it was to be included probably only enabled with --interactive. cheers, Pádraig
bug#60989: [PATCH] rm: fail on duplicate input if force not enabled
On 21/01/2023 15:51, Łukasz Sroka wrote: On 21/01/2023 15:53, Pádraig Brady wrote: On 21/01/2023 13:05, Łukasz Sroka wrote: When the input files contain duplicates, then the rm fails. Because duplicates occur most often when the * is used and the shell unwraps it. There is a very common scenario when a user accidentally enters space after a filename, or enters space instead of forward slash. Example: rm prefix_ * The user intended to remove all files with a `prefix_` but removed all of the files in cwd. The program quits immediately when a duplicate is detected, to prevent pressing `y` because user expected a prompt regarding removing multiple files. The force option disables this function to enable scripts to work without modifying them. ``` diff --git a/src/rm.c b/src/rm.c index 354e2b0df..e4f9949f0 100644 --- a/src/rm.c +++ b/src/rm.c @@ -123,6 +123,16 @@ diagnose_leading_hyphen (int argc, char **argv) } } +static bool +find_duplicates (int n_files, char **files) +{ + for (int l = 0; l < n_files-1; l++) +for (int r = l+1; r < n_files; r++) + if (strcmp(files[l], files[r]) == 0) + return true; + return false; +} + void usage (int status) { @@ -211,6 +221,7 @@ main (int argc, char **argv) bool preserve_root = true; struct rm_options x; bool prompt_once = false; + bool force_rm = false; int c; initialize_main (&argc, &argv); @@ -238,6 +249,7 @@ main (int argc, char **argv) x.interactive = RMI_NEVER; x.ignore_missing_files = true; prompt_once = false; + force_rm = true; break; case 'i': @@ -352,6 +364,17 @@ main (int argc, char **argv) uintmax_t n_files = argc - optind; char **file = argv + optind; + if (!force_rm && find_duplicates(n_files, file)) +{ + /* Because usually when the input files are duplicated it means that the user + sumbitted both a directory and an * as separate arguments, probably by accident */ + fprintf (stderr, + "%s: input contains duplicates, most likely you've put " + "both * and a file from the same directory.\n", + program_name); + return EXIT_FAILURE; +} + if (prompt_once && (x.recursive || 3 < n_files)) { fprintf (stderr, ``` An interesting proposal. The main protection would be for `dir/ *` rather than `file_prefix_ *`. The former would be unusual for a user to type, while the latter more usual, but wouldn't trigger the protection AFAICS. This ads O(N^2) on each interaction, so if it was to be included probably only enabled with --interactive. cheers, Pádraig Yeah, true. Implemented it quickly and focused on the `dir/ *` scanerio at first, because that has happened to me -.- It was because I tried to do something quickly while editing command from backsearch and ended up with `rm tmp/ *` instead of `rm tmp/*`. To make the prefix_ protection it won't be possible without checking if a "prefix" exits on disk (eg. file and file1), which would be more time complex. I don't like the --interactive approach, as when you're trying to do something quickly, you probably won't opt for doing it interactively and you're probably either disabled or ignoring shell warnings. True, but at a high level -I can be seen as an unintrusive "be more careful" option, and so is often enabled by default with an alias. Red Hat flavored systems even enable the more intrusive -i option by default for the root user for example. The more I think about this problem, the more I tilt towards implementing it in shell. If you'd've written "rm prefix_ *" and had 10k files there, it would be so much easier to detect it on the shell side than checking every filename with the other and triggering syscalls for every one... Either way if you have other solutions in mind, please update me, as it wouldn't require to configure shell in dockers or remote machines, thus making the solution more available. A shell option like `shopt -s uniqueglob` would indeed be more general. There are various other globbing options, like the similar "failglob" which fails to execute the command if a glob doesn't match anything. cheers, Pádraig
bug#60989: [PATCH] rm: fail on duplicate input if force not enabled
On 21/01/2023 15:53, Pádraig Brady wrote: > > On 21/01/2023 13:05, Łukasz Sroka wrote: > > When the input files contain duplicates, then the rm fails. Because > > duplicates occur most often when the * is used and the shell unwraps > > it. > > There is a very common scenario when a user accidentally enters space > > after a filename, or enters space instead of forward slash. > > Example: > > > >rm prefix_ * > > > > The user intended to remove all files with a `prefix_` but removed all > > of the files in cwd. > > The program quits immediately when a duplicate is detected, to prevent > > pressing `y` because user expected a prompt regarding removing multiple > > files. > > The force option disables this function to enable scripts to work > > without modifying them. > > > > ``` > > diff --git a/src/rm.c b/src/rm.c > > index 354e2b0df..e4f9949f0 100644 > > --- a/src/rm.c > > +++ b/src/rm.c > > @@ -123,6 +123,16 @@ diagnose_leading_hyphen (int argc, char **argv) > > } > > } > > > > +static bool > > +find_duplicates (int n_files, char **files) > > +{ > > + for (int l = 0; l < n_files-1; l++) > > +for (int r = l+1; r < n_files; r++) > > + if (strcmp(files[l], files[r]) == 0) > > + return true; > > + return false; > > +} > > + > > void > > usage (int status) > > { > > @@ -211,6 +221,7 @@ main (int argc, char **argv) > > bool preserve_root = true; > > struct rm_options x; > > bool prompt_once = false; > > + bool force_rm = false; > > int c; > > > > initialize_main (&argc, &argv); > > @@ -238,6 +249,7 @@ main (int argc, char **argv) > > x.interactive = RMI_NEVER; > > x.ignore_missing_files = true; > > prompt_once = false; > > + force_rm = true; > > break; > > > > case 'i': > > @@ -352,6 +364,17 @@ main (int argc, char **argv) > > uintmax_t n_files = argc - optind; > > char **file = argv + optind; > > > > + if (!force_rm && find_duplicates(n_files, file)) > > +{ > > + /* Because usually when the input files are duplicated it means > > that the user > > + sumbitted both a directory and an * as separate arguments, > > probably by accident */ > > + fprintf (stderr, > > + "%s: input contains duplicates, most likely you've put " > > + "both * and a file from the same directory.\n", > > + program_name); > > + return EXIT_FAILURE; > > +} > > + > > if (prompt_once && (x.recursive || 3 < n_files)) > > { > > fprintf (stderr, > > ``` > > An interesting proposal. > The main protection would be for `dir/ *` rather than `file_prefix_ *`. > The former would be unusual for a user to type, while the latter more usual, > but wouldn't trigger the protection AFAICS. > This ads O(N^2) on each interaction, so if it was to be included probably > only enabled with --interactive. > > cheers, > Pádraig Yeah, true. Implemented it quickly and focused on the `dir/ *` scanerio at first, because that has happened to me -.- It was because I tried to do something quickly while editing command from backsearch and ended up with `rm tmp/ *` instead of `rm tmp/*`. To make the prefix_ protection it won't be possible without checking if a "prefix" exits on disk (eg. file and file1), which would be more time complex. I don't like the --interactive approach, as when you're trying to do something quickly, you probably won't opt for doing it interactively and you're probably either disabled or ignoring shell warnings. The more I think about this problem, the more I tilt towards implementing it in shell. If you'd've written "rm prefix_ *" and had 10k files there, it would be so much easier to detect it on the shell side than checking every filename with the other and triggering syscalls for every one... Either way if you have other solutions in mind, please update me, as it wouldn't require to configure shell in dockers or remote machines, thus making the solution more available.
bug#60989: [PATCH] rm: fail on duplicate input if force not enabled
On Sat, 21 Jan 2023, at 13:05, Łukasz Sroka wrote: > When the input files contain duplicates, then the rm fails. Because > duplicates occur most often when the * is used and the shell unwraps it. > There is a very common scenario when a user accidentally enters space > after a filename, or enters space instead of forward slash. To fail on duplicate FILE args, this bash function would do (lightly tested, doesn't attempt getopt processing): function safe_rm { local -A seen local file for file in "$@"; do if [[ -v ${seen[$file]} ]]; then echo "error: duplicate name '$file'" 1>&2 return 1 fi seen[$file]=1 done # no dupes seen command rm "$@" } and could be used today, without waiting for the next coreutils release. As an aside, I could be reading it wrong but the coreutils manual suggests the file arguments are optional rm [option]… [file]… Cheers, Phil
bug#60989: [PATCH] rm: fail on duplicate input if force not enabled
On 22/01/2023 18:18, Philip Rowlands wrote: On Sat, 21 Jan 2023, at 13:05, Łukasz Sroka wrote: When the input files contain duplicates, then the rm fails. Because duplicates occur most often when the * is used and the shell unwraps it. There is a very common scenario when a user accidentally enters space after a filename, or enters space instead of forward slash. To fail on duplicate FILE args, this bash function would do (lightly tested, doesn't attempt getopt processing): function safe_rm { local -A seen local file for file in "$@"; do if [[ -v ${seen[$file]} ]]; then echo "error: duplicate name '$file'" 1>&2 return 1 fi seen[$file]=1 done # no dupes seen command rm "$@" } and could be used today, without waiting for the next coreutils release. That's informative, thanks. As an aside, I could be reading it wrong but the coreutils manual suggests the file arguments are optional rm [option]… [file]… Right with the -f option rm will not fail if no arguments are specified (in the presence of nullglob etc.), which is POSIX compliant. cheers, Pádraig