GNU bug report logs -
#44889
[PATCH] rm: do not skip extra files when removal of empty directories fails
Previous Next
Full log
View this message in rfc822 format
[Message part 1 (text/plain, inline)]
Your message dated Thu, 26 Nov 2020 19:39:54 +0000
with message-id <5b72061f-dc81-118f-bfa1-95d968e3900d <at> draigBrady.com>
and subject line Re: bug#44889: [PATCH] rm: do not skip extra files when removal of empty directories fails
has caused the debbugs.gnu.org bug report #44889,
regarding [PATCH] rm: do not skip extra files when removal of empty directories fails
to be marked as done.
(If you believe you have received this mail in error, please contact
help-debbugs <at> gnu.org.)
--
44889: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=44889
GNU Bug Tracking System
Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
From: Nishant Nayan <nishant.nayan <at> oracle.com>
When removing a directory fails for some reason, and that directory
is empty, the rm_fts code gets the return value of the excise call
confused with the return value of its earlier call to prompt,
causing fts_skip_tree to be called repeatedly and the next file
that rm would otherwise have deleted to survive.
[nca: added commit log, test]
* src/remove.c (rm_fts): Only ever carry out one branch of the
rm -i failure path for directories, never both (a user cannot both
answer y and n to an rm -i prompt).
* tests/rm/empty-immutable-skip.sh: New root-only test.
* tests/local.mk: Add it.
* NEWS: Mention the bug fix.
---
NEWS | 3 +++
src/remove.c | 3 +--
tests/local.mk | 1 +
tests/rm/empty-immutable-skip.sh | 46 ++++++++++++++++++++++++++++++++
4 files changed, 51 insertions(+), 2 deletions(-)
create mode 100755 tests/rm/empty-immutable-skip.sh
diff --git a/NEWS b/NEWS
index 61b5d42f6..ce40fc4ff 100644
--- a/NEWS
+++ b/NEWS
@@ -24,6 +24,9 @@ GNU coreutils NEWS -*- outline -*-
invalid combinations of case character classes.
[bug introduced in coreutils-8.6]
+ rm no longer skips an extra file when the removal of an empty directory fails.
+ [introduced by the rewrite to make rm use fts in coreutils-8.0]
+
** Changes in behavior
cp and install now default to copy-on-write (COW) if available.
diff --git a/src/remove.c b/src/remove.c
index 2d40c55cd..1150cf179 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -508,8 +508,7 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
s = excise (fts, ent, x, true);
fts_skip_tree (fts, ent);
}
-
- if (s != RM_OK)
+ else if (s != RM_OK)
{
mark_ancestor_dirs (ent);
fts_skip_tree (fts, ent);
diff --git a/tests/local.mk b/tests/local.mk
index e1c4675c2..297760cc5 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -136,6 +136,7 @@ all_root_tests = \
tests/rm/no-give-up.sh \
tests/rm/one-file-system.sh \
tests/rm/read-only.sh \
+ tests/rm/empty-immutable-skip.sh \
tests/tail-2/append-only.sh \
tests/tail-2/end-of-device.sh \
tests/touch/now-owned-by-other.sh
diff --git a/tests/rm/empty-immutable-skip.sh b/tests/rm/empty-immutable-skip.sh
new file mode 100755
index 000000000..e31417073
--- /dev/null
+++ b/tests/rm/empty-immutable-skip.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+# Ensure that rm does not skip extra files after hitting an empty immutable dir.
+# Requires root access to do chattr +i, as well as an ext[23] or xfs file system
+
+# Copyright (C) 2006-2020 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ tail
+require_root_
+
+# These simple one-file operations are expected to work even in the
+# presence of this bug, and we need them to set up the rest of the test.
+chattr_i_works=1
+touch f
+chattr +i f 2>/dev/null || chattr_i_works=0
+rm f 2>/dev/null
+test -f f || chattr_i_works=0
+chattr -i f 2>/dev/null || chattr_i_works=0
+rm f 2>/dev/null || chattr_i_works=0
+test -f f && chattr_i_works=0
+
+if test $chattr_i_works = 0; then
+ skip_ "chattr +i doesn't work on this file system"
+fi
+
+mkdir empty
+touch x y
+chattr +i empty
+rm -rf empty x y
+{ test -f x || test -f y || test -f z; } && fail=1
+chattr -i empty
+
+Exit $fail
--
2.29.0.249.g249b51256f
[Message part 3 (message/rfc822, inline)]
On 26/11/2020 18:49, Nick Alcock wrote:
> On 26 Nov 2020, Pádraig Brady verbalised:
>
>> On 26/11/2020 14:35, Nick Alcock wrote:
>>> diff --git a/src/remove.c b/src/remove.c
>>> index 2d40c55cd..1150cf179 100644
>>> --- a/src/remove.c
>>> +++ b/src/remove.c
>>> @@ -508,8 +508,7 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
>>> s = excise (fts, ent, x, true);
>>> fts_skip_tree (fts, ent);
>>> }
>>> -
>>> - if (s != RM_OK)
>>> + else if (s != RM_OK)
>>> {
>>> mark_ancestor_dirs (ent);
>>> fts_skip_tree (fts, ent);
>>
>> I think we'd still like to mark ancestors when failing to remove,
>> so that we don't prompt unnecessarily.
>
> If this doesn't make the test fail, I'm fine with it.
Cool, the test still passes :)
>> I think it would be better to do:
>>
>> diff --git a/src/remove.c b/src/remove.c
>> index 2d40c55cd..adf948935 100644
>> --- a/src/remove.c
>> +++ b/src/remove.c
>> @@ -506,7 +506,8 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
>> /* When we know (from prompt when in interactive mode)
>> that this is an empty directory, don't prompt twice. */
>> s = excise (fts, ent, x, true);
>> - fts_skip_tree (fts, ent);
>> + if (s == RM_OK)
>> + fts_skip_tree (fts, ent);
>> }
>
> I don't really understand what mark_ancestor_dirs is doing, but as long
> as it's not making the file disappear and the new test still passes I'm
> fine with this (though honestly the s= stuff is incredibly confusing and
> really should be using two distinct variables for result-of-prompt and
> result-of-excision to make it obvious what the flaming dingbats is going
> on).
>
> Another worry of mine is that I don't understand why fts_skip_tree is
> skipping an entry *other* than ent the second time it's called. Naively
> I'd have assumed fts_skip_tree (fts, ent) would be idempotent: calling
> it repeatedly with the same ent should do the same as calling it only
> once. But clearly this is not the case, so I'm misreading the code in
> gnulib and/or glibc somehow.
There is an fts_read() in fts_skip_tree() that's consuming the entry.
Pushed at:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=6bf108358
cheers,
Pádraig
This bug report was last modified 4 years and 176 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.