From 84ded2d0c4214f63265f9ce192665b3c7d1b3660 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Wed, 24 Nov 2021 18:45:21 -0800 Subject: [PATCH] Backport the ksh93v- rm builtin to fix 'rm -d' (#348) The -d flag implemented in the rm builtin is completely broken. No matter what you do it refuses to remove directories, even if -r is also passed. Reproducer: $ mkdir /tmp/empty $ PATH=/opt/ast/bin rm -d /tmp/empty rm: /tmp/empty: directory $ PATH=/opt/ast/bin rm -dr /tmp/empty rm: /tmp/empty: directory not removed [Is a directory] Additionally, the description of 'rm -d' in the man page contradicts how it's specified in . The ksh93v- rm builtin fixed nearly all of these issues, so I've backported it to 93u+m and applied one additional fix for 'rm -rd'. src/lib/libcmd/rm.c: - Backported the fixes from the ksh93v- rm builtin's -d flag when used on empty directories. - Backported the man page update for rm(1) from ksh93v-. - The ksh93v- rm builtin had one additional bug that caused the -r option to fail when combined with -d. This was fixed by overriding -d if -r is also passed. src/cmd/ksh93/tests/builtins.sh: - Add regression tests for the rm builtin's -d option. --- NEWS | 5 ++++ src/cmd/ksh93/tests/builtins.sh | 41 +++++++++++++++++++++++++++++++++ src/lib/libcmd/rm.c | 31 ++++++++++++++----------- 3 files changed, 63 insertions(+), 14 deletions(-) diff --git a/NEWS b/NEWS index 4d0a889a8..1be10d9c8 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,11 @@ Any uppercase BUG_* names are modernish shell bug IDs. looking up functions defined with the 'function' keyword. In the POSIX standard and on other shells, the '.' command finds only script files. +- The rm built-in's -d/--directory option has been fixed. It now properly + removes empty directories and refuses to remove non-empty directories + (as specified in https://www.austingroupbugs.net/view.php?id=802). Note + that the rm built-in command isn't compiled in by default. + 2021-11-23: - A bug was fixed that allowed arithmetic expressions to assign out-of-range diff --git a/src/cmd/ksh93/tests/builtins.sh b/src/cmd/ksh93/tests/builtins.sh index 03c6fe555..32bcbc7d0 100755 --- a/src/cmd/ksh93/tests/builtins.sh +++ b/src/cmd/ksh93/tests/builtins.sh @@ -1311,5 +1311,46 @@ printf -v 'got[1][two][3]' 'ok\f%012d\n' $ver 2>/dev/null "(expected $(printf %q "$exp"), got $(printf %q "$got"))" unset got ver +# ====== +# The rm builtin's -d option should remove files and empty directories without +# removing non-empty directories (unless the -r option is also passed). +# https://www.austingroupbugs.net/view.php?id=802 +if builtin rm 2> /dev/null; then + echo foo > "$tmp/bar" + mkdir "$tmp/emptydir" + mkdir -p "$tmp/nonemptydir1/subfolder" + mkdir "$tmp/nonemptydir2" + echo dummyfile > "$tmp/nonemptydir2/shouldexist" + + # Tests for lone -d option + got=$(rm -d "$tmp/emptydir" 2>&1) + [[ $? == 0 ]] || err_exit 'rm builtin fails to remove empty directory with -d option' \ + "(got $(printf %q "$got"))" + [[ -d $tmp/emptydir ]] && err_exit 'rm builtin fails to remove empty directory with -d option' + got=$(rm -d $tmp/bar 2>&1) + [[ $? == 0 ]] || err_exit 'rm builtin fails to remove files with -d option' \ + "(got $(printf %q "$got"))" + [[ -f $tmp/bar ]] && err_exit 'rm builtin fails to remove files with -d option' + rm -d "$tmp/nonemptydir1" 2> /dev/null + [[ ! -d $tmp/nonemptydir1/subfolder ]] && err_exit 'rm builtin has unwanted recursion with -d option on folder containing folder' + rm -d "$tmp/nonemptydir2" 2> /dev/null + [[ ! -f $tmp/nonemptydir2/shouldexist ]] && err_exit 'rm builtin has unwanted recursion with -d option on folder containing file' + + # Recreate non-empty directories in case the above tests failed + mkdir -p "$tmp/nonemptydir1/subfolder" + mkdir -p "$tmp/nonemptydir2" + echo dummyfile > "$tmp/nonemptydir2/shouldexist" + + # Tests combining -d with -r + got=$(rm -rd "$tmp/nonemptydir1" 2>&1) + [[ $? == 0 ]] || err_exit 'rm builtin fails to remove non-empty directory and subdirectory with -rd options' \ + "(got $(printf %q "$got"))" + [[ -d $tmp/nonemptydir1/subfolder || -d $tmp/nonemptydir1 ]] && err_exit 'rm builtin fails to remove all folders with -rd options' + got=$(rm -rd "$tmp/nonemptydir2" 2>&1) + [[ $? == 0 ]] || err_exit 'rm builtin fails to remove non-empty directory and file with -rd options' \ + "(got $(printf %q "$got"))" + [[ -f $tmp/nonemptydir2/shouldexist || -d $tmp/nonemptydir2 ]] && err_exit 'rm builtin fails to remove all folders and files with -rd options' +fi + # ====== exit $((Errors<125?Errors:125)) diff --git a/src/lib/libcmd/rm.c b/src/lib/libcmd/rm.c index da3ba632d..44d810550 100644 --- a/src/lib/libcmd/rm.c +++ b/src/lib/libcmd/rm.c @@ -1,7 +1,7 @@ /*********************************************************************** * * * This software is part of the ast package * -* Copyright (c) 1992-2012 AT&T Intellectual Property * +* Copyright (c) 1992-2013 AT&T Intellectual Property * * Copyright (c) 2020-2021 Contributors to ksh 93u+m * * and is licensed under the * * Eclipse Public License, Version 1.0 * @@ -15,8 +15,8 @@ * AT&T Research * * Florham Park NJ * * * -* Glenn Fowler * -* David Korn * +* Glenn Fowler * +* David Korn * * * ***********************************************************************/ #pragma prototyped @@ -28,7 +28,7 @@ */ static const char usage[] = -"[-?\n@(#)$Id: rm (AT&T Research) 2012-02-14 $\n]" +"[-?\n@(#)$Id: rm (AT&T Research) 2013-12-01 $\n]" "[--catalog?" ERROR_CATALOG "]" "[+NAME?rm - remove files]" "[+DESCRIPTION?\brm\b removes the named \afile\a arguments. By default it" @@ -43,11 +43,9 @@ static const char usage[] = " writing a 0 filled buffer the same size as the file, executing" " \bfsync\b(2) and closing before attempting to remove. Implemented" " only on systems that support \bfsync\b(2).]" -"[d:directory?\bremove\b(3) (or \bunlink\b(2)) directories rather than" -" \brmdir\b(2), and don't require that they be empty before removal." -" The caller requires sufficient privilege, not to mention a strong" -" constitution, to use this option. Even though the directory must" -" not be empty, \brm\b still attempts to empty it before removal.]" +"[d:directory?If the current entry is a directory then remove it using " + "\brmdir\b(3) instead of the default \bunlink\b(2). If \b--recursive\b " + "is not specified then non-empty directories will not be removed.]" "[f:force?Ignore nonexistent files, ignore no file operands specified," " and never prompt the user.]" "[i:interactive|prompt?Prompt whether to remove each file." @@ -84,7 +82,7 @@ typedef struct State_s /* program state */ { Shbltin_t* context; /* builtin context */ int clobber; /* clear out file data first */ - int directory; /* remove(dir) not rmdir(dir) */ + int directory; /* rmdir(dir) not unlink(dir) */ int force; /* force actions */ int interactive; /* prompt for approval */ int recursive; /* remove subtrees too */ @@ -151,8 +149,11 @@ rm(State_t* state, register FTSENT* ent) if (!state->recursive) { fts_set(NiL, ent, FTS_SKIP); - error(2, "%s: directory", ent->fts_path); - break; + if (!state->directory) + { + error(2, "%s: directory", ent->fts_path); + break; + } } if (!beenhere(ent)) { @@ -188,7 +189,7 @@ rm(State_t* state, register FTSENT* ent) nonempty(ent); } } - if (ent->fts_info == FTS_D) + if (!state->directory && ent->fts_info == FTS_D) break; } else @@ -209,7 +210,7 @@ rm(State_t* state, register FTSENT* ent) path = ent->fts_accpath; if (state->verbose) sfputr(sfstdout, ent->fts_path, '\n'); - if ((ent->fts_info == FTS_DC || state->directory) ? remove(path) : rmdir(path)) + if ((state->recursive || state->directory) ? rmdir(path) : unlink(path)) switch (errno) { case ENOENT: @@ -385,6 +386,8 @@ b_rm(int argc, register char** argv, Shbltin_t* context) } if (!*argv) return 0; + if (state.directory && state.recursive) + state.directory = 0; /* the -r option overrides -d */ /* * do it