rm -r would get a failed assertion when run from an inaccessible

directory and with two or more command line arguments including an
absolute-named directory followed by a relative-named directory.

(struct cwd_state): Define.
(AD_pop_and_chdir): Redesign interface so that a restore_cwd failure
can be detected by the caller.  Instead of returning a malloc'd
directory name, communicate it to caller via a new parameter, and
return an indication of whether restore_cwd failed.  Update caller.
Eliminate an unnecessary call to AC_stack_top.
(remove_dir): Change type of cwd_state parameter to `struct cwd_state'
so we can now communicate to caller whether/how functions like
restore_cwd have failed.  Update caller.
(rm_1): Fail if we've failed to restore the working directory
and the name of the next file to remove is `.'-relative.
(rm): Fail if the require_restore_cwd flag is true and we've
failed to restore the working directory.

diff --git a/src/remove.c b/src/remove.c
index c29623a..4bb90d0 100644 (file)
--- a/src/remove.c
+++ b/src/remove.c
@@ -146,6 +146,16 @@ struct dirstack_state
 };
 typedef struct dirstack_state Dirstack_state;
 
+struct cwd_state
+{
+  /* The value of errno after a failed save_cwd or restore_cwd.  */
+  int saved_errno;
+
+  /* Information (open file descriptor or absolute directory name)
+     required in order to restore the initial working directory.  */
+  struct saved_cwd saved_cwd;
+};
+
 static Dirstack_state *
 ds_init ()
 {
@@ -361,16 +371,19 @@ AD_stack_pop (Dirstack_state *ds)
 /* chdir `up' one level.
    Whenever using chdir '..', verify that the post-chdir
    dev/ino numbers for `.' match the saved ones.
-   Return the name (in malloc'd storage) of the
-   directory (usually now empty) from which we're coming.  */
-static char *
-AD_pop_and_chdir (Dirstack_state *ds)
+   Set *PREV_DIR to the name (in malloc'd storage) of the
+   directory (usually now empty) from which we're coming.
+   Return nonzero upon failure of restore_cwd.  Otherwise, return 0. */
+static int
+AD_pop_and_chdir (Dirstack_state *ds, char **prev_dir)
 {
-  /* Get the name of the current directory from the top of the stack.  */
-  char *dir = top_dir (ds);
   enum RM_status old_status = AD_stack_top(ds)->status;
   struct AD_ent *top;
 
+  /* Get the name of the current (but soon to be `previous') directory
+     from the top of the stack.  */
+  *prev_dir = top_dir (ds);
+
   AD_stack_pop (ds);
   top = AD_stack_top (ds);
 
@@ -384,7 +397,7 @@ AD_pop_and_chdir (Dirstack_state *ds)
       struct stat sb;
 
       /* We can give a better diagnostic here, since the target is relative. */
-      if (chdir (".."))
+      if (chdir ("..") != 0)
        {
          error (EXIT_FAILURE, errno,
                 _("cannot chdir from %s to .."),
@@ -402,12 +415,14 @@ AD_pop_and_chdir (Dirstack_state *ds)
     }
   else
     {
-      if (restore_cwd (&top->u.saved_cwd))
-       error (EXIT_FAILURE, errno,
-              _("failed to return to initial working directory"));
+      if (restore_cwd (&top->u.saved_cwd) != 0)
+       {
+         /* failed to return to initial working directory */
+         return 1;
+       }
     }
 
-  return dir;
+  return 0;
 }
 
 /* Initialize *HT if it is NULL.
@@ -947,10 +962,11 @@ The following directory is part of the cycle:\n  %s\n"),
     return to the same directory from which we came, if necessary.
     Return 1 for success, 0 if some file cannot be removed or if
     a chdir fails.
-    If the working directory cannot be restored, exit immediately.  */
+    If the initial working directory cannot be saved or restored,
+    record the offending errno value in (*CWD_STATE)->saved_errno.  */
 
 static enum RM_status
-remove_dir (Dirstack_state *ds, char const *dir, struct saved_cwd **cwd_state,
+remove_dir (Dirstack_state *ds, char const *dir, struct cwd_state **cwd_state,
            struct rm_options const *x)
 {
   enum RM_status status;
@@ -963,15 +979,34 @@ remove_dir (Dirstack_state *ds, char const *dir, struct saved_cwd **cwd_state,
   if (*cwd_state == NULL)
     {
       *cwd_state = xmalloc (sizeof **cwd_state);
-      if (save_cwd (*cwd_state))
+
+      if (save_cwd (&(*cwd_state)->saved_cwd) != 0)
        {
-         error (0, errno, _("cannot get current directory"));
-         return RM_ERROR;
+         (*cwd_state)->saved_errno = errno;
+         assert (errno != 0);
+
+         /* Pretend we started from ".".  That is fine as long as there
+            is no requirement to return to the original working directory.  */
+         (*cwd_state)->saved_cwd.name = xstrdup (".");
        }
-      AD_push_initial (ds, *cwd_state);
+      else
+       (*cwd_state)->saved_errno = 0;
+
+      AD_push_initial (ds, &(*cwd_state)->saved_cwd);
       AD_INIT_OTHER_MEMBERS ();
     }
 
+  /* If we've failed to record and/or restore the initial working directory,
+     and we're now trying to access a `.'-relative file name, then give a
+     diagnostic, record the failure, and proceed with any subsequent
+     command-line arguments.  */
+  if ((*cwd_state)->saved_errno && IS_RELATIVE_FILE_NAME (dir))
+    {
+      error (0, (*cwd_state)->saved_errno, _("cannot remove directory %s"),
+            quote (full_filename (dir)));
+      longjmp (ds->current_arg_jumpbuf, 1);
+    }
+
   /* There is a race condition in that an attacker could replace the nonempty
      directory, DIR, with a symlink between the preceding call to rmdir
      (in our caller) and the chdir below.  However, the following lstat,
@@ -1036,7 +1071,18 @@ remove_dir (Dirstack_state *ds, char const *dir, struct saved_cwd **cwd_state,
       /* Execution reaches this point when we've removed the last
         removable entry from the current directory.  */
       {
-       char *d = AD_pop_and_chdir (ds);
+       /* This is the name of the directory that we have just
+          returned from, after nominally removing all of its contents.  */
+       char *empty_dir;
+
+       if (AD_pop_and_chdir (ds, &empty_dir) != 0)
+         (*cwd_state)->saved_errno = errno;
+
+       /* Note: the above may have failed due to restore_cwd failure.
+          That failure may be harmless if x->require_restore_cwd is false,
+          but we do have to remember that fact, including the errno value,
+          so we can give an accurate diagnostic when reporting the failure
+          to remove a subsequent relative-named command-line argument.  */
 
        /* Try to remove D only if remove_cwd_entries succeeded.  */
        if (tmp_status == RM_OK)
@@ -1047,32 +1093,32 @@ remove_dir (Dirstack_state *ds, char const *dir, struct saved_cwd **cwd_state,
               But that's no big deal since we're interactive.  */
            Ternary is_dir;
            Ternary is_empty;
-           enum RM_status s = prompt (ds, d, x, PA_REMOVE_DIR,
+           enum RM_status s = prompt (ds, empty_dir, x, PA_REMOVE_DIR,
                                       &is_dir, &is_empty);
 
            if (s != RM_OK)
              {
-               free (d);
+               free (empty_dir);
                return s;
              }
 
-           if (rmdir (d) == 0)
+           if (rmdir (empty_dir) == 0)
              {
                if (x->verbose)
                  printf (_("removed directory: %s\n"),
-                         quote (full_filename (d)));
+                         quote (full_filename (empty_dir)));
              }
            else
              {
                error (0, errno, _("cannot remove directory %s"),
-                      quote (full_filename (d)));
-               AD_mark_as_unremovable (ds, d);
+                      quote (full_filename (empty_dir)));
+               AD_mark_as_unremovable (ds, empty_dir);
                status = RM_ERROR;
                UPDATE_STATUS (AD_stack_top(ds)->status, status);
              }
          }
 
-       free (d);
+       free (empty_dir);
 
        if (AD_stack_height (ds) == 1)
          break;
@@ -1091,7 +1137,7 @@ remove_dir (Dirstack_state *ds, char const *dir, struct saved_cwd **cwd_state,
 
 static enum RM_status
 rm_1 (Dirstack_state *ds, char const *filename,
-      struct rm_options const *x, struct saved_cwd **cwd_state)
+      struct rm_options const *x, struct cwd_state **cwd_state)
 {
   char *base = base_name (filename);
   enum RM_status status;
@@ -1102,6 +1148,14 @@ rm_1 (Dirstack_state *ds, char const *filename,
       return RM_ERROR;
     }
 
+  if (*cwd_state && (*cwd_state)->saved_errno
+      && IS_RELATIVE_FILE_NAME (filename))
+    {
+      error (0, (*cwd_state)->saved_errno,
+            _("cannot remove %s"), quote (filename));
+      return RM_ERROR;
+    }
+
   status = remove_entry (ds, filename, x, NULL);
   if (status != RM_NONEMPTY_DIR)
     return status;
@@ -1114,7 +1168,7 @@ rm_1 (Dirstack_state *ds, char const *filename,
 extern enum RM_status
 rm (size_t n_files, char const *const *file, struct rm_options const *x)
 {
-  struct saved_cwd *cwd_state = NULL;
+  struct cwd_state *cwd_state = NULL;
   Dirstack_state *ds;
 
   /* Put the following two variables in static storage, so they can't
@@ -1139,6 +1193,13 @@ rm (size_t n_files, char const *const *file, struct rm_options const *x)
       UPDATE_STATUS (status, s);
     }
 
+  if (x->require_restore_cwd && cwd_state && cwd_state->saved_errno != 0)
+    {
+      error (0, cwd_state->saved_errno,
+            _("cannot restore current working directory"));
+      status = RM_ERROR;
+    }
+
   ds_free (ds);
 
   free (cwd_state);