Subject: Re: 26.0.50; copy-directory no longer creates parent directories of target if they do not exist

From: Andrew Christianson <[email-masked]>
Date: Wed, 13 Sep 2017 21:34:47 -0700

Ah.  I didn’t fully specify my test case, sorry about that.  In reality I did mkdir ~/test; touch ~/test/{a,b,c,d} .

Looks like that’s the key though.  If the first file in the directory to be copied is a normal file, not a directory, the call fails. So

mkdir ~/test
touch ~/test/{a,b,c,d}
path/to/emacs -Q -batch -eval '(copy-directory "~/test" "~/a/new/directory/" t t t)’

will fail, but

mkdir -p ~/test2/a
touch ~/test2/{b,c,d}
touch ~/test2/a/e
path/to/emacs -Q -batch -eval '(copy-directory "~/test2" "~/another/new/directory/" t t t)’

will succeed.

I've replicated the former behavior on Fedora 26, on a clean build of the most recent master (bc511a64f6). M-x report-emacs-bug template for that system is attached, along with the strace output.  There are no references to mkdir, unfortunately.  It seems like the issue is that copy-directory never actually calls make-directory in this case.  If neither condition in the middle cond block (list/files.el#5543) applies (which seems to be why this only happens with a NEWNAME with a trailing slash, and non-nil COPY-CONTENTS) then copy-directory just proceeds to the dolist, and if the first item is a file, it goes straight to copy-file, which then fails, as the target directory doesn’t exist.

Looks like the issue may have arose in commit e22794867d878d53675fcc91d2ef1ad2494a2ff2, trading file-directory-p for directory-name-p in the first first condition in that cond block.

Would adding a condition like:

1 file changed, 3 insertions(+), 1 deletion(-)
lisp/files.el | 4 +++-

modified   lisp/files.el
@@ -5541,31 +5541,33 @@ into NEWNAME instead."
 	    newname (expand-file-name newname))
 
       (cond ((not (directory-name-p newname))
 	     ;; If NEWNAME is not a directory name, create it;
 	     ;; that is where we will copy the files of DIRECTORY.
 	     (make-directory newname parents))
 	    ;; If NEWNAME is a directory name and COPY-CONTENTS
 	    ;; is nil, copy into NEWNAME/[DIRECTORY-BASENAME].
 	    ((not copy-contents)
 	     (setq newname (concat newname
 			    (file-name-nondirectory directory)))
 	     (and (file-exists-p newname)
 		  (not (file-directory-p newname))
 		  (error "Cannot overwrite non-directory %s with a directory"
 			 newname))
-	     (make-directory newname t)))
+	     (make-directory newname t))
+            ((and copy-contents (not (file-directory-p newname)))
+             (make-directory newname parents)))
 
       ;; Copy recursively.

Impact the current security concern?

Andrew

> On Sep 13, 2017, at 6:22 PM, Paul Eggert <[email-masked]> wrote:
> 
> Thanks for the bug report and test case. I cannot reproduce the problem on Fedora 26 x86-64.
> 
> What are the permissions on the files and directories involved? E.g., 'cd; ls -lR a test'.
> 
> I have a sneaking suspicion that the problem lies in the recent changes I made to make-directory (commit cf9891e14e48a93bca2065fdd7998f5f677786dc). Can you please try something like this:
> 
> cd
> rm -fr a test
> mkdir -p a test/{a,b,c,d}
> strace -o tr path/to/emacs -Q -batch -eval '(copy-directory "~/test" "~/a/new/directory/" t t t)'
> grep mkdir tr
> 
> Here's what I observe on Fedora:
> 
> mkdir("/home/eggert/a/new/directory/a", 0777) = -1 ENOENT (No such file or directory)
> mkdir("/home/eggert/a/new/directory", 0777) = -1 ENOENT (No such file or directory)
> mkdir("/home/eggert/a/new", 0777)       = 0
> mkdir("/home/eggert/a/new/directory", 0777) = 0
> mkdir("/home/eggert/a/new/directory/a", 0777) = 0
> mkdir("/home/eggert/a/new/directory/b", 0777) = 0
> mkdir("/home/eggert/a/new/directory/c", 0777) = 0
> mkdir("/home/eggert/a/new/directory/d", 0777) = 0
> 
> which has the desired behavior. If Darwin doesn't have strace, please use the equivalent there to trace system calls
> 
> If you don't have an strace equivalent, please try make-directory and see whether it has a similar problem:
> 
> cd
> rm -fr a
> mkdir a
> path/to/emacs -Q -batch -eval '(make-directory "a/new/directory/a" t)'
> ls -al a/new/directory/a
>