Re: minor mkdir bug fixes

[Paul Eggert]

Jim Meyering <address@hidden> writes:

> --- lib/mkdir-p.c     12 Jun 2005 19:56:37 -0000      1.2
> +++ lib/mkdir-p.c     14 Jun 2005 08:55:37 -0000      1.5
> @@ -63,7 +63,8 @@
>               _("failed to return to initial working directory")); \
>             free_cwd (&cwd);                          \
>             errno = _saved_errno;                     \
> -           return false;                             \
> +           *different_working_dir = true;            \
> +           return true;                              \

This fix doesn't look entirely right to me.  In some cases this code
is executed when the requested directory isn't actually built.  So
this change will cause make_dir_parents to return true even though it
should return false in that case.

I looked at the code and got a bit confused by its CLEANUP and
CLEANUP_CWD macros, so I rewrote it to remove the macros.  I then
fixed this bug along with one or two other bugs that I discovered
while rewriting it.  Also, it seemed slightly cleaner to me if the
newly-added bool result accumulates chdir failures rather than always
being set or cleared.

So I installed this:

2005-06-14  Paul Eggert  <address@hidden>

        * lib/mkdir-p.c (CLEANUP_CWD, CLEANUP): Remove.
        (make_dir_parents): Revamp to avoid need for CLEANUP_CWD, CLEANUP.
        If the file already exists but is not a directory, don't bother
        to try to make its parents.
        Close potential file descriptor leak if we can't chdir("/") (!).
        Don't always return true if chdir($PWD) fails; return true only
        if the requested action was done successfully (except for the
        chdir($PWD)).
        Don't log final directory unless we actually made it.
        Refactor to avoid duplicate code to fix up permissions.
        Don't attempt to fix up parent permissions if chdir($PWD) fails.
        * src/install.c (main): Adjust to new make_dir_parents convention.
        * src/mkdir.c (main): Likewise.

Index: lib/mkdir-p.c
===================================================================
RCS file: /fetish/cu/lib/mkdir-p.c,v
retrieving revision 1.5
diff -p -u -r1.5 mkdir-p.c
--- lib/mkdir-p.c       14 Jun 2005 08:55:37 -0000      1.5
+++ lib/mkdir-p.c       14 Jun 2005 18:34:58 -0000
@@ -49,36 +49,6 @@
 
 #define WX_USR (S_IWUSR | S_IXUSR)
 
-#define CLEANUP_CWD                                    \
-  do                                                   \
-    {                                                  \
-      /* We're done operating on basename_dir.         \
-        Restore working directory.  */                 \
-      if (do_chdir)                                    \
-       {                                               \
-         if (restore_cwd (&cwd) != 0)                  \
-           {                                           \
-             int _saved_errno = errno;                 \
-             error (0, errno,                          \
-               _("failed to return to initial working directory")); \
-             free_cwd (&cwd);                          \
-             errno = _saved_errno;                     \
-             *different_working_dir = true;            \
-             return true;                              \
-           }                                           \
-         free_cwd (&cwd);                              \
-       }                                               \
-    }                                                  \
-  while (0)
-
-#define CLEANUP                                                \
-  do                                                   \
-    {                                                  \
-      umask (oldmask);                                 \
-      CLEANUP_CWD;                                     \
-    }                                                  \
-  while (0)
-
 /* Attempt to create directory DIR (aka FULLDIR) with the specified MODE.
    If CREATED_DIR_P is non-NULL, set *CREATED_DIR_P if this
    function creates DIR and clear it otherwise.  Give a diagnostic and
@@ -145,9 +115,10 @@ make_dir (char const *dir, char const *f
    with the name of the directory that was just made as an argument.
    If PRESERVE_EXISTING is true and ARG is an existing directory,
    then do not attempt to set its permissions and ownership.
-   Upon return, set *DIFFERENT_WORKING_DIR to true if this function
-   has changed the current working directory and is unable to restore
-   it to its initial state.
+
+   Set *DIFFERENT_WORKING_DIR to true if this function has changed the
+   current working directory and is unable to restore it to its
+   initial state.  Do not change *DIFFERENT_WORKING_DIR otherwise.
 
    Return true iff ARG exists as a directory with the proper ownership
    and permissions when done.  Note that this function returns true
@@ -165,21 +136,40 @@ make_dir_parents (char const *arg,
 {
   struct stat stats;
   bool retval = true;
-  *different_working_dir = false;
+  bool do_chdir = false;       /* Whether to chdir before each mkdir.  */
+  struct saved_cwd cwd;
+  bool cwd_problem = false;
+  char const *fixup_permissions_dir = NULL;
+  char const *full_dir = arg;
+
+  struct ptr_list
+  {
+    char *dirname_end;
+    struct ptr_list *next;
+  };
+  struct ptr_list *leading_dirs = NULL;
 
-  if (stat (arg, &stats) != 0)
+  if (stat (arg, &stats) == 0)
+    {
+      if (! S_ISDIR (stats.st_mode))
+       {
+         error (0, 0, _("%s exists but is not a directory"), quote (arg));
+         return false;
+       }
+
+      if (!preserve_existing)
+       fixup_permissions_dir = arg;
+    }
+  else if (errno != ENOENT || !*arg)
+    {
+      error (0, errno, "%s", quote (arg));
+      return false;
+    }
+  else
     {
       char *slash;
       mode_t tmp_mode;         /* Initial perms for leading dirs.  */
       bool re_protect;         /* Should leading dirs be unwritable? */
-      struct ptr_list
-      {
-       char *dirname_end;
-       struct ptr_list *next;
-      };
-      struct ptr_list *p, *leading_dirs = NULL;
-      bool do_chdir;           /* Whether to chdir before each mkdir.  */
-      struct saved_cwd cwd;
       char *basename_dir;
       char *dir;
 
@@ -190,6 +180,7 @@ make_dir_parents (char const *arg,
       dir = (char *) alloca (strlen (arg) + 1);
       strcpy (dir, arg);
       strip_trailing_slashes (dir);
+      full_dir = dir;
 
       /* If leading directories shouldn't be writable or executable,
         or should have set[ug]id or sticky bits set and we are setting
@@ -220,7 +211,10 @@ make_dir_parents (char const *arg,
             file name starts with exactly two slashes.  */
          char const *root = "//" + (dir[1] != '/' || dir[2] == '/');
          if (chdir (root) != 0)
-           do_chdir = false;
+           {
+             free_cwd (&cwd);
+             do_chdir = false;
+           }
        }
 
       slash = dir;
@@ -229,7 +223,7 @@ make_dir_parents (char const *arg,
       while (*slash == '/')
        slash++;
 
-      while (1)
+      while (true)
        {
          bool newly_created_dir;
 
@@ -248,8 +242,8 @@ make_dir_parents (char const *arg,
          *slash = '\0';
          if (! make_dir (basename_dir, dir, tmp_mode, &newly_created_dir))
            {
-             CLEANUP;
-             return false;
+             retval = false;
+             break;
            }
 
          if (newly_created_dir)
@@ -266,8 +260,8 @@ make_dir_parents (char const *arg,
                {
                  error (0, errno, _("cannot change owner and/or group of %s"),
                         quote (dir));
-                 CLEANUP;
-                 return false;
+                 retval = false;
+                 break;
                }
 
              if (re_protect)
@@ -288,8 +282,8 @@ make_dir_parents (char const *arg,
            {
              error (0, errno, _("cannot chdir to directory %s"),
                     quote (dir));
-             CLEANUP;
-             return false;
+             retval = false;
+             break;
            }
 
          *slash++ = '/';
@@ -308,26 +302,40 @@ make_dir_parents (char const *arg,
 
       /* We're done making leading directories.
         Create the final component of the file name.  */
-
-      if (! make_dir (basename_dir, dir, mode, NULL))
+      if (retval)
        {
-         CLEANUP;
-         return false;
+         bool newly_created_dir;
+
+         if (! make_dir (basename_dir, dir, mode, &newly_created_dir))
+           retval = false;
+
+         if (newly_created_dir)
+           {
+             if (verbose_fmt_string)
+               error (0, 0, verbose_fmt_string, quote (dir));
+             fixup_permissions_dir = basename_dir;
+           }
        }
+    }
 
-      if (verbose_fmt_string != NULL)
-       error (0, 0, verbose_fmt_string, quote (dir));
+  if (fixup_permissions_dir)
+    {
+      /* chown must precede chmod because on some systems,
+        chown clears the set[ug]id bits for non-superusers,
+        resulting in incorrect permissions.
+        On System V, users can give away files with chown and then not
+        be able to chmod them.  So don't give files away.  */
 
       if (owner != (uid_t) -1 || group != (gid_t) -1)
        {
-         if (chown (basename_dir, owner, group)
+         if (chown (fixup_permissions_dir, owner, group) != 0
 #ifdef AFS
              && errno != EPERM
 #endif
              )
            {
              error (0, errno, _("cannot change owner and/or group of %s"),
-                    quote (dir));
+                    quote (full_dir));
              retval = false;
            }
        }
@@ -337,67 +345,39 @@ make_dir_parents (char const *arg,
         required to honor only the file permission bits.  In particular,
         it need not honor the `special' bits, so if MODE includes any
         special bits, set them here.  */
-      if ((mode & ~S_IRWXUGO)
-         && chmod (basename_dir, mode))
+      if ((mode & ~S_IRWXUGO) && chmod (fixup_permissions_dir, mode) != 0)
        {
          error (0, errno, _("cannot change permissions of %s"),
-                quote (dir));
+                quote (full_dir));
          retval = false;
        }
-
-      CLEANUP_CWD;
-
-      /* If the mode for leading directories didn't include owner "wx"
-        privileges, we have to reset their protections to the correct
-        value.  */
-      for (p = leading_dirs; p != NULL; p = p->next)
-       {
-         *(p->dirname_end) = '\0';
-         if (chmod (dir, parent_mode) != 0)
-           {
-             error (0, errno, _("cannot change permissions of %s"),
-                    quote (dir));
-             retval = false;
-           }
-       }
     }
-  else
-    {
-      /* We get here if the file already exists.  */
-
-      char const *dir = arg;
 
-      if (!S_ISDIR (stats.st_mode))
-       {
-         error (0, 0, _("%s exists but is not a directory"), quote (dir));
-         return false;
+  if (do_chdir)
+    {
+      int saved_errno;
+      cwd_problem = (restore_cwd (&cwd) != 0);
+      saved_errno = errno;
+      free_cwd (&cwd);
+
+      if (cwd_problem)
+       {
+         error (0, saved_errno,
+                _("failed to return to initial working directory"));
+         *different_working_dir = true;
        }
+    }
 
-      if (!preserve_existing)
+  /* If the mode for leading directories didn't include owner "wx"
+     privileges, reset their protections to the correct value.  */
+  for (; leading_dirs != NULL; leading_dirs = leading_dirs->next)
+    {
+      leading_dirs->dirname_end[0] = '\0';
+      if (cwd_problem || chmod (full_dir, parent_mode) != 0)
        {
-         /* chown must precede chmod because on some systems,
-            chown clears the set[ug]id bits for non-superusers,
-            resulting in incorrect permissions.
-            On System V, users can give away files with chown and then not
-            be able to chmod them.  So don't give files away.  */
-
-         if ((owner != (uid_t) -1 || group != (gid_t) -1)
-             && chown (dir, owner, group)
-#ifdef AFS
-             && errno != EPERM
-#endif
-             )
-           {
-             error (0, errno, _("cannot change owner and/or group of %s"),
-                    quote (dir));
-             retval = false;
-           }
-         if (chmod (dir, mode) != 0)
-           {
-             error (0, errno, _("cannot change permissions of %s"),
-                                quote (dir));
-             retval = false;
-           }
+         error (0, (cwd_problem ? 0 : errno),
+                _("cannot change permissions of %s"), quote (full_dir));
+         retval = false;
        }
     }
 
Index: src/install.c
===================================================================
RCS file: /fetish/cu/src/install.c,v
retrieving revision 1.182
diff -p -u -r1.182 install.c
--- src/install.c       14 Jun 2005 08:18:48 -0000      1.182
+++ src/install.c       14 Jun 2005 18:34:58 -0000
@@ -360,7 +360,6 @@ main (int argc, char **argv)
       bool cwd_not_restored = false;
       for (i = 0; i < n_files; i++)
        {
-         bool different_cwd;
          if (cwd_not_restored && IS_RELATIVE_FILE_NAME (argv[optind]))
            {
              error (0, 0,
@@ -374,8 +373,7 @@ main (int argc, char **argv)
          ok &=
            make_dir_parents (file[i], mode, mode, owner_id, group_id, false,
                              (x.verbose ? _("creating directory %s") : NULL),
-                             &different_cwd);
-         cwd_not_restored |= different_cwd;
+                             &cwd_not_restored);
        }
     }
   else
Index: src/mkdir.c
===================================================================
RCS file: /fetish/cu/src/mkdir.c,v
retrieving revision 1.99
diff -p -u -r1.99 mkdir.c
--- src/mkdir.c 14 Jun 2005 08:18:43 -0000      1.99
+++ src/mkdir.c 14 Jun 2005 18:34:59 -0000
@@ -87,7 +87,7 @@ main (int argc, char **argv)
   const char *verbose_fmt_string = NULL;
   int exit_status = EXIT_SUCCESS;
   int optc;
-  bool cwd_not_restored;
+  bool cwd_not_restored = false;
 
   initialize_main (&argc, &argv);
   program_name = argv[0];
@@ -146,8 +146,6 @@ main (int argc, char **argv)
        umask (umask_value);
     }
 
-  /* FIXME: when we assume C99, declare this here.  */
-  cwd_not_restored = false;
   for (; optind < argc; ++optind)
     {
       bool ok;
@@ -163,12 +161,10 @@ main (int argc, char **argv)
 
       if (create_parents)
        {
-         bool different_cwd;
          char *dir = argv[optind];
          ok = make_dir_parents (dir, newmode, parent_mode,
                                 -1, -1, true, verbose_fmt_string,
-                                &different_cwd);
-         cwd_not_restored |= different_cwd;
+                                &cwd_not_restored);
        }
       else
        {