Subject: bug#39364: [PATCH] rmdir: fix clobbered errno

From: Pádraig Brady <[email-masked]>
Date: Mon, 3 Feb 2020 13:26:27 +0000

On 31/01/2020 17:51, Pádraig Brady wrote:
> On 31/01/2020 01:46, Matthew Pfeiffer wrote:
>> 'rmdir --ignore-fail-on-non-empty' would not report errors on non-empty
>> directories that fail for a different reason.
>> ---
>>    src/rmdir.c | 10 ++++++----
>>    1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/rmdir.c b/src/rmdir.c
>> index c9f417957..7b253ab0d 100644
>> --- a/src/rmdir.c
>> +++ b/src/rmdir.c
>> @@ -133,18 +133,19 @@ remove_parents (char *dir)
>>            prog_fprintf (stdout, _("removing directory, %s"), quoteaf (dir));
>>    
>>          ok = (rmdir (dir) == 0);
>> +      int rmdir_errno = errno;
>>    
>>          if (!ok)
>>            {
>>              /* Stop quietly if --ignore-fail-on-non-empty. */
>> -          if (ignorable_failure (errno, dir))
>> +          if (ignorable_failure (rmdir_errno, dir))
>>                {
>>                  ok = true;
>>                }
>>              else
>>                {
>>                  /* Barring race conditions, DIR is expected to be a directory.  */
>> -              error (0, errno, _("failed to remove directory %s"),
>> +              error (0, rmdir_errno, _("failed to remove directory %s"),
>>                         quoteaf (dir));
>>                }
>>              break;
>> @@ -233,12 +234,13 @@ main (int argc, char **argv)
>>    
>>          if (rmdir (dir) != 0)
>>            {
>> -          if (ignorable_failure (errno, dir))
>> +          int rmdir_errno = errno;
>> +          if (ignorable_failure (rmdir_errno, dir))
>>                continue;
>>    
>>              /* Here, the diagnostic is less precise, since we have no idea
>>                 whether DIR is a directory.  */
>> -          error (0, errno, _("failed to remove %s"), quoteaf (dir));
>> +          error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
>>              ok = false;
>>            }
>>          else if (remove_empty_parents)
>>
> 
> This looks like a regression introduced in v6.10-21-ged5c4e7

For reference, this was originally discussed at:
https://lists.gnu.org/archive/html/bug-coreutils/2008-01/msg00283.html

> I.E. is_empty_dir() is globbering errno, and thus
> a non specific and non terminating warning is output,
> rather than a specific error, and exit.

Actually I think the key issue is not errno handling,
but a logic error fixed with:

@@ -102,7 +102,7 @@ ignorable_failure (int error_number, char const *dir)
    return (ignore_fail_on_non_empty
            && (errno_rmdir_non_empty (error_number)
                || (errno_may_be_empty (error_number)
-                  && is_empty_dir (AT_FDCWD, dir))));
+                  && ! is_empty_dir (AT_FDCWD, dir))));

Attached is a full patch to address these issues.
cheers,
Pádraig
>From 3b379d77ec342b66a36d4f39435dccc121ca86eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Thu, 30 Jan 2020 20:46:40 -0500
Subject: [PATCH] rmdir: fix --ignore-fail-on-non-empty with permissions errors

Since 6.11 `rmdir --ignore-fail-on-non-empty` had reversed
the failure status for directories that failed to be removed
for permissions reasons.  I.E. it would have returned a
failure status for such non empty dirs, and vice versa.

* src/rmdir.c (errno_may_be_non_empty): Rename from the
more confusing errno_may_be_empty(), and remove the EEXIST
case (specific to Solaris), which is moot here since
handled in errno_rmdir_non_empty().
(ignorable_failure): Fix the logic error so that
_non_ empty dirs are deemed to have ignorable failures.
(main): Fix clobbering of errno by is_empty_dir().
(remove_parents): Likewise.
* tests/rmdir/ignore.sh: Add a test case.
* THANKS.in: Add reporter who fixed the errno handling.
* NEWS: Mention the bug fix.
Fixes https://bugs.gnu.org/39364
---
 NEWS                  |  5 +++++
 THANKS.in             |  1 +
 src/rmdir.c           | 21 +++++++++++----------
 tests/rmdir/ignore.sh | 12 ++++++++++++
 4 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/NEWS b/NEWS
index 5231b84ac..8a349634e 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,11 @@ GNU coreutils NEWS                                    -*- outline -*-
   (like Solaris 10 and Solaris 11).
   [bug introduced in coreutils-8.31]
 
+  rmdir --ignore-fail-on-non-empty now works correctly for directories
+  that fail to be removed due to permission issues.  Previously the exit status
+  was reversed, failing for non empty and succeeding for empty directories.
+  [bug introduced in coreutils-6.11]
+
   'shuf -r -n 0 file' no longer mistakenly reads from standard input.
   [bug introduced with the --repeat feature in coreutils-8.22]
 
diff --git a/THANKS.in b/THANKS.in
index 23b089754..a667197ed 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -420,6 +420,7 @@ Matt Swift                          [email-masked]
 Matthew Arnison                     [email-masked]
 Matthew Braun                       [email-masked]
 Matthew Clarke                      [email-masked]
+Matthew Pfeiffer                    [email-masked]
 Matthew M. Boedicker                [email-masked]
 Matthew S. Levine                   [email-masked]
 Matthew Smith                       [email-masked]
diff --git a/src/rmdir.c b/src/rmdir.c
index c9f417957..a2f0f0813 100644
--- a/src/rmdir.c
+++ b/src/rmdir.c
@@ -77,16 +77,15 @@ errno_rmdir_non_empty (int error_number)
 }
 
 /* Return true if when rmdir fails with errno == ERROR_NUMBER
-   the directory may be empty.  */
+   the directory may be non empty.  */
 static bool
-errno_may_be_empty (int error_number)
+errno_may_be_non_empty (int error_number)
 {
   switch (error_number)
     {
     case EACCES:
     case EPERM:
     case EROFS:
-    case EEXIST:
     case EBUSY:
       return true;
     default:
@@ -101,8 +100,8 @@ ignorable_failure (int error_number, char const *dir)
 {
   return (ignore_fail_on_non_empty
           && (errno_rmdir_non_empty (error_number)
-              || (errno_may_be_empty (error_number)
-                  && is_empty_dir (AT_FDCWD, dir))));
+              || (errno_may_be_non_empty (error_number)
+                  && ! is_empty_dir (AT_FDCWD, dir))));
 }
 
 /* Remove any empty parent directories of DIR.
@@ -133,18 +132,19 @@ remove_parents (char *dir)
         prog_fprintf (stdout, _("removing directory, %s"), quoteaf (dir));
 
       ok = (rmdir (dir) == 0);
+      int rmdir_errno = errno;
 
-      if (!ok)
+      if (! ok)
         {
           /* Stop quietly if --ignore-fail-on-non-empty. */
-          if (ignorable_failure (errno, dir))
+          if (ignorable_failure (rmdir_errno, dir))
             {
               ok = true;
             }
           else
             {
               /* Barring race conditions, DIR is expected to be a directory.  */
-              error (0, errno, _("failed to remove directory %s"),
+              error (0, rmdir_errno, _("failed to remove directory %s"),
                      quoteaf (dir));
             }
           break;
@@ -233,12 +233,13 @@ main (int argc, char **argv)
 
       if (rmdir (dir) != 0)
         {
-          if (ignorable_failure (errno, dir))
+          int rmdir_errno = errno;
+          if (ignorable_failure (rmdir_errno, dir))
             continue;
 
           /* Here, the diagnostic is less precise, since we have no idea
              whether DIR is a directory.  */
-          error (0, errno, _("failed to remove %s"), quoteaf (dir));
+          error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
           ok = false;
         }
       else if (remove_empty_parents)
diff --git a/tests/rmdir/ignore.sh b/tests/rmdir/ignore.sh
index d000c30cb..0d2be25ed 100755
--- a/tests/rmdir/ignore.sh
+++ b/tests/rmdir/ignore.sh
@@ -29,4 +29,16 @@ test -d "$cwd/a/x" || fail=1
 test -d "$cwd/a/b" && fail=1
 test -d "$cwd/a/b/c" && fail=1
 
+# Ensure that with --ignore-fail-on-non-empty, we still fail, e.g., for EPERM.
+# Between 6.11 and 8.31, the following rmdir would mistakenly succeed.
+mkdir -p x/y || framework_failure_
+chmod a-w x || framework_failure_
+returns_ 1 rmdir --ignore-fail-on-non-empty x/y || fail=1
+test -d x/y || fail=1
+# Between 6.11 and 8.31, the following rmdir would mistakenly fail,
+# and also give a non descript error
+touch x/y/z || framework_failure_
+rmdir --ignore-fail-on-non-empty x/y || fail=1
+test -d x/y || fail=1
+
 Exit $fail
-- 
2.24.1