GNU bug report logs - #44889
[PATCH] rm: do not skip extra files when removal of empty directories fails

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 44889 in the body.
You can then email your comments to 44889 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nick Alcock <nick.alcock <at> oracle.com>
To: bug-coreutils <at> gnu.org
Cc: nishant.nayan <at> oracle.com, bert.barbe <at> oracle.com, elena.zannoni <at> oracle.com,
 jemarch <at> gnu.org
Subject: [PATCH] rm: do not skip extra files when removal of empty directories
 fails
Date: Thu, 26 Nov 2020 14:35:17 +0000

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 #8 received at 44889 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Nick Alcock <nick.alcock <at> oracle.com>, 44889 <at> debbugs.gnu.org
Cc: nishant.nayan <at> oracle.com, bert.barbe <at> oracle.com, elena.zannoni <at> oracle.com,
 jemarch <at> gnu.org
Subject: Re: bug#44889: [PATCH] rm: do not skip extra files when removal of
 empty directories fails
Date: Thu, 26 Nov 2020 17:01:12 +0000

forcemerge 44889 44865 44883
stop

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.
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've also adjusted the test slightly to not check for `tail`,
and another few cleanups.

I'll push in your name if you're ok with the above changes.

thanks,
Pádraig

Message #17 received at 44889 <at> debbugs.gnu.org (full text, mbox):

From: Nick Alcock <nick.alcock <at> oracle.com>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: elena.zannoni <at> oracle.com, nishant.nayan <at> oracle.com, bert.barbe <at> oracle.com,
 44889 <at> debbugs.gnu.org, jemarch <at> gnu.org
Subject: Re: bug#44889: [PATCH] rm: do not skip extra files when removal of
 empty directories fails
Date: Thu, 26 Nov 2020 18:49:43 +0000

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.

> 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.

> I'll push in your name if you're ok with the above changes.

It's really in Nishan's name, but OK :)

Message #22 received at 44889-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Nick Alcock <nick.alcock <at> oracle.com>
Cc: elena.zannoni <at> oracle.com, nishant.nayan <at> oracle.com, bert.barbe <at> oracle.com,
 44889-done <at> debbugs.gnu.org, jemarch <at> gnu.org
Subject: Re: bug#44889: [PATCH] rm: do not skip extra files when removal of
 empty directories fails
Date: Thu, 26 Nov 2020 19:39:54 +0000

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.