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

2020-02-09 Thread Bernhard Voelker 
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

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

2020-02-07 Thread Pádraig Brady 

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

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

2020-02-06 Thread Bernhard Voelker 
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

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

2020-02-04 Thread Pádraig Brady 

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

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

2020-02-03 Thread Pádraig Brady 

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

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

2020-02-03 Thread Jim Meyering 
On Mon, Feb 3, 2020 at 5:28 AM 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;

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.

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

2020-02-03 Thread Pádraig Brady 

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?= 
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  sw...@alum.mit.edu
 Matthew Arnison maf...@cat.org.au
 Matthew Braun   matt...@ans.net
 Matthew Clarke  matthew_cla...@mindlink.bc.ca
+Matthew Pfeifferspferi...@gmail.com
 Matthew M. Boedickermatth...@boedicker.org
 Matthew S. Levine

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

2020-02-02 Thread Jim Meyering 
On Sun, Feb 2, 2020 at 5:11 AM Bernhard Voelker
 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.

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

2020-02-01 Thread Jim Meyering 
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  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
>
>
>

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

2020-01-31 Thread Pádraig Brady 

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

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

2020-01-30 Thread Matthew Pfeiffer 
'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