GNU bug report logs - #39364
[PATCH] rmdir: fix clobbered errno

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 39364 in the body.
You can then email your comments to 39364 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: Matthew Pfeiffer <spferical <at> gmail.com>
To: bug-coreutils <at> gnu.org
Cc: Matthew Pfeiffer <spferical <at> gmail.com>
Subject: [PATCH] rmdir: fix clobbered errno
Date: Thu, 30 Jan 2020 20:46:40 -0500

'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)
-- 
2.25.0

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Matthew Pfeiffer <spferical <at> gmail.com>, 39364 <at> debbugs.gnu.org
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Fri, 31 Jan 2020 17:51:22 +0000

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

thanks,
Pádraig

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

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 39364 <at> debbugs.gnu.org, Matthew Pfeiffer <spferical <at> gmail.com>
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Sat, 1 Feb 2020 20:54:23 -0800

Nice find and thank you for the patch. That's a 12-year-old bug I introduced.
I confirm: with 6.10, running this:
  mkdir -p a/b && chmod a-w a && rmdir --ignore a/b
prints this and exits nonzero:
  rmdir: failed to remove `a/b': Permission denied

With 6.11 and newer, it silently succeeds.

On Fri, Jan 31, 2020 at 9:55 AM Pádraig Brady <P <at> draigbrady.com> 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
> 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.
>
> thanks,
> Pádraig
>
>
>

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

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 39364 <at> debbugs.gnu.org, Matthew Pfeiffer <spferical <at> gmail.com>
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Sat, 1 Feb 2020 22:32:42 -0800

[Message part 1 (text/plain, inline)]

On Fri, Jan 31, 2020 at 9:55 AM Pádraig Brady <P <at> draigbrady.com> wrote:
> ...
> This looks like a regression introduced in v6.10-21-ged5c4e7
> 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.

FTR, here's a minimal test addition that exercises the bug. Succeeds
on 6.10, fails on 6.11:

[rmdir-test-addition.diff (application/octet-stream, attachment)]

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Jim Meyering <jim <at> meyering.net>, Pádraig Brady
 <P <at> draigbrady.com>
Cc: 39364 <at> debbugs.gnu.org, Matthew Pfeiffer <spferical <at> gmail.com>
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Sun, 2 Feb 2020 14:11:18 +0100

On 2020-02-02 07:32, Jim Meyering wrote:
> FTR, here's a minimal test addition that exercises the bug. Succeeds
> on 6.10, fails on 6.11:

Minor tweak for the test ...

-rmdir --ignore-fail-on-non-empty x/y && fail=1
+returns_ 1 rmdir --ignore-fail-on-non-empty x/y || fail=1


... due to:

  tests/rmdir/ignore.sh:36:rmdir --ignore-fail-on-non-empty x/y && fail=1
  maint.mk: && fail=1 detected. Please use: returns_ 1 ... || fail=1
  make: *** [cfg.mk:516: sc_prohibit_and_fail_1] Error 1

Thanks & have a nice day,
Berny

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

From: Jim Meyering <jim <at> meyering.net>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: Pádraig Brady <P <at> draigbrady.com>, 39364 <at> debbugs.gnu.org,
 Matthew Pfeiffer <spferical <at> gmail.com>
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Sun, 2 Feb 2020 20:58:04 -0800

On Sun, Feb 2, 2020 at 5:11 AM Bernhard Voelker
<mail <at> bernhard-voelker.de> wrote:
> On 2020-02-02 07:32, Jim Meyering wrote:
> > FTR, here's a minimal test addition that exercises the bug. Succeeds
> > on 6.10, fails on 6.11:
>
> Minor tweak for the test ...
>
> -rmdir --ignore-fail-on-non-empty x/y && fail=1
> +returns_ 1 rmdir --ignore-fail-on-non-empty x/y || fail=1
>
>
> ... due to:
>
>   tests/rmdir/ignore.sh:36:rmdir --ignore-fail-on-non-empty x/y && fail=1
>   maint.mk: && fail=1 detected. Please use: returns_ 1 ... || fail=1
>   make: *** [cfg.mk:516: sc_prohibit_and_fail_1] Error 1

Thanks. Good point. Though note that I'm pretty sure that
prematurely-posted "test" is fundamentally inadequate. I haven't had
time to look more closely.

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Matthew Pfeiffer <spferical <at> gmail.com>, 39364 <at> debbugs.gnu.org
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Mon, 3 Feb 2020 13:26:27 +0000

[Message part 1 (text/plain, inline)]

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

[rmdir-ignore-perm.patch (text/x-patch, attachment)]

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

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 39364 <at> debbugs.gnu.org, Matthew Pfeiffer <spferical <at> gmail.com>
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Mon, 3 Feb 2020 08:35:43 -0800

On Mon, Feb 3, 2020 at 5:28 AM Pádraig Brady <P <at> draigbrady.com> wrote:
...
> 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))));

Nice! Thanks for tracking that down. The patch looks great.
You might want to mention (in the log) the commit that introduced the
bug, since you already did the work to track it down:
v6.10-21-ged5c4e7

I preferred to require that for each NEWS-worthy bug fix, because
otherwise, it can be costly to re-derive or dig up in mail archives.

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Matthew Pfeiffer <spferical <at> gmail.com>, 39364 <at> debbugs.gnu.org
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Mon, 3 Feb 2020 16:45:06 +0000

On 03/02/2020 13:26, Pádraig Brady wrote:
> On 31/01/2020 17:51, Pádraig Brady wrote:
> 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.

I'll also squash this in to the previous commit,
to ensure we diagnose the case where we can't
determine if the directory is empty.

diff --git a/src/rmdir.c b/src/rmdir.c
index a2f0f0813..7301db5ee 100644
--- a/src/rmdir.c
+++ b/src/rmdir.c
@@ -101,7 +101,8 @@ ignorable_failure (int error_number, char const *dir)
   return (ignore_fail_on_non_empty
           && (errno_rmdir_non_empty (error_number)
               || (errno_may_be_non_empty (error_number)
-                  && ! is_empty_dir (AT_FDCWD, dir))));
+                  && ! is_empty_dir (AT_FDCWD, dir)
+                  && errno == 0 /* definitely non empty  */)));
 }

 /* Remove any empty parent directories of DIR.
diff --git a/src/system.h b/src/system.h
index 9d777680c..ebf68349a 100644
--- a/src/system.h
+++ b/src/system.h
@@ -285,7 +285,9 @@ readdir_ignoring_dot_and_dotdot (DIR *dirp)
     }
 }

-/* Return true if DIR is determined to be an empty directory.  */
+/* Return true if DIR is determined to be an empty directory.
+   Return false with ERRNO==0 if DIR is a non empty directory.
+   Return false if not able to determine if directory empty.  */
 static inline bool
 is_empty_dir (int fd_cwd, char const *dir)
 {
@@ -310,6 +312,7 @@ is_empty_dir (int fd_cwd, char const *dir)
   dp = readdir_ignoring_dot_and_dotdot (dirp);
   saved_errno = errno;
   closedir (dirp);
+  errno = saved_errno;
   if (dp != NULL)
     return false;
   return saved_errno == 0 ? true : false;
diff --git a/tests/rmdir/ignore.sh b/tests/rmdir/ignore.sh
index 0d2be25ed..65e92d012 100755
--- a/tests/rmdir/ignore.sh
+++ b/tests/rmdir/ignore.sh
@@ -40,5 +40,10 @@ test -d x/y || fail=1
 touch x/y/z || framework_failure_
 rmdir --ignore-fail-on-non-empty x/y || fail=1
 test -d x/y || fail=1
+# assume empty dir if unreadable entries (so failure to remove diagnosed)
+rm x/y/z || framework_failure_
+chmod a-r x/y || framework_failure_
+returns_ 1 rmdir --ignore-fail-on-non-empty x/y || fail=1
+test -d x/y || fail=1

 Exit $fail

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Matthew Pfeiffer <spferical <at> gmail.com>, 39364-done <at> debbugs.gnu.org
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Tue, 4 Feb 2020 19:21:42 +0000

On 03/02/2020 16:45, Pádraig Brady wrote:
> On 03/02/2020 13:26, Pádraig Brady wrote:
>> On 31/01/2020 17:51, Pádraig Brady wrote:
>> 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.
> 
> I'll also squash this in to the previous commit,
> to ensure we diagnose the case where we can't
> determine if the directory is empty.

pushed. marking done

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: 39364 <at> debbugs.gnu.org, P <at> draigBrady.com, spferical <at> gmail.com
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Thu, 6 Feb 2020 19:04:58 +0100

On 2020-02-04 20:21, Pádraig Brady wrote:
> pushed. marking done

Hi Padraig,

I just noticed that 'is_empty_dir' from 'src/system.h' is also used
in 'src/remove.c', so - without having looked there yet - it could
be that the patch changed something in rm(1) as well (good or bad).
Did you check?

Thanks & have a nice day,
Berny

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>, 39364 <at> debbugs.gnu.org,
 spferical <at> gmail.com
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Fri, 7 Feb 2020 13:58:33 +0000

On 06/02/2020 18:04, Bernhard Voelker wrote:
> On 2020-02-04 20:21, Pádraig Brady wrote:
>> pushed. marking done
> 
> Hi Padraig,
> 
> I just noticed that 'is_empty_dir' from 'src/system.h' is also used
> in 'src/remove.c', so - without having looked there yet - it could
> be that the patch changed something in rm(1) as well (good or bad).
> Did you check?

Yes the existing is_empty_dir() interface didn't change.
We just added more info (errno) in the non empty case,
that is not inspected by remove.c (which is fine for its uses).

thanks for checking,
Pádraig

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 39364 <at> debbugs.gnu.org, 
 spferical <at> gmail.com
Subject: Re: bug#39364: [PATCH] rmdir: fix clobbered errno
Date: Sun, 9 Feb 2020 21:29:38 +0100

On 2020-02-07 14:58, Pádraig Brady wrote:
> Yes the existing is_empty_dir() interface didn't change.
> We just added more info (errno) in the non empty case,
> that is not inspected by remove.c (which is fine for its uses).

Thanks for checking.

Have a nice day,
Berny

Previous Next

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