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

[Jan Blunck]

On Fri, Jan 04, Paul Eggert wrote:

> Jan Blunck <address@hidden> writes:
> 
> > I found a bug with cp -p --parents when the destination partially exists and
> > the filesystem isn't mounted with acls.
> >
> > $ mkdir -p a/b/c a/b/d e
> > $ touch a/b/c/foo a/b/d/foo
> > $ cp -p --parent a/b/c e
> > $ cp -p --parent a/b/d e
> > $ ls -ld e/a
> > d--------- 3 jblunck suse 4096 1970-01-01 01:00 e/a
> 
> I can't reproduce the problem on my Debian stable host, using
> coreutils 6.9.91:
> 
>   $ mkdir -p a/b/c a/b/d e
>   $ touch a/b/c/foo a/b/d/foo
>   $ cp -p --parent a/b/c e
>   cp: omitting directory `a/b/c'

Oh, my testcase should read as follows
$ cp -p --parent a/b/c/foo e
$ cp -p --parent a/b/d/foo e

Anyway, the first cp succeeds. The second call to cp is setting the mode in
re_protect() to 0. This happens since we do not fill new->st if the
destination directory is already existing:

>From make_dir_parents_private():

          *slash = '\0';
          if (stat (dir, &stats) != 0)
            {
              mode_t src_mode;
              mode_t omitted_permissions;
              mode_t mkdir_mode;
              int src_errno;

              /* This component does not exist.  We must set
                 *new_dst and new->st.st_mode inside this loop because,
                 for example, in the command `cp --parents ../a/../b/c e_dir',
                 make_dir_parents_private creates only e_dir/../a if
                 ./b already exists. */
              *new_dst = true;
              src_errno = (stat (src, &new->st) != 0
                           ? errno
                           : S_ISDIR (new->st.st_mode)
                           ? 0
                           : ENOTDIR);
              if (src_errno)
                {
                  error (0, src_errno, _("failed to get attributes of %s"),
                         quote (src));
                  return false;
                }
              src_mode = new->st.st_mode;


Therefore re_protect() is calling copy_acl() with argument 0 for the
destination mode. If the filesystem isn't capable of handling acls, copy_acl()
is using chmod_or_fchmod() instead. This leads to the described bug.

> > I'm not sure if reseting the permissions of the already existing directories
> > is the right thing to do. At least it is the same behavior that cp had 
> > before
> > the above commit.
> 
> It sounds like some fix is needed, but I'm afraid I don't understand
> the bug or the patch yet.

The patch is changing the stat() of the source directory to be unconditional
of the existence of the destination directory.

Hope that helps to understand the bug,
Jan