GNU bug report logs - #58050
[INSTALLED] rm: fix diagnostics on I/O error

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 58050 in the body.
You can then email your comments to 58050 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: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Cc: Paul Eggert <eggert <at> cs.ucla.edu>
Subject: [INSTALLED] rm: fix diagnostics on I/O error
Date: Sat, 24 Sep 2022 16:37:49 -0700

I ran into this problem when attempting to recursively
remove a directory in a filesystem on flaky hardware.
Although the underlying readdir syscall failed with errno == EIO,
rm issued no diagnostic about the I/O error.

Without this patch I see this behavior:

  $ rm -fr baddir
  rm: cannot remove 'baddir': Directory not empty
  $ rm -ir baddir
  rm: descend into directory 'baddir'? y
  rm: remove directory 'baddir'? y
  rm: cannot remove 'baddir': Directory not empty

With this patch I see the following behavior, which
lets the user know about the I/O error when rm tries
to read baddir's directory entries:

  $ rm -fr baddir
  rm: cannot remove 'baddir': Input/output error
  $ rm -ir baddir
  rm: cannot remove 'baddir': Input/output error

* src/remove.c (Ternary): Remove.  All uses removed.
(get_dir_status): New static function.
(prompt): Last arg is now directory status, not ternary.
Return RM_USER_ACCEPTED if user explicitly accepted.
All uses changed.
Report any significant error in directory status right away.
(prompt, rm_fts): Use get_dir_status to get directory status lazily.
(excise): Treat any FTS_DNR errno as being more descriptive, not
just EPERM and EACCESS.  For example, EIO is more descriptive.
(rm_fts): Distinguish more clearly between explicit and implied
user OK.
* src/remove.h (RM_USER_ACCEPTED): New constant.
(VALID_STATUS): Treat it as valid.
* src/system.h (is_empty_dir): Remove, replacing with ...
(directory_status): ... this more-general function.
All uses changed.  Avoid undefined behavior of looking at
a non-null readdir pointer after corresponding closedir.
* tests/rm/rm-readdir-fail.sh: Adjust test of internals
to match current behavior.
---
 src/remove.c                | 80 +++++++++++++++++++------------------
 src/remove.h                |  4 +-
 src/rmdir.c                 |  3 +-
 src/system.h                | 25 ++++++------
 tests/rm/rm-readdir-fail.sh |  1 +
 5 files changed, 58 insertions(+), 55 deletions(-)

diff --git a/src/remove.c b/src/remove.c
index 6756c409d..0b6754bf7 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -33,14 +33,6 @@
 #include "xfts.h"
 #include "yesno.h"
 
-enum Ternary
-  {
-    T_UNKNOWN = 2,
-    T_NO,
-    T_YES
-  };
-typedef enum Ternary Ternary;
-
 /* The prompt function may be called twice for a given directory.
    The first time, we ask whether to descend into it, and the
    second time, we ask whether to remove it.  */
@@ -168,9 +160,23 @@ write_protected_non_symlink (int fd_cwd,
   }
 }
 
-/* Prompt whether to remove FILENAME (ent->, if required via a combination of
+/* Return the status of the directory identified by FTS and ENT.
+   This is -1 if the directory is empty, 0 if it is nonempty,
+   and a positive error number if there was trouble determining the status,
+   e.g., it is not a directory, or permissions problems, or I/O errors.
+   Use *DIR_STATUS is a cache for the status.  */
+static int
+get_dir_status (FTS const *fts, FTSENT const *ent, int *dir_status)
+{
+  if (*dir_status < -1)
+    *dir_status = directory_status (fts->fts_cwd_fd, ent->fts_accpath);
+  return *dir_status;
+}
+
+/* Prompt whether to remove FILENAME, if required via a combination of
    the options specified by X and/or file attributes.  If the file may
-   be removed, return RM_OK.  If the user declines to remove the file,
+   be removed, return RM_OK or RM_USER_ACCEPTED, the latter if the user
+   was prompted and accepted.  If the user declines to remove the file,
    return RM_USER_DECLINED.  If not ignoring missing files and we
    cannot lstat FILENAME, then return RM_ERROR.
 
@@ -178,20 +184,16 @@ write_protected_non_symlink (int fd_cwd,
 
    Depending on MODE, ask whether to 'descend into' or to 'remove' the
    directory FILENAME.  MODE is ignored when FILENAME is not a directory.
-   Set *IS_EMPTY_P to T_YES if FILENAME is an empty directory, and it is
-   appropriate to try to remove it with rmdir (e.g. recursive mode).
-   Don't even try to set *IS_EMPTY_P when MODE == PA_REMOVE_DIR.  */
+   Use and update *DIR_STATUS as needed, via the conventions of
+   get_dir_status.  */
 static enum RM_status
 prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
         struct rm_options const *x, enum Prompt_action mode,
-        Ternary *is_empty_p)
+        int *dir_status)
 {
   int fd_cwd = fts->fts_cwd_fd;
   char const *full_name = ent->fts_path;
   char const *filename = ent->fts_accpath;
-  if (is_empty_p)
-    *is_empty_p = T_UNKNOWN;
-
   struct stat st;
   struct stat *sbuf = &st;
   cache_stat_init (sbuf);
@@ -199,13 +201,6 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
   int dirent_type = is_dir ? DT_DIR : DT_UNKNOWN;
   int write_protected = 0;
 
-  bool is_empty = false;
-  if (is_empty_p)
-    {
-      is_empty = is_empty_dir (fd_cwd, filename);
-      *is_empty_p = is_empty ? T_YES : T_NO;
-    }
-
   /* When nonzero, this indicates that we failed to remove a child entry,
      either because the user declined an interactive prompt, or due to
      some other failure, like permissions.  */
@@ -257,10 +252,12 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
              /* Unless we're either deleting directories or deleting
                 recursively, we want to raise an EISDIR error rather than
                 prompting the user  */
-            if ( ! (x->recursive || (x->remove_empty_directories && is_empty)))
+            if ( ! (x->recursive
+                    || (x->remove_empty_directories
+                        && get_dir_status (fts, ent, dir_status) < 0)))
               {
                 write_protected = -1;
-                wp_errno = EISDIR;
+                wp_errno = *dir_status <= 0 ? EISDIR : *dir_status;
               }
             break;
           }
@@ -276,12 +273,17 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
       /* Issue the prompt.  */
       if (dirent_type == DT_DIR
           && mode == PA_DESCEND_INTO_DIR
-          && !is_empty)
+          && get_dir_status (fts, ent, dir_status) == 0)
         fprintf (stderr,
                  (write_protected
                   ? _("%s: descend into write-protected directory %s? ")
                   : _("%s: descend into directory %s? ")),
                  program_name, quoted_name);
+      else if (0 < *dir_status)
+        {
+          error (0, *dir_status, _("cannot remove %s"), quoted_name);
+          return RM_ERROR;
+        }
       else
         {
           if (cache_fstatat (fd_cwd, filename, sbuf, AT_SYMLINK_NOFOLLOW) != 0)
@@ -302,8 +304,7 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
                    program_name, file_type (sbuf), quoted_name);
         }
 
-      if (!yesno ())
-        return RM_USER_DECLINED;
+      return yesno () ? RM_USER_ACCEPTED : RM_USER_DECLINED;
     }
   return RM_OK;
 }
@@ -405,13 +406,12 @@ excise (FTS *fts, FTSENT *ent, struct rm_options const *x, bool is_dir)
 
   /* When failing to rmdir an unreadable directory, we see errno values
      like EISDIR or ENOTDIR (or, on Solaris 10, EEXIST), but they would be
-     meaningless in a diagnostic.  When that happens and the errno value
-     from the failed open is EPERM or EACCES, use the earlier, more
+     meaningless in a diagnostic.  When that happens, use the earlier, more
      descriptive errno value.  */
   if (ent->fts_info == FTS_DNR
       && (errno == ENOTEMPTY || errno == EISDIR || errno == ENOTDIR
           || errno == EEXIST)
-      && (ent->fts_errno == EPERM || ent->fts_errno == EACCES))
+      && ent->fts_errno != 0)
     errno = ent->fts_errno;
   error (0, errno, _("cannot remove %s"), quoteaf (ent->fts_path));
   mark_ancestor_dirs (ent);
@@ -427,12 +427,14 @@ excise (FTS *fts, FTSENT *ent, struct rm_options const *x, bool is_dir)
 static enum RM_status
 rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
 {
+  int dir_status = -2;
+
   switch (ent->fts_info)
     {
     case FTS_D:			/* preorder directory */
       if (! x->recursive
           && !(x->remove_empty_directories
-               && is_empty_dir (fts->fts_cwd_fd, ent->fts_accpath)))
+               && get_dir_status (fts, ent, &dir_status) < 0))
         {
           /* This is the first (pre-order) encounter with a directory
              that we cannot delete.
@@ -507,11 +509,10 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
         }
 
       {
-        Ternary is_empty_directory;
         enum RM_status s = prompt (fts, ent, true /*is_dir*/, x,
-                                   PA_DESCEND_INTO_DIR, &is_empty_directory);
+                                   PA_DESCEND_INTO_DIR, &dir_status);
 
-        if (s == RM_OK && is_empty_directory == T_YES)
+        if (s == RM_USER_ACCEPTED && dir_status == -1)
           {
             /* When we know (from prompt when in interactive mode)
                that this is an empty directory, don't prompt twice.  */
@@ -520,7 +521,7 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
               fts_skip_tree (fts, ent);
           }
 
-        if (s != RM_OK)
+        if (! (s == RM_OK || s == RM_USER_ACCEPTED))
           {
             mark_ancestor_dirs (ent);
             fts_skip_tree (fts, ent);
@@ -553,8 +554,9 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
           }
 
         bool is_dir = ent->fts_info == FTS_DP || ent->fts_info == FTS_DNR;
-        enum RM_status s = prompt (fts, ent, is_dir, x, PA_REMOVE_DIR, NULL);
-        if (s != RM_OK)
+        enum RM_status s = prompt (fts, ent, is_dir, x, PA_REMOVE_DIR,
+                                   &dir_status);
+        if (! (s == RM_OK || s == RM_USER_ACCEPTED))
           return s;
         return excise (fts, ent, x, is_dir);
       }
diff --git a/src/remove.h b/src/remove.h
index e926084e2..b92c63daa 100644
--- a/src/remove.h
+++ b/src/remove.h
@@ -79,13 +79,15 @@ enum RM_status
 {
   /* These must be listed in order of increasing seriousness. */
   RM_OK = 2,
+  RM_USER_ACCEPTED,
   RM_USER_DECLINED,
   RM_ERROR,
   RM_NONEMPTY_DIR
 };
 
 # define VALID_STATUS(S) \
-  ((S) == RM_OK || (S) == RM_USER_DECLINED || (S) == RM_ERROR)
+  ((S) == RM_OK || (S) == RM_USER_ACCEPTED || (S) == RM_USER_DECLINED \
+   || (S) == RM_ERROR)
 
 # define UPDATE_STATUS(S, New_value)				\
   do								\
diff --git a/src/rmdir.c b/src/rmdir.c
index f6284cbe2..283f332ec 100644
--- a/src/rmdir.c
+++ b/src/rmdir.c
@@ -101,8 +101,7 @@ 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)
-                  && errno == 0 /* definitely non empty  */)));
+                  && directory_status (AT_FDCWD, dir) == 0)));
 }
 
 /* Remove any empty parent directories of DIR.
diff --git a/src/system.h b/src/system.h
index d36ca1115..91228ec13 100644
--- a/src/system.h
+++ b/src/system.h
@@ -287,37 +287,36 @@ readdir_ignoring_dot_and_dotdot (DIR *dirp)
     }
 }
 
-/* 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)
+/* Return -1 if DIR is an empty directory,
+   0 if DIR is a nonempty directory,
+   and a positive error number if there was trouble determining
+   whether DIR is an empty or nonempty directory.  */
+static inline int
+directory_status (int fd_cwd, char const *dir)
 {
   DIR *dirp;
-  struct dirent const *dp;
+  bool no_direntries;
   int saved_errno;
   int fd = openat (fd_cwd, dir,
                    (O_RDONLY | O_DIRECTORY
                     | O_NOCTTY | O_NOFOLLOW | O_NONBLOCK));
 
   if (fd < 0)
-    return false;
+    return errno;
 
   dirp = fdopendir (fd);
   if (dirp == NULL)
     {
+      saved_errno = errno;
       close (fd);
-      return false;
+      return saved_errno;
     }
 
   errno = 0;
-  dp = readdir_ignoring_dot_and_dotdot (dirp);
+  no_direntries = !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;
+  return no_direntries && saved_errno == 0 ? -1 : saved_errno;
 }
 
 /* Factor out some of the common --help and --version processing code.  */
diff --git a/tests/rm/rm-readdir-fail.sh b/tests/rm/rm-readdir-fail.sh
index a77d5225f..9dbf9380c 100755
--- a/tests/rm/rm-readdir-fail.sh
+++ b/tests/rm/rm-readdir-fail.sh
@@ -112,6 +112,7 @@ done
 # (with ENOENT in this case but it could be anything).
 cat <<EOF > exp
 rm: cannot remove 'dir'
+Failed to get dirent
 rm: traversal failed: dir
 EOF
 sed 's/\(rm:.*\):.*/\1/' errt > err || framework_failure_
-- 
2.37.3

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, 58050 <at> debbugs.gnu.org
Subject: Re: bug#58050: [INSTALLED] rm: fix diagnostics on I/O error
Date: Sun, 25 Sep 2022 15:25:25 +0100

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

On 25/09/2022 00:37, Paul Eggert wrote:
> I ran into this problem when attempting to recursively
> remove a directory in a filesystem on flaky hardware.
> Although the underlying readdir syscall failed with errno == EIO,
> rm issued no diagnostic about the I/O error.

This looks good.
How about the attached to add a NEWS entry,
and add DS_EMPTY, DS_NONEMPTY enums to make the code easier to read?

thanks,
Pádraig

[rm-use-constants.patch (text/x-patch, attachment)]

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 58050 <at> debbugs.gnu.org, Gnulib bugs <bug-gnulib <at> gnu.org>
Subject: Re: bug#58050: [INSTALLED] rm: fix diagnostics on I/O error
Date: Sun, 25 Sep 2022 18:37:49 -0700

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

On 9/25/22 07:25, Pádraig Brady wrote:
> How about the attached to add a NEWS entry,
> and add DS_EMPTY, DS_NONEMPTY enums to make the code easier to read?

Sure, that looks good; thanks.

Oh, I forgot that via code inspection I found a theoretical portability 
bug in fts while I was looking into Bug#58050. I fixed that by 
installing the attached into Gnulib.

[0001-fts-fix-errno-handling-if-dirfd-fails.patch (text/x-patch, attachment)]

Previous Next

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