Re: [PATCH] Fix preserve_mode when destination directory partially exists

[Paul Eggert]

Jim Meyering <address@hidden> writes:

> 2008-01-07  Jan Blunck  <address@hidden>
>
>       cp --parents: don't use uninitialized memory when restoring permissions

In reviewing that patch I noticed that the bug of using uninitialized
memory still remains in some (unlikely) cases.  If 'stat (src,
&new->st)' fails, the resulting uninitialized new->st buffer still
remains in the list of directories whose modes need fixing later.  As
far as I can tell the bug is triggered only in a race condition, where
a directory is moved as we are trying to copy it, so it's hard to come
up with a test case for it.  However, here's a patch.

2008-01-08  Paul Eggert  <address@hidden>

        Fix a minor race condition when using cp -p --parents.
        * src/cp.c (make_dir_parents_private): If stat fails on the parent
        directory, do not add it to the list of directories whose modes
        might need fixing later.  Also, do not bother invoking 'stat'
        unless the stat results might be needed later.

diff --git a/src/cp.c b/src/cp.c
index 01d98cc..a46f900 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -403,28 +403,39 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
        slash++;
       while ((slash = strchr (slash, '/')))
        {
-         int src_errno;
-         /* Add this directory to the list of directories whose modes need
-            fixing later. */
-         struct dir_attr *new = xmalloc (sizeof *new);
-         new->slash_offset = slash - dir;
-         new->restore_mode = false;
-         new->next = *attr_list;
-         *attr_list = new;
+         struct dir_attr *new IF_LINT (= NULL);
+         bool missing_dir;

          *slash = '\0';
-         src_errno = (stat (src, &new->st) != 0
-                      ? errno
-                      : S_ISDIR (new->st.st_mode)
-                      ? 0
-                      : ENOTDIR);
-         if (src_errno)
+         missing_dir = (stat (dir, &stats) != 0);
+
+         if (missing_dir | x->preserve_ownership | x->preserve_mode
+             | x->preserve_timestamps)
            {
-             error (0, src_errno, _("failed to get attributes of %s"),
-                    quote (src));
-             return false;
+             /* Add this directory to the list of directories whose
+                modes might need fixing later. */
+             struct stat src_st;
+             int src_errno = (stat (src, &src_st) != 0
+                              ? errno
+                              : S_ISDIR (src_st.st_mode)
+                              ? 0
+                              : ENOTDIR);
+             if (src_errno)
+               {
+                 error (0, src_errno, _("failed to get attributes of %s"),
+                        quote (src));
+                 return false;
+               }
+
+             new = xmalloc (sizeof *new);
+             new->st = src_st;
+             new->slash_offset = slash - dir;
+             new->restore_mode = false;
+             new->next = *attr_list;
+             *attr_list = new;
            }
-         if (stat (dir, &stats) != 0)
+
+         if (missing_dir)
            {
              mode_t src_mode;
              mode_t omitted_permissions;