bug#27986: 26.0.50; `rename-file' can rename files without confirmation

bug#27986: 26.0.50; `rename-file' can rename files without confirmation

[Philipp]

Run the following in *scratch*:

(make-directory "/tmp/emacs")
nil
(write-region "" nil "/tmp/emacs/ß")
nil
(write-region "" nil "/tmp/emacs/ẞ")
nil
(directory-files "/tmp/emacs")
("." ".." "ß" "ẞ")
(rename-file "/tmp/emacs/ẞ" "/tmp/emacs/ß")
nil

Note how `rename-file' has silently overwritten `ß'.  This is because on
macOS, `ß' and `ẞ' are different file names, but Emacs treats them as
equal.  Probably the test for case-insensitive file names should be
removed altogether (it can't work correctly and introduces a filesystem
race), and `rename-file' should use link(2) + unlink(2) if renameat2
isn't available.


In GNU Emacs 26.0.50 (build 77, x86_64-apple-darwin16.7.0, NS appkit-1504.83 Version 10.12.6 (Build 16G29))
 of 2017-08-06 built on p
Repository revision: b1b99edd3ee587a5154106d4d16547eac4916c55
Windowing system distributor 'Apple', version 10.3.1504
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --with-modules --without-xml2 --without-pop --with-mailutils
 --enable-gcc-warnings=yes MAKEINFO=/usr/local/opt/texinfo/bin/makeinfo
 'CFLAGS=-O3 -g0' LDFLAGS=-O3'

Configured features:
DBUS NOTIFY ACL GNUTLS ZLIB TOOLKIT_SCROLL_BARS NS MODULES

Important settings:
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils time-date tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/ns-win
ns-win ucs-normalize mule-util term/common-win tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
elisp-mode lisp-mode prog-mode register page menu-bar rfn-eshadow
isearch timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript charprop case-table epa-hook
jka-cmpr-hook help simple abbrev obarray minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote dbusbind kqueue cocoa ns multi-tty
make-network-process emacs)

Memory information:
((conses 16 204143 9065)
 (symbols 48 20146 1)
 (miscs 40 43 181)
 (strings 32 29176 1731)
 (string-bytes 1 784929)
 (vectors 16 35010)
 (vector-slots 8 710995 9656)
 (floats 8 48 68)
 (intervals 56 205 0)
 (buffers 992 11))

bug#27986: 26.0.50; `rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Sun, 06 Aug 2017 17:40:18 +0200
> 
> (rename-file "/tmp/emacs/ẞ" "/tmp/emacs/ß")
> nil
> 
> Note how `rename-file' has silently overwritten `ß'.  This is because on
> macOS, `ß' and `ẞ' are different file names, but Emacs treats them as
> equal.  Probably the test for case-insensitive file names should be
> removed altogether

Which one? there are two of them.

> (it can't work correctly and introduces a filesystem race)

It cannot work correctly _because_ of a possible race or because of
some other reasons?  If the latter, please elaborate.  If the former,
then at least on MS-Windows we have a race anyway, because the
underlying system APIs are not atomic.  So at least on MS-Windows the
feature should stay, as it introduces no new issues and supports a
valid and quite important use case (the Windows shell supports it as
well, so we really cannot do less).

> and `rename-file' should use link(2) + unlink(2) if renameat2
> isn't available.

'link' and 'unlink' accept strings as arguments, not integer numbers
such as 2.

More to the point, how can this strategy work on a case-insensitive
filesystem?  What am I missing?

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

>> Probably the test for case-insensitive file names should be
>> removed altogether
> 
> Which one? there are two of them.

There should be just one such test, since it's testing the same thing and 
there's no point to doing the same test twice. And I notice the code has some 
other duplicate system calls. I installed the attached patch, which prunes away 
some of these duplicates. With this patch, (rename-file "a" "b/") now takes just 
one system call on recent GNU/Linux, whereas it used to take four (and was not 
atomic).

However, even with this patch there are races on GNU/Linux which can lead to 
potential security problems. Perhaps we can't fix these races on MS-Windows but 
we should be able to fix them on a GNUish host. However, we will need to change 
the semantics of rename-file etc. slightly, since no single system call supports 
the cp-like target rewriting of these functions. I have a fix in mind to do that 
in a hopefully compatible-enough way, which I'll try to propose soon. I'll keep 
case-insensitive file systems in mind when I do that.

This reminds me of a similar problem in GNU coreutils which I fixed a dozen 
years ago by adding the -T option to mv, ln, etc.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-performance-for-rename-file-etc.patch --]
[-- Type: text/x-patch; name="0001-Improve-performance-for-rename-file-etc.patch", Size: 13653 bytes --]

From a56e6e79613779895975b1762c311bf8fe46f551 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 11 Aug 2017 01:04:30 -0700
Subject: [PATCH] Improve performance for rename-file etc.

Although this does not fix Bug#27986, it is a step forward.
I plan to propose a more-significant patch later.
* lisp/files.el (directory-name-p): Move from here ...
* src/fileio.c (Fdirectory_name_p): ... to here.
(directory_like, cp_like_target): New static functions.
(Fcopy_file, Frename_file, Fadd_name_to_file)
(Fmake_symbolic_link):
Use them, to avoid directory-testing syscalls on file names that
must be directories if they exist.  Omit unnecessary
initializations and CHECK_STRING calls.
(Frename_file): Don't call file_name_case_insensitive_p
twice on the same file.  Compare both file names expanded, instead
of the old name expanded and the new one unexpanded.
---
 lisp/files.el |  10 -----
 src/fileio.c  | 127 ++++++++++++++++++++++++++++++----------------------------
 2 files changed, 66 insertions(+), 71 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index f2758ab..0fe7f9c 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -792,16 +792,6 @@ cd
                     (lambda (f) (and (file-directory-p f) 'dir-ok)))
        (error "No such directory found via CDPATH environment variable"))))
 
-(defsubst directory-name-p (name)
-  "Return non-nil if NAME ends with a directory separator character."
-  (let ((len (length name))
-        (lastc ?.))
-    (if (> len 0)
-        (setq lastc (aref name (1- len))))
-    (or (= lastc ?/)
-        (and (memq system-type '(windows-nt ms-dos))
-             (= lastc ?\\)))))
-
 (defun directory-files-recursively (dir regexp &optional include-directories)
   "Return list of all files under DIR that have file names matching REGEXP.
 This function works recursively.  Files are returned in \"depth first\"
diff --git a/src/fileio.c b/src/fileio.c
index 15845e3..9aae7d9 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -267,9 +267,9 @@ Otherwise, return nil.
 A file name is handled if one of the regular expressions in
 `file-name-handler-alist' matches it.
 
-If OPERATION equals `inhibit-file-name-operation', then we ignore
+If OPERATION equals `inhibit-file-name-operation', then ignore
 any handlers that are members of `inhibit-file-name-handlers',
-but we still do run any other handlers.  This lets handlers
+but still do run any other handlers.  This lets handlers
 use the standard functions without calling themselves recursively.  */)
   (Lisp_Object filename, Lisp_Object operation)
 {
@@ -583,6 +583,38 @@ directory_file_name (char *dst, char *src, ptrdiff_t srclen, bool multibyte)
   return srclen;
 }
 
+DEFUN ("directory-name-p", Fdirectory_name_p, Sdirectory_name_p, 1, 1, 0,
+       doc: /* Return non-nil if NAME ends with a directory separator character.  */)
+  (Lisp_Object name)
+{
+  CHECK_STRING (name);
+  ptrdiff_t namelen = SBYTES (name);
+  unsigned char c = namelen ? SREF (name, namelen - 1) : 0;
+  return IS_DIRECTORY_SEP (c) ? Qt : Qnil;
+}
+
+/* Return true if NAME must be that of a directory if it exists.
+   When NAME is a directory name, this avoids system calls compared to
+   just calling Ffile_directory_p.  */
+
+static bool
+directory_like (Lisp_Object name)
+{
+  return !NILP (Fdirectory_name_p (name)) || !NILP (Ffile_directory_p (name));
+}
+
+/* Return the expansion of NEWNAME, except that if NEWNAME is like a
+   directory then return the expansion of FILE's basename under
+   NEWNAME.  This is like how 'cp FILE NEWNAME' works.  */
+
+static Lisp_Object
+expand_cp_target (Lisp_Object file, Lisp_Object newname)
+{
+  return (directory_like (newname)
+	  ? Fexpand_file_name (Ffile_name_nondirectory (file), newname)
+	  : Fexpand_file_name (newname, Qnil));
+}
+
 DEFUN ("directory-file-name", Fdirectory_file_name, Sdirectory_file_name,
        1, 1, 0,
        doc: /* Returns the file name of the directory named DIRECTORY.
@@ -1874,9 +1906,9 @@ This function always sets the file modes of the output file to match
 the input file.
 
 The optional third argument OK-IF-ALREADY-EXISTS specifies what to do
-if file NEWNAME already exists.  If OK-IF-ALREADY-EXISTS is nil, we
+if file NEWNAME already exists.  If OK-IF-ALREADY-EXISTS is nil,
 signal a `file-already-exists' error without overwriting.  If
-OK-IF-ALREADY-EXISTS is a number, we request confirmation from the user
+OK-IF-ALREADY-EXISTS is an integer, request confirmation from the user
 about overwriting; this is what happens in interactive use with M-x.
 Any other value for OK-IF-ALREADY-EXISTS means to overwrite the
 existing file.
@@ -1886,8 +1918,8 @@ last-modified time as the old one.  (This works on only some systems.)
 
 A prefix arg makes KEEP-TIME non-nil.
 
-If PRESERVE-UID-GID is non-nil, we try to transfer the
-uid and gid of FILE to NEWNAME.
+If PRESERVE-UID-GID is non-nil, try to transfer the uid and gid of
+FILE to NEWNAME.
 
 If PRESERVE-PERMISSIONS is non-nil, copy permissions of FILE to NEWNAME;
 this includes the file modes, along with ACL entries and SELinux
@@ -1914,16 +1946,8 @@ permissions.  */)
   struct stat st;
 #endif
 
-  encoded_file = encoded_newname = Qnil;
-  CHECK_STRING (file);
-  CHECK_STRING (newname);
-
-  if (!NILP (Ffile_directory_p (newname)))
-    newname = Fexpand_file_name (Ffile_name_nondirectory (file), newname);
-  else
-    newname = Fexpand_file_name (newname, Qnil);
-
   file = Fexpand_file_name (file, Qnil);
+  newname = expand_cp_target (file, newname);
 
   /* If the input file name has special constructs in it,
      call the corresponding file handler.  */
@@ -2304,9 +2328,9 @@ DEFUN ("rename-file", Frename_file, Srename_file, 2, 3,
        "fRename file: \nGRename %s to file: \np",
        doc: /* Rename FILE as NEWNAME.  Both args must be strings.
 If file has names other than FILE, it continues to have those names.
-Signals a `file-already-exists' error if a file NEWNAME already exists
+Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-A number as third arg means request confirmation if NEWNAME already exists.
+An integer third arg means request confirmation if NEWNAME already exists.
 This is what happens in interactive use with M-x.  */)
   (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists)
 {
@@ -2314,24 +2338,22 @@ This is what happens in interactive use with M-x.  */)
   Lisp_Object encoded_file, encoded_newname, symlink_target;
   int dirp = -1;
 
-  symlink_target = encoded_file = encoded_newname = Qnil;
-  CHECK_STRING (file);
-  CHECK_STRING (newname);
   file = Fexpand_file_name (file, Qnil);
 
-  if ((!NILP (Ffile_directory_p (newname)))
-      /* If the filesystem is case-insensitive and the file names are
-	 identical but for the case, don't attempt to move directory
-	 to itself.  */
-      && (NILP (Ffile_name_case_insensitive_p (file))
-	  || NILP (Fstring_equal (Fdowncase (file), Fdowncase (newname)))))
+  /* If the filesystem is case-insensitive and the file names are
+     identical but for case, treat it as a change-case request, and do
+     not worry whether NEWNAME exists or whether it is a directory, as
+     it is already another name for FILE.  */
+  bool case_only_rename = false;
+  if (!NILP (Ffile_name_case_insensitive_p (file)))
     {
-      dirp = !NILP (Ffile_directory_p (file));
-      Lisp_Object fname = dirp ? Fdirectory_file_name (file) : file;
-      newname = Fexpand_file_name (Ffile_name_nondirectory (fname), newname);
+      newname = Fexpand_file_name (newname, Qnil);
+      case_only_rename = !NILP (Fstring_equal (Fdowncase (file),
+					       Fdowncase (newname)));
     }
-  else
-    newname = Fexpand_file_name (newname, Qnil);
+
+  if (!case_only_rename)
+    newname = expand_cp_target (Fdirectory_file_name (file), newname);
 
   /* If the file name has special constructs in it,
      call the corresponding file handler.  */
@@ -2345,15 +2367,9 @@ This is what happens in interactive use with M-x.  */)
   encoded_file = ENCODE_FILE (file);
   encoded_newname = ENCODE_FILE (newname);
 
-  /* If the filesystem is case-insensitive and the file names are
-     identical but for the case, don't worry whether the destination
-     already exists: the caller simply wants to change the letter-case
-     of the file name.  */
-  bool plain_rename
-    = ((!NILP (ok_if_already_exists) && !INTEGERP (ok_if_already_exists))
-       || (file_name_case_insensitive_p (SSDATA (encoded_file))
-	   && ! NILP (Fstring_equal (Fdowncase (file), Fdowncase (newname)))));
-
+  bool plain_rename = (case_only_rename
+		       || (!NILP (ok_if_already_exists)
+			   && !INTEGERP (ok_if_already_exists)));
   int rename_errno;
   if (!plain_rename)
     {
@@ -2395,7 +2411,7 @@ This is what happens in interactive use with M-x.  */)
   else
     {
       if (dirp < 0)
-	dirp = !NILP (Ffile_directory_p (file));
+	dirp = directory_like (file);
       if (dirp)
 	call4 (Qcopy_directory, file, newname, Qt, Qnil);
       else
@@ -2414,24 +2430,17 @@ This is what happens in interactive use with M-x.  */)
 DEFUN ("add-name-to-file", Fadd_name_to_file, Sadd_name_to_file, 2, 3,
        "fAdd name to file: \nGName to add to %s: \np",
        doc: /* Give FILE additional name NEWNAME.  Both args must be strings.
-Signals a `file-already-exists' error if a file NEWNAME already exists
+Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-A number as third arg means request confirmation if NEWNAME already exists.
+An integer third arg means request confirmation if NEWNAME already exists.
 This is what happens in interactive use with M-x.  */)
   (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exists)
 {
   Lisp_Object handler;
   Lisp_Object encoded_file, encoded_newname;
 
-  encoded_file = encoded_newname = Qnil;
-  CHECK_STRING (file);
-  CHECK_STRING (newname);
   file = Fexpand_file_name (file, Qnil);
-
-  if (!NILP (Ffile_directory_p (newname)))
-    newname = Fexpand_file_name (Ffile_name_nondirectory (file), newname);
-  else
-    newname = Fexpand_file_name (newname, Qnil);
+  newname = expand_cp_target (file, newname);
 
   /* If the file name has special constructs in it,
      call the corresponding file handler.  */
@@ -2471,28 +2480,23 @@ DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
        "FMake symbolic link to file: \nGMake symbolic link to file %s: \np",
        doc: /* Make a symbolic link to TARGET, named LINKNAME.
 Both args must be strings.
-Signals a `file-already-exists' error if a file LINKNAME already exists
+Signal a `file-already-exists' error if a file LINKNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-A number as third arg means request confirmation if LINKNAME already exists.
+An integer third arg means request confirmation if LINKNAME already exists.
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
 {
   Lisp_Object handler;
   Lisp_Object encoded_target, encoded_linkname;
 
-  encoded_target = encoded_linkname = Qnil;
   CHECK_STRING (target);
-  CHECK_STRING (linkname);
   /* If the link target has a ~, we must expand it to get
      a truly valid file name.  Otherwise, do not expand;
      we want to permit links to relative file names.  */
   if (SREF (target, 0) == '~')
     target = Fexpand_file_name (target, Qnil);
 
-  if (!NILP (Ffile_directory_p (linkname)))
-    linkname = Fexpand_file_name (Ffile_name_nondirectory (target), linkname);
-  else
-    linkname = Fexpand_file_name (linkname, Qnil);
+  linkname = expand_cp_target (target, linkname);
 
   /* If the file name has special constructs in it,
      call the corresponding file handler.  */
@@ -5577,7 +5581,7 @@ and are changed since last auto-saved.
 Auto-saving writes the buffer into a file
 so that your editing is not lost if the system crashes.
 This file is not the file you visited; that changes only when you save.
-Normally we run the normal hook `auto-save-hook' before saving.
+Normally, run the normal hook `auto-save-hook' before saving.
 
 A non-nil NO-MESSAGE argument means do not print any message if successful.
 A non-nil CURRENT-ONLY argument means save only current buffer.  */)
@@ -6111,7 +6115,7 @@ This applies only to the operation `inhibit-file-name-operation'.  */);
   Vinhibit_file_name_operation = Qnil;
 
   DEFVAR_LISP ("auto-save-list-file-name", Vauto_save_list_file_name,
-	       doc: /* File name in which we write a list of all auto save file names.
+	       doc: /* File name in which to write a list of all auto save file names.
 This variable is initialized automatically from `auto-save-list-file-prefix'
 shortly after Emacs reads your init file, if you have not yet given it
 a non-nil value.  */);
@@ -6166,6 +6170,7 @@ This includes interactive calls to `delete-file' and
   defsubr (&Sfile_name_nondirectory);
   defsubr (&Sunhandled_file_name_directory);
   defsubr (&Sfile_name_as_directory);
+  defsubr (&Sdirectory_name_p);
   defsubr (&Sdirectory_file_name);
   defsubr (&Smake_temp_name);
   defsubr (&Sexpand_file_name);
-- 
2.7.4

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

Paul Eggert wrote:
> there are races on GNU/Linux which can lead to potential security problems. 
> Perhaps we can't fix these races on MS-Windows but we should be able to fix them 
> on a GNUish host. However, we will need to change the semantics of rename-file 
> etc. slightly, since no single system call supports the cp-like target rewriting 
> of these functions. I have a fix in mind to do that in a hopefully 
> compatible-enough way, which I'll try to propose soon. I'll keep 
> case-insensitive file systems in mind when I do that.

Attached is a proposed patch to fix this security problem. If I understand 
things correctly, the fix should work on MS-Windows and on case-insensitive file 
systems. Since this patch entails an incompatible change to the (undocumented) 
behavior of (rename-file A B) when B is a directory but is not a directory name, 
I'll mention the proposed change on emacs-devel.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-race-with-rename-file-etc.-with-dir-NEWNAME.patch --]
[-- Type: text/x-patch; name="0001-Fix-race-with-rename-file-etc.-with-dir-NEWNAME.patch", Size: 11765 bytes --]

From 967d1c009f6743aa75e6829129ea9445ebc06f30 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 13 Aug 2017 15:20:56 -0700
Subject: [PROPOSED] Fix race with rename-file etc. with dir NEWNAME

This changes the behavior of rename-file etc. slightly.
The old behavior mostly disagreed with the documentation, and had
a race condition bug that could allow attackers to modify victims'
write-protected directories.
* doc/lispref/files.texi (Changing Files): Document that in
rename-file etc., NEWFILE is special if it is a directory name.
* etc/NEWS: Document the change in behavior.
* src/fileio.c (directory_like): Remove.  All uses removed.
(expand_cp_target): Test only whether NEWNAME is a directory name,
not whether it is currently a directory.  This avoids a race.
(Fcopy_file, Frename_file, Fadd_name_to_file, Fmake_symbolic_link):
Document behavior if NEWNAME is a directory name.
(Frename_file): Simplify now that the destdir behavior occurs
only when NEWNAME is a directory name.
* test/lisp/net/tramp-tests.el (tramp-test11-copy-file)
(tramp-test12-rename-file, tramp--test-check-files):
Adjust tests to match new behavior.
---
 doc/lispref/files.texi       | 10 +++++---
 etc/NEWS                     | 13 +++++++++++
 src/fileio.c                 | 55 ++++++++++++++++++++++----------------------
 test/lisp/net/tramp-tests.el | 16 ++++++-------
 4 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 25f32c231c..8956c4f1b7 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1553,9 +1553,13 @@ Changing Files
 made by these functions instead of writing them immediately to
 secondary storage.  @xref{Files and Storage}.
 
-  In the functions that have an argument @var{newname}, if a file by the
-name of @var{newname} already exists, the actions taken depend on the
-value of the argument @var{ok-if-already-exists}:
+  In the functions that have an argument @var{newname}, if
+this argument is a directory name it is treated as if the nondirectory
+part of the source name were appended. For example, if the old name is
+@code{"a/b/c"}, the @var{newname} @code{"d/e/f/"} is treated as if it
+were @code{"d/e/f/c"}.  Furthermore, if a file already exists with the
+new name, the actions taken depend on the value of the argument
+@var{ok-if-already-exists}:
 
 @itemize @bullet
 @item
diff --git a/etc/NEWS b/etc/NEWS
index 3f38153048..947114148b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1204,6 +1204,19 @@ break.
 ** The arguments LOCKNAME and MUSTBENEW of 'write-region' are
 propagated to file name handlers now.
 
++++
+** When the NEWNAME argument of 'copy-file', 'rename-file',
+'add-name-to-file', or 'make-symbolic-link' is an existing directory
+and the intent is to act on an entry in that directory, NEWNAME should
+now be a directory name, e.g., (rename-file "e" "f/") renames to 'f/e'.
+Although this formerly happened sometimes even when NEWNAME was not a
+directory name, as in (rename-file "e" "f") where 'f' happened to be a
+directory, the old behavior had inherent races that led to security
+holes, and disagreed with longstanding documentation.  A call like
+(rename-file A B) that used the old, undocumented behavior can be
+written as (rename-file A (file-name-as-directory B)), a formulation
+portable to both older and newer versions of Emacs.
+
 \f
 * Lisp Changes in Emacs 26.1
 
diff --git a/src/fileio.c b/src/fileio.c
index 69079c6ae4..4bfa915951 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -594,24 +594,16 @@ DEFUN ("directory-name-p", Fdirectory_name_p, Sdirectory_name_p, 1, 1, 0,
   return IS_DIRECTORY_SEP (c) ? Qt : Qnil;
 }
 
-/* Return true if NAME must be that of a directory if it exists.
-   When NAME is a directory name, this avoids system calls compared to
-   just calling Ffile_directory_p.  */
-
-static bool
-directory_like (Lisp_Object name)
-{
-  return !NILP (Fdirectory_name_p (name)) || !NILP (Ffile_directory_p (name));
-}
-
-/* Return the expansion of NEWNAME, except that if NEWNAME is like a
-   directory then return the expansion of FILE's basename under
-   NEWNAME.  This is like how 'cp FILE NEWNAME' works.  */
+/* Return the expansion of NEWNAME, except that if NEWNAME is a
+   directory name then return the expansion of FILE's basename under
+   NEWNAME.  This resembles how 'cp FILE NEWNAME' works, except that
+   it requires NEWNAME to be a directory name (typically, by ending in
+   "/").  */
 
 static Lisp_Object
 expand_cp_target (Lisp_Object file, Lisp_Object newname)
 {
-  return (directory_like (newname)
+  return (!NILP (Fdirectory_name_p (newname))
 	  ? Fexpand_file_name (Ffile_name_nondirectory (file), newname)
 	  : Fexpand_file_name (newname, Qnil));
 }
@@ -1818,7 +1810,8 @@ clone_file (int dest, int source)
 DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 6,
        "fCopy file: \nGCopy %s to file: \np\nP",
        doc: /* Copy FILE to NEWNAME.  Both args must be strings.
-If NEWNAME names a directory, copy FILE there.
+If NEWNAME is a directory name, copy FILE to a like-named file under
+NEWNAME.
 
 This function always sets the file modes of the output file to match
 the input file.
@@ -2242,6 +2235,9 @@ DEFUN ("rename-file", Frename_file, Srename_file, 2, 3,
        "fRename file: \nGRename %s to file: \np",
        doc: /* Rename FILE as NEWNAME.  Both args must be strings.
 If file has names other than FILE, it continues to have those names.
+If NEWNAME is a directory name, rename FILE to a like-named file under
+NEWNAME.
+
 Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
 An integer third arg means request confirmation if NEWNAME already exists.
@@ -2250,7 +2246,6 @@ This is what happens in interactive use with M-x.  */)
 {
   Lisp_Object handler;
   Lisp_Object encoded_file, encoded_newname, symlink_target;
-  int dirp = -1;
 
   file = Fexpand_file_name (file, Qnil);
 
@@ -2319,22 +2314,21 @@ This is what happens in interactive use with M-x.  */)
   if (rename_errno != EXDEV)
     report_file_errno ("Renaming", list2 (file, newname), rename_errno);
 
-  symlink_target = Ffile_symlink_p (file);
-  if (!NILP (symlink_target))
-    Fmake_symbolic_link (symlink_target, newname, ok_if_already_exists);
+  bool dirp = !NILP (Fdirectory_name_p (file));
+  if (dirp)
+    call4 (Qcopy_directory, file, newname, Qt, Qnil);
   else
     {
-      if (dirp < 0)
-	dirp = directory_like (file);
-      if (dirp)
-	call4 (Qcopy_directory, file, newname, Qt, Qnil);
+      symlink_target = Ffile_symlink_p (file);
+      if (!NILP (symlink_target))
+	Fmake_symbolic_link (symlink_target, newname, ok_if_already_exists);
       else
 	Fcopy_file (file, newname, ok_if_already_exists, Qt, Qt, Qt);
     }
 
   ptrdiff_t count = SPECPDL_INDEX ();
   specbind (Qdelete_by_moving_to_trash, Qnil);
-  if (dirp && NILP (symlink_target))
+  if (dirp)
     call2 (Qdelete_directory, file, Qt);
   else
     Fdelete_file (file, Qnil);
@@ -2344,6 +2338,9 @@ This is what happens in interactive use with M-x.  */)
 DEFUN ("add-name-to-file", Fadd_name_to_file, Sadd_name_to_file, 2, 3,
        "fAdd name to file: \nGName to add to %s: \np",
        doc: /* Give FILE additional name NEWNAME.  Both args must be strings.
+If NEWNAME is a directory name, give FILE a like-named new name under
+NEWNAME.
+
 Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
 An integer third arg means request confirmation if NEWNAME already exists.
@@ -2392,11 +2389,13 @@ This is what happens in interactive use with M-x.  */)
 
 DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
        "FMake symbolic link to file: \nGMake symbolic link to file %s: \np",
-       doc: /* Make a symbolic link to TARGET, named LINKNAME.
-Both args must be strings.
-Signal a `file-already-exists' error if a file LINKNAME already exists
+       doc: /* Make a symbolic link to TARGET, named NEWNAME.
+If NEWNAME is a directory name, make a like-named symbolic link under
+NEWNAME.
+
+Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-An integer third arg means request confirmation if LINKNAME already exists.
+An integer third arg means request confirmation if NEWNAME already exists.
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
 {
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index 45cf95fcfe..6aca0a278e 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -1892,7 +1892,7 @@ tramp--test-backtrace
 	    (should-error (copy-file tmp-name1 tmp-name2))
 	    (copy-file tmp-name1 tmp-name2 'ok)
 	    (make-directory tmp-name3)
-	    (copy-file tmp-name1 tmp-name3)
+	    (copy-file tmp-name1 (file-name-as-directory tmp-name3))
 	    (should
 	     (file-exists-p
 	      (expand-file-name (file-name-nondirectory tmp-name1) tmp-name3))))
@@ -1914,7 +1914,7 @@ tramp--test-backtrace
 	    (should-error (copy-file tmp-name1 tmp-name4))
 	    (copy-file tmp-name1 tmp-name4 'ok)
 	    (make-directory tmp-name5)
-	    (copy-file tmp-name1 tmp-name5)
+	    (copy-file tmp-name1 (file-name-as-directory tmp-name5))
 	    (should
 	     (file-exists-p
 	      (expand-file-name (file-name-nondirectory tmp-name1) tmp-name5))))
@@ -1936,7 +1936,7 @@ tramp--test-backtrace
 	    (should-error (copy-file tmp-name4 tmp-name1))
 	    (copy-file tmp-name4 tmp-name1 'ok)
 	    (make-directory tmp-name3)
-	    (copy-file tmp-name4 tmp-name3)
+	    (copy-file tmp-name4 (file-name-as-directory tmp-name3))
 	    (should
 	     (file-exists-p
 	      (expand-file-name (file-name-nondirectory tmp-name4) tmp-name3))))
@@ -1975,7 +1975,7 @@ tramp--test-backtrace
 	    (should-not (file-exists-p tmp-name1))
 	    (write-region "foo" nil tmp-name1)
 	    (make-directory tmp-name3)
-	    (rename-file tmp-name1 tmp-name3)
+	    (rename-file tmp-name1 (file-name-as-directory tmp-name3))
 	    (should-not (file-exists-p tmp-name1))
 	    (should
 	     (file-exists-p
@@ -2002,7 +2002,7 @@ tramp--test-backtrace
 	    (should-not (file-exists-p tmp-name1))
 	    (write-region "foo" nil tmp-name1)
 	    (make-directory tmp-name5)
-	    (rename-file tmp-name1 tmp-name5)
+	    (rename-file tmp-name1 (file-name-as-directory tmp-name5))
 	    (should-not (file-exists-p tmp-name1))
 	    (should
 	     (file-exists-p
@@ -2029,7 +2029,7 @@ tramp--test-backtrace
 	    (should-not (file-exists-p tmp-name4))
 	    (write-region "foo" nil tmp-name4 nil 'nomessage)
 	    (make-directory tmp-name3)
-	    (rename-file tmp-name4 tmp-name3)
+	    (rename-file tmp-name4 (file-name-as-directory tmp-name3))
 	    (should-not (file-exists-p tmp-name4))
 	    (should
 	     (file-exists-p
@@ -3464,11 +3464,11 @@ tramp--test-check-files
 		  (should (string-equal (buffer-string) elt)))
 
 		;; Copy file both directions.
-		(copy-file file1 tmp-name2)
+		(copy-file file1 (file-name-as-directory tmp-name2))
 		(should (file-exists-p file2))
 		(delete-file file1)
 		(should-not (file-exists-p file1))
-		(copy-file file2 tmp-name1)
+		(copy-file file2 (file-name-as-directory tmp-name1))
 		(should (file-exists-p file1))
 
 		;; Method "smb" supports `make-symbolic-link' only if the
-- 
2.13.4

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

Getting back to Philipp's original bug report, Apple documentation says macOS 
has a facility like the Linux renameat2 system call (i.e., it's like 'renameat' 
except it can be told to fail if the destination already exists). Attached is a 
proposed patch to use this facility, which means that the case-insensitivity 
test would no longer need to be done in macOS. If there's some way to implement 
renameat_noreplace on MS-Windows we could get rid of the case-insensitivity test 
there too.

I don't have easy access to macOS so I have not installed this patch. It'd be 
nice, Philipp, if you could try it out.

This patch is independent of the destination-directory patch that I recently 
proposed in:

https://bugs.gnu.org/27986#14

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-rename-file-behavior-on-macOS.patch --]
[-- Type: text/x-patch; name="0001-Improve-rename-file-behavior-on-macOS.patch", Size: 1777 bytes --]

From 62605066c4d5a7b91c7800fcc300dbe5082426dc Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 13 Aug 2017 16:31:08 -0700
Subject: [PATCH] Improve rename-file behavior on macOS

Problem reported by Philipp Stephani (Bug#27986).
* src/fileio.c (Frename_file):
Worry about file name case sensitivity only if WINDOWSNT.
* src/sysdep.c (renameat_noreplace): Use renameatx_np on macOS,
since this provides the necessary atomicity guarantees.
---
 src/fileio.c | 2 ++
 src/sysdep.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/src/fileio.c b/src/fileio.c
index 69079c6ae4..ee70dc6e69 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2259,12 +2259,14 @@ This is what happens in interactive use with M-x.  */)
      not worry whether NEWNAME exists or whether it is a directory, as
      it is already another name for FILE.  */
   bool case_only_rename = false;
+#ifdef WINDOWSNT
   if (!NILP (Ffile_name_case_insensitive_p (file)))
     {
       newname = Fexpand_file_name (newname, Qnil);
       case_only_rename = !NILP (Fstring_equal (Fdowncase (file),
 					       Fdowncase (newname)));
     }
+#endif
 
   if (!case_only_rename)
     newname = expand_cp_target (Fdirectory_file_name (file), newname);
diff --git a/src/sysdep.c b/src/sysdep.c
index 9eb733221e..4e98f1db24 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -2693,6 +2693,8 @@ renameat_noreplace (int srcfd, char const *src, int dstfd, char const *dst)
 {
 #if defined SYS_renameat2 && defined RENAME_NOREPLACE
   return syscall (SYS_renameat2, srcfd, src, dstfd, dst, RENAME_NOREPLACE);
+#elif defined RENAME_EXCL
+  return renameatx_np (srcfd, src, dstfd, dst, RENAME_EXCL);
 #else
   errno = ENOSYS;
   return -1;
-- 
2.13.4

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Ken Brown]

On 8/13/2017 7:48 PM, Paul Eggert wrote:
> Getting back to Philipp's original bug report, Apple documentation says 
> macOS has a facility like the Linux renameat2 system call (i.e., it's 
> like 'renameat' except it can be told to fail if the destination already 
> exists). Attached is a proposed patch to use this facility, which means 
> that the case-insensitivity test would no longer need to be done in 
> macOS. If there's some way to implement renameat_noreplace on MS-Windows 
> we could get rid of the case-insensitivity test there too.

I think we still need the case-insensitivity test on Cygwin as well as 
on MS-Windows.

Ken

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Ken Brown <kbrown@cornell.edu>
> Date: Mon, 14 Aug 2017 09:44:54 -0400
> Cc: 27986@debbugs.gnu.org
> 
> I think we still need the case-insensitivity test on Cygwin as well as 
> on MS-Windows.

Yes.  But I think there are more general issues with this, which I
will describe separately.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Eli Zaretskii <eliz@gnu.org>, 27986@debbugs.gnu.org
> Date: Sun, 13 Aug 2017 16:48:59 -0700
> 
> Getting back to Philipp's original bug report, Apple documentation says macOS 
> has a facility like the Linux renameat2 system call (i.e., it's like 'renameat' 
> except it can be told to fail if the destination already exists). Attached is a 
> proposed patch to use this facility, which means that the case-insensitivity 
> test would no longer need to be done in macOS. If there's some way to implement 
> renameat_noreplace on MS-Windows we could get rid of the case-insensitivity test 
> there too.

There's nothing easier than implementing renameat_noreplace on
MS-Windows, since the underlying system call does that by default, and
it's emulating the Posix behavior that requires complications.  In
fact, we already have this implementation: see sys_rename_replace (in
w32.c) which needs to be called with its last argument FALSE (modulo
the "at" part, which is easily handled).

(Ken, what about Cygwin?)

So I think we may be able to remove the case-sensitivity check right
now.  And in any case, that check should be in
barf_or_query_if_file_exists anyway, because renameat_noreplace on
case-insensitive filesystems already supports renaming to a different
letter-case.  The reason we had this check before is because we didn't
employ renameat_noreplace, but called 'rename' right away, and that
needed a question before overwriting the target.  But now
renameat_noreplace will silently succeed to rename when the user wants
to only change the letter-case, so no such check is needed.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Philipp <p.stephani2@gmail.com>, 27986@debbugs.gnu.org
> Date: Sun, 13 Aug 2017 15:42:05 -0700
> 
> Paul Eggert wrote:
> > there are races on GNU/Linux which can lead to potential security problems. 
> > Perhaps we can't fix these races on MS-Windows but we should be able to fix them 
> > on a GNUish host.

For the record: 'rename' is atomic on Windows when the target doesn't
exist.  It's the case when the target exists that cannot be guaranteed
to be handled atomically, AFAIU, because deleting the old target and
renaming are not necessarily an atomic operation on Windows.  I don't
know if this is an issue, since the current code in rename-file uses 2
separate system calls in this case anyway, even on Posix platforms.

> Attached is a proposed patch to fix this security problem. If I understand 
> things correctly, the fix should work on MS-Windows and on case-insensitive file 
> systems. Since this patch entails an incompatible change to the (undocumented) 
> behavior of (rename-file A B) when B is a directory but is not a directory name, 
> I'll mention the proposed change on emacs-devel.

I'm uneasy, to say the least, to change the semantic of such a veteran
behavior.  Could you please take a step back and elaborate on the
races and the security problems related to this, and why the change in
the semantics you propose is the solution?  I'd like to understand the
problem better before we decide on the solution.

Thanks.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Date: Mon, 14 Aug 2017 18:34:30 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> 
> There's nothing easier than implementing renameat_noreplace on
> MS-Windows, since the underlying system call does that by default, and
> it's emulating the Posix behavior that requires complications.  In
> fact, we already have this implementation: see sys_rename_replace (in
> w32.c) which needs to be called with its last argument FALSE (modulo
> the "at" part, which is easily handled).

Which I just did.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

Paul Eggert <eggert@cs.ucla.edu> schrieb am Mo., 14. Aug. 2017 um 01:49 Uhr:

> Getting back to Philipp's original bug report, Apple documentation says
> macOS
> has a facility like the Linux renameat2 system call (i.e., it's like
> 'renameat'
> except it can be told to fail if the destination already exists). Attached
> is a
> proposed patch to use this facility, which means that the
> case-insensitivity
> test would no longer need to be done in macOS. If there's some way to
> implement
> renameat_noreplace on MS-Windows we could get rid of the
> case-insensitivity test
> there too.
>
> I don't have easy access to macOS so I have not installed this patch. It'd
> be
> nice, Philipp, if you could try it out.
>
>
Thanks, the patch fixes the problem. However, it's still not 100% correct:
now casing changes such as from "A" to "a" where macOS treats the file
names as equivalent trigger the "file already exists" signal as well. I
don't think that can be fixed, though; there's no way to special-case
casing changes while keeping atomicity intact. So I'd rather have Emacs
react conservatively and skip the casing check entirely.
Note that the manpage says:

RENAME_EXCL   On file systems that support it (see getattrlist(2)
VOL_CAP_INT_RENAME_EXCL), it will cause EEXIST to be returned if the
destination already exists.

I interpret this such that if the filesystem doesn't support RENAME_EXCL
the rename will succeed even if the destination exists.

Since we probably won't be able to solve all issues across operating
systems and filesystems, probably we should have at least a warning in the
documentation that rename-file attempts to be race-free and atomic, but
only on a best-effort basis.

[-- Attachment #2: Type: text/html, Size: 2333 bytes --]

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Mo., 14. Aug. 2017 um 17:34 Uhr:

> > From: Paul Eggert <eggert@cs.ucla.edu>
> > Cc: Eli Zaretskii <eliz@gnu.org>, 27986@debbugs.gnu.org
> > Date: Sun, 13 Aug 2017 16:48:59 -0700
> >
> > Getting back to Philipp's original bug report, Apple documentation says
> macOS
> > has a facility like the Linux renameat2 system call (i.e., it's like
> 'renameat'
> > except it can be told to fail if the destination already exists).
> Attached is a
> > proposed patch to use this facility, which means that the
> case-insensitivity
> > test would no longer need to be done in macOS. If there's some way to
> implement
> > renameat_noreplace on MS-Windows we could get rid of the
> case-insensitivity test
> > there too.
>
> There's nothing easier than implementing renameat_noreplace on
> MS-Windows, since the underlying system call does that by default, and
> it's emulating the Posix behavior that requires complications.
>

You might be able to eliminate these complications by calling MoveFileExW
with MOVEFILE_REPLACE_EXISTING.

[-- Attachment #2: Type: text/html, Size: 1597 bytes --]

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Mon, 14 Aug 2017 16:58:04 +0000
> Cc: 27986@debbugs.gnu.org
> 
>  There's nothing easier than implementing renameat_noreplace on
>  MS-Windows, since the underlying system call does that by default, and
>  it's emulating the Posix behavior that requires complications.
> 
> You might be able to eliminate these complications by calling MoveFileExW with
> MOVEFILE_REPLACE_EXISTING.

Thanks, but I see no need, because using that function doesn't solve
any problems we have (it isn't guaranteed to be atomic if the
destination exists), and adds new complexity (because it won't move
files across volumes, and because it didn't exist before XP).  The
"complications" I mentioned above are already implemented and well
tested, so I see no point in eliminating them.

bug#27986: 26.0.50; `rename-file' can rename files without confirmation

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am So., 6. Aug. 2017 um 19:05 Uhr:

> > From: Philipp <p.stephani2@gmail.com>
> > Date: Sun, 06 Aug 2017 17:40:18 +0200
> >
> > (rename-file "/tmp/emacs/ẞ" "/tmp/emacs/ß")
> > nil
> >
> > Note how `rename-file' has silently overwritten `ß'.  This is because on
> > macOS, `ß' and `ẞ' are different file names, but Emacs treats them as
> > equal.  Probably the test for case-insensitive file names should be
> > removed altogether
>
> Which one? there are two of them.
>

I guess all of them where correctness would depend on the outcome.


>
> > (it can't work correctly and introduces a filesystem race)
>
> It cannot work correctly _because_ of a possible race or because of
> some other reasons?  If the latter, please elaborate.


As this example shows, there are cases where two case-insensitive filenames
are considered equivalent by Emacs, but different by the actual filesystem.
This is unavoidable, because the definition of "case-insensitive" changes
all the time, both in Emacs and in the filesystems. Generally it's
impossible to detect whether two filenames would refer to the same file
without actually creating the file. And even then the answer depends on how
the file is created, see e.g. FILE_FLAG_POSIX_SEMANTICS. So Emacs can't
compare filenames and make decisions based on the result upon which the
correctness of a critical function depends. Comparing filenames can still
be OK for best-effort attempts at giving the user better error messages or
similar.


> If the former,
> then at least on MS-Windows we have a race anyway, because the
> underlying system APIs are not atomic.


Wouldn't MoveFileExW with MOVE_FILE_REPLACE_EXISTING be atomic?

> and `rename-file' should use link(2) + unlink(2) if renameat2
> > isn't available.
>
> 'link' and 'unlink' accept strings as arguments, not integer numbers
> such as 2.
>

Yes, I mean the functions described in section 2 of the man page. link(2)
is a common markup for this.


>
> More to the point, how can this strategy work on a case-insensitive
> filesystem?  What am I missing?
>

IIUC link(2) + unlink(2) would, if successful, guarantee enough atomicity
in the sense that the old file is now guaranteed to be the new file, and
the call is guaranteed to fail if the new file already exists. I don't
think anything can help with the case-changing problem; I think we just
have to live with an occasional false positive signal in this case.

[-- Attachment #2: Type: text/html, Size: 3602 bytes --]

bug#27986: 26.0.50; `rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Mon, 14 Aug 2017 17:09:35 +0000
> Cc: 27986@debbugs.gnu.org
> 
>  > Note how `rename-file' has silently overwritten `ß'. This is because on
>  > macOS, `ß' and `ẞ' are different file names, but Emacs treats them as
>  > equal. Probably the test for case-insensitive file names should be
>  > removed altogether
> 
>  Which one? there are two of them.
> 
> I guess all of them where correctness would depend on the outcome.

But then we lose a feature which just a few releases ago was deemed
valuable enough to have.

> As this example shows, there are cases where two case-insensitive filenames are considered equivalent by
> Emacs, but different by the actual filesystem. This is unavoidable, because the definition of "case-insensitive"
> changes all the time, both in Emacs and in the filesystems. Generally it's impossible to detect whether two
> filenames would refer to the same file without actually creating the file.

Wouldn't trying to rename it actually tell you that, by failing with EEXIST?
(On Windows, it simply succeeds and changes the letter-case silently,
but I understand that is not what happens on macOS.)

>  If the former,
>  then at least on MS-Windows we have a race anyway, because the
>  underlying system APIs are not atomic.
> 
> Wouldn't MoveFileExW with MOVE_FILE_REPLACE_EXISTING be atomic?

No, it isn't guaranteed to be atomic.  No documentation says that,
certainly not any official documentation.  If I had access to the
sources, I could have looked there, but I don't.  Failing that, my
assumption is that it isn't atomic, because meta-data of more than one
file needs to be touched in this case.

> Yes, I mean the functions described in section 2 of the man page. link(2) is a common markup for this.

My point was that GNU coding standards frown on use of such markup.

>  More to the point, how can this strategy work on a case-insensitive
>  filesystem? What am I missing?
> 
> IIUC link(2) + unlink(2) would, if successful, guarantee enough atomicity in the sense that the old file is now
> guaranteed to be the new file, and the call is guaranteed to fail if the new file already exists.

I actually think that if the old and the new name are equal but for
the letter-case, and the filesystem is case-insensitive, doing that
will delete the file, so you are left with nothing.

> I don't think anything can help with the case-changing problem; I
> think we just have to live with an occasional false positive signal
> in this case.

That'd be an unfortunate regression, IMO.  It isn't right for us to
back out changes we just introduced, which were considered important
when we did that.  We ought to find a solution that doesn't remove
this feature, not even on macOS.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

Philipp Stephani wrote:

> there's no way to special-case
> casing changes while keeping atomicity intact. So I'd rather have Emacs
> react conservatively and skip the casing check entirely.

Yes, that makes sense, at least for macOS.

> Note that the manpage says:
> 
> RENAME_EXCL   On file systems that support it (see getattrlist(2)
> VOL_CAP_INT_RENAME_EXCL), it will cause EEXIST to be returned if the
> destination already exists.
> 
> I interpret this such that if the filesystem doesn't support RENAME_EXCL
> the rename will succeed even if the destination exists.

I interpret it to mean that if the filesystem doesn't support RENAME_EXCL, the 
rename will fail with errno == EINVAL or ENOSYS.  At least, that's how it works 
under GNU/Linux. If it behaved the way you suggest, there'd be no good way to 
emulate RENAME_EXCL on filesystems lacking it, which would surely not be Apple's 
intent.

Please check this, though. I installed the patch on 'master' to help you do that.

Now that renameat_noreplace works on DOS_NT, would it make sense to apply the 
attached further patch as well? If we can get renameat_noreplace to work on 
Cygwin the we could simplify the fileio.c code even further.

> Since we probably won't be able to solve all issues across operating
> systems and filesystems, probably we should have at least a warning in the
> documentation that rename-file attempts to be race-free and atomic, but
> only on a best-effort basis.

True. I'd like to get the directory issue fixed before worrying about this, 
though. (That way I don't have to document the security holes in the current 
implementation. :-) I'll follow up separately about that.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-rename-file-behavior-on-DOS_NT.patch --]
[-- Type: text/x-patch; name="0001-Improve-rename-file-behavior-on-DOS_NT.patch", Size: 1493 bytes --]

From c929fb0ed882f15e492caff9f6610c87a57bca9a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 14 Aug 2017 15:59:37 -0700
Subject: [PATCH] Improve rename-file behavior on DOS_NT

See Bug#27986.
* src/fileio.c (Frename_file): Do not worry about file name case
sensitivity if DOS_NT, since renameat_noreplace should work in
that case.
---
 src/fileio.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index 9f6de5b6ca..f5000352ab 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2254,12 +2254,13 @@ This is what happens in interactive use with M-x.  */)
 
   file = Fexpand_file_name (file, Qnil);
 
-  /* If the filesystem is case-insensitive and the file names are
-     identical but for case, treat it as a change-case request, and do
-     not worry whether NEWNAME exists or whether it is a directory, as
-     it is already another name for FILE.  */
+  /* If renameat_noreplace does not work and the filesystem is
+     case-insensitive and the file names are identical but for case,
+     treat it as a change-case request, and do not worry whether
+     NEWNAME exists or whether it is a directory, as it is already
+     another name for FILE.  */
   bool case_only_rename = false;
-#if defined CYGWIN || defined DOS_NT
+#ifdef CYGWIN
   if (!NILP (Ffile_name_case_insensitive_p (file)))
     {
       newname = Fexpand_file_name (newname, Qnil);
-- 
2.13.5

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

Eli Zaretskii wrote:
> Could you please take a step back and elaborate on the
> races and the security problems related to this, and why the change in
> the semantics you propose is the solution?

Currently (rename-file A B) requires at least two system calls to work: one to 
test whether B is a directory, and the other to actually do the rename. This 
leads to race conditions if other actors change the file system between the two 
calls.

For example, suppose a victim is about to execute (rename-file "/tmp/foo" 
"/tmp/bar" t), and suppose an attacker wants to destroy the victim's file 
/home/victim/secret/foo. The attacker can do (make-symbolic-link 
"/home/victim/secret" "/tmp/bar"), and this will cause the victim to lose all 
the data in /home/victim/secret/bar even though the attacker is supposed to lack 
access to anything under /home/victim/secret. I doubt whether this is the only 
such scenario; it's just the first one that popped into my mind.

As icing on the cake, the current behavior of (rename-file A B) disagrees with 
its documentation when B is an existing directory.

There is no good solution to this problem. All solutions are bad, in that either 
they are not 100% backward compatible with existing behavior, or they continue 
to encourage insecure Elisp code. The proposed patch attempts to choose the 
least bad way forward, by making the default behavior more secure, at a 
relatively minor cost in compatibility. Most uses of rename-file etc. won't care 
about the change, and the ones that do care are likely to have security problems 
anyway.

The proposed solution improves security, because a common pattern in Lisp code 
when creating a file BAR "atomically" is to create and write a temporary file 
FOO and then execute (rename-file FOO BAR). Currently, this approach can be 
attacked in the way described when BAR's parent directory is /tmp or any similar 
directory. With the proposed patch, this approach cannot be hijacked in this 
way, because BAR will be a file name and not a directory name. That is, the call 
to rename-file will specify whether the destination-directory semantics are 
desired, rather than relying on the state of the filesystem to specify it. This 
is more secure because the state of the filesystem is partially under control of 
attackers.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 223 bytes --]

Paul Eggert wrote:
> I installed the patch on 'master' to help you do that.

After rereading the Apple manual I installed the attached further patch into 
master, so please try updating to the latest master before testing.

[-- Attachment #2: 0001-Improve-rename-file-port-to-macOS.patch --]
[-- Type: text/x-patch, Size: 928 bytes --]

From 97460582e2d0052f27d342ddb90309dc3da700b8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 14 Aug 2017 18:16:04 -0700
Subject: [PATCH] Improve rename-file port to macOS

* src/fileio.c (Frename_file): On macOS, renameat_noreplace can
fail with errno == ENOTSUP on file systems where it is not
supported, according to the Apple documentation.
---
 src/fileio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/fileio.c b/src/fileio.c
index 9f6de5b..e557483 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -2297,7 +2297,7 @@ This is what happens in interactive use with M-x.  */)
       rename_errno = errno;
       switch (rename_errno)
 	{
-	case EEXIST: case EINVAL: case ENOSYS:
+	case EEXIST: case EINVAL: case ENOSYS: case ENOTSUP:
 	  barf_or_query_if_file_exists (newname, rename_errno == EEXIST,
 					"rename to it",
 					INTEGERP (ok_if_already_exists),
-- 
2.7.4

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 14 Aug 2017 16:03:13 -0700
> 
> Philipp Stephani wrote:
> 
> > there's no way to special-case
> > casing changes while keeping atomicity intact. So I'd rather have Emacs
> > react conservatively and skip the casing check entirely.
> 
> Yes, that makes sense, at least for macOS.

As I wrote, we should not lose the feature that renaming a file on
macOS to a different letter-case works without nagging the user with
confirmations.  I don't think this runs afoul of security, since the
letter-case check doesn't break atomicity in any way.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

Eli Zaretskii wrote:
> we should not lose the feature that renaming a file on
> macOS to a different letter-case works without nagging the user with
> confirmations.

I was hoping that the current code does that as-is on macOS. Someone needs to 
test that, though.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Andy Moreton]

On Mon 14 Aug 2017, Paul Eggert wrote:

> Paul Eggert wrote:
>> I installed the patch on 'master' to help you do that.
>
> After rereading the Apple manual I installed the attached further patch into
> master, so please try updating to the latest master before testing.

This patch breaks the Windows build for mingw64:

../../src/fileio.c: In function 'Frename_file':
../../src/fileio.c:2300:41: error: duplicate case value
  case EEXIST: case EINVAL: case ENOSYS: case ENOTSUP:
                                         ^~~~
../../src/fileio.c:2300:28: note: previously used here
  case EEXIST: case EINVAL: case ENOSYS: case ENOTSUP:
                            ^~~~

A brief look suggests that the inclusion of ms-w32.h in conf_post.h is
the culprit, as it defines some error numbers before errno.h is
included, and thus overrides the conditional definitions in errno.h.

    AndyM

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 14 Aug 2017 16:31:38 -0700
> 
> Currently (rename-file A B) requires at least two system calls to work: one to test whether B is a directory, and the other to actually do the rename. This leads to race conditions if other actors change the file system between the two calls.
> 
> For example, suppose a victim is about to execute (rename-file "/tmp/foo" "/tmp/bar" t), and suppose an attacker wants to destroy the victim's file /home/victim/secret/foo. The attacker can do (make-symbolic-link "/home/victim/secret" "/tmp/bar")

You mean, the attacker creates this symlink between the time Emacs
tests whether /tmp/bar is a directory and the time Emacs calls
'rename'?

> and this will cause the victim to lose all the data in /home/victim/secret/bar even though the attacker is supposed to lack access to anything under /home/victim/secret.

You mean, "lose data" because files in /tmp/foo will overwrite their
namesakes in /home/victim/secret/bar?  IOW, files in
/home/victim/secret/bar which don't have identically-named
counterparts in /tmp/foo will not be lost, right?  (I'm not saying
this is not a problem, I'm just trying to understand the meaning of
"lose data" in this case.)

If my understanding of the situation is correct, then such an attack
will still be possible with the proposed change, if /tmp/bar exists,
because Emacs will still issue two separate system calls, and the
symlink can be created in-between, albeit at a price of deleting
/tmp/bar first.  Right?

> As icing on the cake, the current behavior of (rename-file A B) disagrees with its documentation when B is an existing directory.

Well, 2/3 of it.  The 3rd instance, in the Emacs manual gets it right:

  [...]  If the argument NEW is just a directory name, the real new name
  is in that directory, with the same non-directory component as OLD.
  For example, ‘M-x rename-file <RET> ~/foo <RET> /tmp <RET>’ renames
  ‘~/foo’ to ‘/tmp/foo’.

Note that there's no trailing slash in "/tmp".

> There is no good solution to this problem. All solutions are bad, in that either they are not 100% backward compatible with existing behavior, or they continue to encourage insecure Elisp code. The proposed patch attempts to choose the least bad way forward, by making the default behavior more secure, at a relatively minor cost in compatibility. Most uses of rename-file etc. won't care about the change, and the ones that do care are likely to have security problems anyway.

How about eating the cake and having it, too?  We could refrain from
testing whether B is a directory if either (1) B ends in a slash, or
(2) rename_noreplace succeeds.  If B doesn't end in a slash and
rename_noreplace fails, and the value of errno from rename_noreplace
doesn't indicate B is a directory, then test if it's a directory and
fall back on the old behavior.  We could then change our code that
calls rename-file to make B end in a slash, thus encouraging secure
code and leaving the insecure behavior only as backward-compatibility
feature.  This will make the insecure code limited to situation which
are already insecure due to a separate call to rename_noreplace.

> The proposed solution improves security, because a common pattern in Lisp code when creating a file BAR "atomically" is to create and write a temporary file FOO and then execute (rename-file FOO BAR). Currently, this approach can be attacked in the way described when BAR's parent directory is /tmp or any similar directory. With the proposed patch, this approach cannot be hijacked in this way, because BAR will be a file name and not a directory name. That is, the call to rename-file will specify whether the destination-directory semantics are desired, rather than relying on the state of the filesystem to specify it. This is more secure because the state of the filesystem is partially under control of attackers.

What I don't quite understand is what will happen under your proposal
to the calls of the form (rename-file A B) where B names an existing
directory and doesn't end in slash?  Will it fail, sometimes or
always?  AFAIU, the 'rename' call will fail if B is a non-empty
directory, but what if it's empty, and what does rename_noreplace do
in these situations?  Your documentation patches don't cover this use
case; I think we should.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 15 Aug 2017 00:00:49 -0700
> 
> Eli Zaretskii wrote:
> > we should not lose the feature that renaming a file on
> > macOS to a different letter-case works without nagging the user with
> > confirmations.
> 
> I was hoping that the current code does that as-is on macOS.

In that case, I have no problems with the changes.  From what Philipp
wrote I concluded, perhaps wrongly, that this is not happening on
macOS.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Tue, 15 Aug 2017 13:45:02 +0100
> 
> ../../src/fileio.c: In function 'Frename_file':
> ../../src/fileio.c:2300:41: error: duplicate case value
>   case EEXIST: case EINVAL: case ENOSYS: case ENOTSUP:
>                                          ^~~~
> ../../src/fileio.c:2300:28: note: previously used here
>   case EEXIST: case EINVAL: case ENOSYS: case ENOTSUP:
>                             ^~~~

Should be fixed now.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 3650 bytes --]

>> and this will cause the victim to lose all the data in /home/victim/secret/bar even though the attacker is supposed to lack access to anything under /home/victim/secret.
> 
> You mean, "lose data" because files in /tmp/foo will overwrite their
> namesakes in /home/victim/secret/bar?

Yes. It's not just losing data of course; the attacker can add files to the 
victim directory.

> If my understanding of the situation is correct, then such an attack
> will still be possible with the proposed change, if /tmp/bar exists,
> because Emacs will still issue two separate system calls, and the
> symlink can be created in-between, albeit at a price of deleting
> /tmp/bar first.  Right?

No, such an attack is not possible. Emacs will run rename_noreplace which will 
fail because /tmp/bar exists, then the attacker will remove /tmp/bar and replace 
it with a symlink, then Emacs will run rename ("/tmp/foo", "/tmp/bar") which 
will fail because a directory like /tmp is sticky and the victim cannot remove 
the attacker's symlink. The attack would fail in a different way if /tmp were 
not sticky: the victim would remove the attacker's symlink.

>> As icing on the cake, the current behavior of (rename-file A B) disagrees with its documentation when B is an existing directory.
> 
> Well, 2/3 of it.  The 3rd instance, in the Emacs manual gets it right:

Unfortunately the manual does not get it right. The main part of that 3rd 
instance agrees with the proposed change, because it says the special case 
occurs if NEWNAME "is just a directory name" (on Unix, this means it ends in 
"/"). You are correct that the example is missing a "/", so I'll update the 
proposed patch to fix that.

That part of the documentation has some other confusion: a phrase "The same rule 
applies to all the remaining commands in this section" is clearly an editing 
error; it must be a revenant of when the section didn't talk about commands like 
insert-file. I just now installed the attached patch to fix that (this patch 
doesn't address the directory issue).

The bigger picture here is that this part of Emacs behavior is so poorly 
documented that it's unclear from the documentation what the intent was.

> How about eating the cake and having it, too?  We could refrain from
> testing whether B is a directory if either (1) B ends in a slash, or
> (2) rename_noreplace succeeds.

That doesn't close the security hole, I'm afraid. For example, the attacker can 
create a nonempty directory B, then rename_noreplace will fail, then Emacs will 
determine that B is a directory, then the attacker can replace B with a symlink 
to the victim directory, and then Emacs will overwrite the victim. I imagine 
other attacks are possible, this is just the first one off the top of my head.

> What I don't quite understand is what will happen under your proposal
> to the calls of the form (rename-file A B) where B names an existing
> directory and doesn't end in slash?  Will it fail, sometimes or
> always?

On POSIX systems rename-file will fail if B is a nonempty directory, and will 
succeed if B names an empty directory (this is all assuming B is not itself a 
directory name). Ideally MS-Windows would be compatible; if not, we'd have to 
document the incompatibility.

AFAIU, the 'rename' call will fail if B is a non-empty
> directory, but what if it's empty, and what does rename_noreplace do
> in these situations?  Your documentation patches don't cover this use
> case; I think we should.

Thanks, good point, I plan to update the proposed patch accordingly and to 
follow up soon.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-manual-section-Copying-and-Naming.patch --]
[-- Type: text/x-patch; name="0001-New-manual-section-Copying-and-Naming.patch", Size: 8879 bytes --]

From 5c3d0ce3e09bf070bb3c89caa9d88f25d4a39283 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 15 Aug 2017 10:06:44 -0700
Subject: [PATCH] New manual section "Copying and Naming"

* doc/emacs/files.texi (Copying and Naming):
New section, split off from Misc File Ops and containing the
operations that copy, name or rename files.  This fixes some
confusion caused by the incorrect phrase "The same rule applies
to all the remaining commands in this section" in the old manual.
This change does not affect the confusion about directories (see
Bug#27986 for ongoing discussion).
---
 doc/emacs/custom.texi |   2 +-
 doc/emacs/emacs.texi  |   1 +
 doc/emacs/files.texi  | 123 +++++++++++++++++++++++++++-----------------------
 3 files changed, 69 insertions(+), 57 deletions(-)

diff --git a/doc/emacs/custom.texi b/doc/emacs/custom.texi
index 1c9c14a..824fb6e 100644
--- a/doc/emacs/custom.texi
+++ b/doc/emacs/custom.texi
@@ -1710,7 +1710,7 @@ Init Rebinding
 specify the key sequence.  Using a string is simpler, but only works
 for @acronym{ASCII} characters and Meta-modified @acronym{ASCII}
 characters.  For example, here's how to bind @kbd{C-x M-l} to
-@code{make-symbolic-link} (@pxref{Misc File Ops}):
+@code{make-symbolic-link} (@pxref{Copying and Naming}):
 
 @example
 (global-set-key "\C-x\M-l" 'make-symbolic-link)
diff --git a/doc/emacs/emacs.texi b/doc/emacs/emacs.texi
index a3eb422..f3e6c94 100644
--- a/doc/emacs/emacs.texi
+++ b/doc/emacs/emacs.texi
@@ -453,6 +453,7 @@ Top
 * Directories::         Creating, deleting, and listing file directories.
 * Comparing Files::     Finding where two files differ.
 * Diff Mode::           Mode for editing file differences.
+* Copying and Naming::  Copying, naming and renaming files.
 * Misc File Ops::       Other things you can do on files.
 * Compressed Files::    Accessing compressed files.
 * File Archives::       Operating on tar, zip, jar etc. archive files.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 0b4e8ed..7bca988 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -33,6 +33,7 @@ Files
 * Directories::         Creating, deleting, and listing file directories.
 * Comparing Files::     Finding where two files differ.
 * Diff Mode::           Mode for editing file differences.
+* Copying and Naming::  Copying, naming and renaming files.
 * Misc File Ops::       Other things you can do on files.
 * Compressed Files::    Accessing compressed files.
 * File Archives::       Operating on tar, zip, jar etc. archive files.
@@ -1545,6 +1546,72 @@ Diff Mode
 displayed in the echo area).  With a prefix argument, it tries to
 modify the original source files rather than the patched source files.
 
+@node Copying and Naming
+@section Copying, Naming and Renaming Files
+
+  Emacs has several commands for copying, naming, and renaming files.
+All of them read two file names @var{old} and @var{new} using the
+minibuffer, and then copy or adjust a file's name accordingly; they do
+not accept wildcard file names.
+
+In all these commands, if the argument @var{new} is just a directory
+name, the real new name is in that directory, with the same
+non-directory component as @var{old}.  For example, @kbd{M-x
+rename-file @key{RET} ~/foo @key{RET}
+@c FIXME: This part of the example should be '/tmp/' not '/tmp',
+@c because '/tmp' is not "just a directory name".
+/tmp
+@c
+@key{RET}} renames @file{~/foo} to @file{/tmp/foo}.  All these
+commands ask for confirmation when the new file name already exists,
+too.
+
+@findex copy-file
+@cindex copying files
+  @kbd{M-x copy-file} copies the contents of the file @var{old} to the
+file @var{new}.
+
+@findex copy-directory
+  @kbd{M-x copy-directory} copies directories, similar to the
+@command{cp -r} shell command.  If @var{new} is an existing directory,
+it creates a copy of the @var{old} directory and puts it in @var{new}.
+If @var{new} is not an existing directory, it copies all the contents
+of @var{old} into a new directory named @var{new}.
+
+@cindex renaming files
+@findex rename-file
+  @kbd{M-x rename-file} renames file @var{old} as @var{new}.  If the
+file name @var{new} already exists, you must confirm with @kbd{yes} or
+renaming is not done; this is because renaming causes the old meaning
+of the name @var{new} to be lost.  If @var{old} and @var{new} are on
+different file systems, the file @var{old} is copied and deleted.
+
+@ifnottex
+  If a file is under version control (@pxref{Version Control}), you
+should rename it using @kbd{M-x vc-rename-file} instead of @kbd{M-x
+rename-file}.  @xref{VC Delete/Rename}.
+@end ifnottex
+
+@findex add-name-to-file
+@cindex hard links (creation)
+  @kbd{M-x add-name-to-file} adds an additional name to an existing
+file without removing the old name.  The new name is created as a hard
+link to the existing file.  The new name must belong on the same file
+system that the file is on.  On MS-Windows, this command works only if
+the file resides in an NTFS file system.  On MS-DOS, it works by
+copying the file.
+
+@findex make-symbolic-link
+@cindex symbolic links (creation)
+  @kbd{M-x make-symbolic-link} creates a symbolic link named
+@var{new}, which points at @var{target}.  The effect is that future
+attempts to open file @var{new} will refer to whatever file is named
+@var{target} at the time the opening is done, or will get an error if
+the name @var{target} is nonexistent at that time.  This command does
+not expand the argument @var{target}, so that it allows you to specify
+a relative name as the target of the link.  On MS-Windows, this
+command works only on MS Windows Vista and later.
+
 @node Misc File Ops
 @section Miscellaneous File Operations
 
@@ -1581,62 +1648,6 @@ Misc File Ops
 delete-file}.  @xref{VC Delete/Rename}.
 @end ifnottex
 
-@findex copy-file
-@cindex copying files
-  @kbd{M-x copy-file} copies the contents of the file @var{old} to the
-file @var{new}.
-
-@findex copy-directory
-  @kbd{M-x copy-directory} copies directories, similar to the
-@command{cp -r} shell command.  It prompts for a directory @var{old}
-and a destination @var{new}.  If @var{new} is an existing directory,
-it creates a copy of the @var{old} directory and puts it in @var{new}.
-If @var{new} is not an existing directory, it copies all the contents
-of @var{old} into a new directory named @var{new}.
-
-@cindex renaming files
-@findex rename-file
-  @kbd{M-x rename-file} reads two file names @var{old} and @var{new}
-using the minibuffer, then renames file @var{old} as @var{new}.  If
-the file name @var{new} already exists, you must confirm with
-@kbd{yes} or renaming is not done; this is because renaming causes the
-old meaning of the name @var{new} to be lost.  If @var{old} and
-@var{new} are on different file systems, the file @var{old} is copied
-and deleted.  If the argument @var{new} is just a directory name, the
-real new name is in that directory, with the same non-directory
-component as @var{old}.  For example, @kbd{M-x rename-file @key{RET}
-~/foo @key{RET} /tmp @key{RET}} renames @file{~/foo} to
-@file{/tmp/foo}.  The same rule applies to all the remaining commands
-in this section.  All of them ask for confirmation when the new file
-name already exists, too.
-
-@ifnottex
-  If a file is under version control (@pxref{Version Control}), you
-should rename it using @kbd{M-x vc-rename-file} instead of @kbd{M-x
-rename-file}.  @xref{VC Delete/Rename}.
-@end ifnottex
-
-@findex add-name-to-file
-@cindex hard links (creation)
-  @kbd{M-x add-name-to-file} adds an additional name to an existing
-file without removing its old name.  The new name is created as a
-hard link to the existing file.  The new name must belong on the
-same file system that the file is on.  On MS-Windows, this command
-works only if the file resides in an NTFS file system.  On MS-DOS, it
-works by copying the file.
-
-@findex make-symbolic-link
-@cindex symbolic links (creation)
-  @kbd{M-x make-symbolic-link} reads two file names @var{target} and
-@var{linkname}, then creates a symbolic link named @var{linkname},
-which points at @var{target}.  The effect is that future attempts to
-open file @var{linkname} will refer to whatever file is named
-@var{target} at the time the opening is done, or will get an error if
-the name @var{target} is nonexistent at that time.  This command does
-not expand the argument @var{target}, so that it allows you to specify
-a relative name as the target of the link.  On MS-Windows, this
-command works only on MS Windows Vista and later.
-
 @kindex C-x i
 @findex insert-file
   @kbd{M-x insert-file} (also @kbd{C-x i}) inserts a copy of the
-- 
2.7.4

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 15 Aug 2017 10:24:03 -0700
> 
> > How about eating the cake and having it, too?  We could refrain from
> > testing whether B is a directory if either (1) B ends in a slash, or
> > (2) rename_noreplace succeeds.
> 
> That doesn't close the security hole, I'm afraid. For example, the attacker can 
> create a nonempty directory B

How would they know to create B before Emacs issues any system call
that uses B?

And how is this case different from the case that Emacs calls
(rename-file A B) thinking B doesn't exist (e.g., because some prior
code tested that)?

Anyway, I firmly believe we should be backward compatible as a
fallback.  It is okay for the fallback to be insecure, as the current
code is also insecure.  But I don't think we should fail use cases
that previously were legitimate, for many years.  If my proposal is
not workable, we should come up with something that is.

> > What I don't quite understand is what will happen under your proposal
> > to the calls of the form (rename-file A B) where B names an existing
> > directory and doesn't end in slash?  Will it fail, sometimes or
> > always?
> 
> On POSIX systems rename-file will fail if B is a nonempty directory, and will 
> succeed if B names an empty directory (this is all assuming B is not itself a 
> directory name). Ideally MS-Windows would be compatible; if not, we'd have to 
> document the incompatibility.

I see no problems in being compatible in this sense.  But wiping out
the empty directory instead of moving the first argument into it is
an incompatible change, and we should avoid that.

> Thanks, good point, I plan to update the proposed patch accordingly and to 
> follow up soon.

Please say in the manual explicitly that what you call "directory
name" should end in a slash.  Yes, I know this is already described
elsewhere in the manual, but since this is important in this case, it
doesn't hurt repeating it.  It's especially important in the doc
string and in lispref manual, since there the slash must be explicit,
whereas in the interactive usage I believe RET might complete the
slash.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

Eli Zaretskii wrote:

> How would they know to create B before Emacs issues any system call
> that uses B?

Because the attackers know how Emacs work and are attempting to exploit its 
security hole.

> And how is this case different from the case that Emacs calls
> (rename-file A B) thinking B doesn't exist (e.g., because some prior
> code tested that)?

The case in question trashes a directory that the attacker lacks permission to. 
The case you're talking about does not: it merely causes rename-file to fail.

> we should be backward compatible as a fallback.

I don't see how this can work, if the fallback method relies on two system calls 
that can fall victim to a race. I tried pretty hard to come up with secure and 
backward-compatible approaches before proposing the change. I could not come up 
with any, and doubt whether anyone else could either.

Another possibility is to implement new functions (say: file-copy, file-rename, 
file-link, file-symlink, and directory-copy) that behave like the existing 
functions except without the security hole, modify callers to use these new 
functions, and then mark the existing functions as deprecated due to security 
concerns. I suspect that this would be more disruptive overall than the proposed 
change, though (albeit disruptive in a different way).

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 15 Aug 2017 12:27:52 -0700
> 
> Eli Zaretskii wrote:
> 
> > How would they know to create B before Emacs issues any system call
> > that uses B?
> 
> Because the attackers know how Emacs work and are attempting to exploit its 
> security hole.

Knowing how Emacs works is not enough: they need to actually know the
name of the directory to create, and I don't see how they can do that
without seeing some system call which names that directory.

> > And how is this case different from the case that Emacs calls
> > (rename-file A B) thinking B doesn't exist (e.g., because some prior
> > code tested that)?
> 
> The case in question trashes a directory that the attacker lacks permission to. 
> The case you're talking about does not: it merely causes rename-file to fail.

No, it's the same use case.  In both of them the attacker creates a
directory ahead of Emacs using it in some system call.

> Another possibility is to implement new functions (say: file-copy, file-rename, 
> file-link, file-symlink, and directory-copy) that behave like the existing 
> functions except without the security hole, modify callers to use these new 
> functions, and then mark the existing functions as deprecated due to security 
> concerns.

If no other solution is possible, maybe this is what we should do.  If
we decide to go that way, we should also decide what to do with the
interactive use of those functions: whether to call the old or the new
variant, because we need to keep backward compatibility there as well.
If we decide to use the old variants, deprecation might not be the
right mechanism for promoting the new variants.

> I suspect that this would be more disruptive overall than the proposed 
> change, though (albeit disruptive in a different way).

How so?

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

Eli Zaretskii wrote:

> Knowing how Emacs works is not enough: they need to actually know the
> name of the directory to create,

By "knowing how Emacs works" I meant all of Emacs, including the Lisp program 
that it is running. We cannot rely on security-via-obscurity; we must assume 
that the attackers know not only the Emacs C source code, but the Lisp code that 
Emacs runs. Such an attacker will know the name of the directory to create.

>>> And how is this case different from the case that Emacs calls
>>> (rename-file A B) thinking B doesn't exist (e.g., because some prior
>>> code tested that)?
>>
>> The case in question trashes a directory that the attacker lacks permission to.
>> The case you're talking about does not: it merely causes rename-file to fail.
> 
> No, it's the same use case.  In both of them the attacker creates a
> directory ahead of Emacs using it in some system call.

Sure, but in the use case I'm talking about, the attacker can trash the victim's 
directory even though it's write-protected. In the case you're talking about, 
the attacker can't do anything other than what the attacker could do already.

>> Another possibility is to implement new functions (say: file-copy, file-rename,
>> file-link, file-symlink, and directory-copy) that behave like the existing
>> functions except without the security hole, modify callers to use these new
>> functions, and then mark the existing functions as deprecated due to security
>> concerns.
> 
> If no other solution is possible, maybe this is what we should do.  If
> we decide to go that way, we should also decide what to do with the
> interactive use of those functions: whether to call the old or the new
> variant, because we need to keep backward compatibility there as well.

I don't see why. If a user calls M-x copy-file interactively they'll get the old 
function; if they call M-x file-copy they'll get the new one. Admittedly there 
will be confusion (see below).

>> I suspect that this would be more disruptive overall than the proposed
>> change, though (albeit disruptive in a different way).
> 
> How so?

It'll be disruption caused by the extra complexity: pairs of functions that do 
nearly the same thing, with user confusion over which function to call, and 
people calling the wrong one. Tramp will need at least four new methods to 
support, probably more. The complexity and confusion will go on and on, and will 
cost more than will be saved by the backward compatibility. It would be worth 
all this trouble if people needed the old behavior, but they mostly do not.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 15 Aug 2017 22:06:38 -0700
> 
>     Knowing how Emacs works is not enough: they need to actually know the
>     name of the directory to create,
> 
> By "knowing how Emacs works" I meant all of Emacs, including the Lisp program that it is running. We cannot rely on security-via-obscurity; we must assume that the attackers know not only the Emacs C source code, but the Lisp code that Emacs runs. Such an attacker will know the name of the directory to create.

It is reasonable to assume that the attacker has access to the Emacs
sources, and so knows what Emacs will do in any specific situation,
given enough information about the files and directories involved in
the particular use case.  But it is not necessarily reasonable to
assume the attacker knows exactly what Emacs is doing at this very
moment, and what files/directories it will be accessing in the very
near future, without any evidence from the system calls emitted by
Emacs and/or the changes to the filesystem it made in the recent past.
You are describing a situation where the attacker somehow knows what
file/directory will be accessed _ahead_ of Emacs actually accessing
it.  This _might_ be somehow possible when Emacs runs
non-interactively, as part of some deterministic script, but not in
interactive usage, which is how Emacs is normally used.  So I don't
see how the attacker could do that in general, except by having full
control of the Emacs process, in the 'ptrace' sense or similar.  And
if the attacker has such a control, you cannot defend against it,
because it can simply call any Emacs functions with any arguments.

IOW I don't see any "security by obscurity" here, I see a case where
you assign super-natural abilities to attackers.

I think our security related scenario should start after the initial
call to rename_noreplace, and not before it, because before that call
there's no external evidence that Emacs is going to call rename-file,
and with what arguments.

>     If no other solution is possible, maybe this is what we should do.  If
>     we decide to go that way, we should also decide what to do with the
>     interactive use of those functions: whether to call the old or the new
>     variant, because we need to keep backward compatibility there as well.
> 
> I don't see why. If a user calls M-x copy-file interactively they'll
> get the old function; if they call M-x file-copy they'll get the new
> one.

I thought you were proposing to redirect the interactive commands to
the new functions.  If that's not what you meant, then indeed there's
no such issue, but then obsoleting is out of the question, since we
cannot obsolete user commands.

> It'll be disruption caused by the extra complexity: pairs of functions that do nearly the same thing, with user confusion over which function to call, and people calling the wrong one. Tramp will need at least four new methods to support, probably more. The complexity and confusion will go on and on, and will cost more than will be saved by the backward compatibility. It would be worth all this trouble if people needed the old behavior, but they mostly do not.

It's additional complexity, I agree.  But if people want secure code,
they _will_ use the more secure variants (which is why I think
rename-file-securely is a better name than file-rename, and similarly
for other functions).  It's basically the same situation as with, say,
strtok vs strtok_r, to take just one similar example.

Whether people want "M-x foo bar" produce bar/foo or just bar, is open
to argument.  My impression is that they want the former, because it
is what "mv foo bar" does by default when bar is a directory.  Note
that mv doesn't require a trailing slash in this case, and it gives
the user an explicit opt-in switch to change the behavior to the one
you proposed.  (We could provide such an optional argument as well, as
an alternative to introducing new functions.)

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

Eli Zaretskii wrote:
> You are describing a situation where the attacker somehow knows what
> file/directory will be accessed_ahead_  of Emacs actually accessing
> it.

Sure, and this happens all the time. Emacs prepares a copy of a file with the 
intent to rename the copy to the original atomically. The attacker will know 
that this is what Emacs will do, by looking at the file system or the syscalls 
Emacs issues before its code calls rename-file (e.g., Emacs will read the old 
file). So I am not supposing any kind of superhuman attack.

I do take your point that interactive use is different. So, here is a proposed 
change to the patch: if the ok-is-already-exists flag is an integer (which 
suggests interactive use), and if the destination is not a directory name 
(trailing "/") but happens to be an existing directory, then Emacs asks the user 
if it is OK to rename to a subfile of the destination. This would allay most the 
security concerns that I have, and I hope it would address most of the 
backward-compatibility concerns that you have.

> I thought you were proposing to redirect the interactive commands to
> the new functions.

I was not proposing to redirect 'M-x rename-file' etc. They would continue to 
use the old insecure behavior, for compatibility reasons.

> we cannot obsolete user commands.

Not immediately, no. But we can mark them as obsolescent and warn users about 
their use, and remove them eventually.

This issue of obsolescence is moot, though, if you agree with the above 
suggestion about ok-if-already-exists.

> if people want secure code,
> they _will_ use the more secure variants

Emacs is a relatively large and complex system, and we cannot expect users to be 
familiar with every detail. Emacs should have safe defaults, not unsafe ones.

The situation with "mv" was different, as POSIX and longstanding documentation 
required the unsafe behavior and many scripts relied on it. In contrast, the 
Emacs documentation is thoroughly muddled and contradictory in this area, and 
code using rename-file etc. would more likely benefit from the proposed change 
(because of improved security) than be hurt by it (by loss of backward 
compatibility with poorly-documented and insecure behavior).

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 16 Aug 2017 08:15:34 -0700
> 
> I do take your point that interactive use is different. So, here is a proposed 
> change to the patch: if the ok-is-already-exists flag is an integer (which 
> suggests interactive use), and if the destination is not a directory name 
> (trailing "/") but happens to be an existing directory, then Emacs asks the user 
> if it is OK to rename to a subfile of the destination. This would allay most the 
> security concerns that I have, and I hope it would address most of the 
> backward-compatibility concerns that you have.

I don't know...  Did you look at all the users of these functions in
our codebase?  E.g., I see at least one use of rename-file in Gnus
that moves a directory, possibly 2 such uses.  And I only looked at a
single function.  What's more, some of the use cases will not even
signal an error after the change, they will instead silently do
something different from the previous versions, which is really bad.

We could be easily shooting ourselves in the foot with such
incompatible changes.  At the very least, all the users in Emacs
should be audited and fixed as needed.

What do others think?  Richard, Stefan, John?

> The situation with "mv" was different, as POSIX and longstanding documentation 
> required the unsafe behavior and many scripts relied on it. In contrast, the 
> Emacs documentation is thoroughly muddled and contradictory in this area, and 
> code using rename-file etc. would more likely benefit from the proposed change 
> (because of improved security) than be hurt by it (by loss of backward 
> compatibility with poorly-documented and insecure behavior).

My problem is not with being able to defend our change in a court of
law, my problem is with people's muscle memory and with existing code
that was working in certain ways since about forever.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

Eli Zaretskii wrote:

> Did you look at all the users of these functions in
> our codebase?

I have not looked at every single one in detail. I've looked at a fair sample. 
See below for more discussion.

> E.g., I see at least one use of rename-file in Gnus
> that moves a directory, possibly 2 such uses.

Moving a directory is not a problem. The only problem is when the destination is 
a directory but not a directory name and the intent is to change an entry in 
that directory rather than to change the original destination.

I agree that some uses in code will need to be adjusted. Most won't, though. For 
example, in the first occurrence of the string "rename-file" in Gnus, where 
gnus-agent-rename-group calls (gnus-rename-file old-path new-path t), the intent 
is to rename OLD-PATH to NEW-PATH, not to rename it to be an subsidiary entry to 
NEW-PATH. For this particular example, the proposed change is slightly 
beneficial, since it prevents rename-file from doing the wrong thing in the 
(admittedly unlikely) event that some other process changes NEW-PATH to a 
directory while Gnus is operating.

> What's more, some of the use cases will not even
> signal an error after the change, they will instead silently do
> something different from the previous versions, which is really bad.

This should be quite rare. The only scenario I see matching your concern is if 
the source is a directory, the destination is not a directory name but is an 
empty directory and is not a symlink, and the destination is not a descendant of 
the source. Although not impossible, this will happen so rarely that it doesn't 
invalidate the proposed change.

> At the very least, all the users in Emacs
> should be audited and fixed as needed.

Sure, I'll volunteer to do that. There are only 172 lines containing the string 
"rename-file" in our Emacs Lisp code base, for example, and it shouldn't be that 
much work to check them.

I've looked at this issue fairly carefully, and I'm afraid the solution I've 
proposed is the best way forward if we want to close the security hole in Emacs.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 16 Aug 2017 10:19:35 -0700
> 
> > What's more, some of the use cases will not even
> > signal an error after the change, they will instead silently do
> > something different from the previous versions, which is really bad.
> 
> This should be quite rare. The only scenario I see matching your concern is if 
> the source is a directory, the destination is not a directory name but is an 
> empty directory and is not a symlink, and the destination is not a descendant of 
> the source. Although not impossible, this will happen so rarely that it doesn't 
> invalidate the proposed change.

I don't think we know how rare that is.  And if it is very rare, I'm
not sure it's better, because it means such problems might go
unnoticed and/or unfixed for years.

> I've looked at this issue fairly carefully, and I'm afraid the solution I've 
> proposed is the best way forward if we want to close the security hole in Emacs.

Let's hear more opinions, okay?

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Glenn Morris]

Eli Zaretskii wrote:

> Let's hear more opinions, okay?

FWIW, I think Paul's proposal is eminently sensible.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Ken Brown]

On 8/14/2017 7:03 PM, Paul Eggert wrote
> Now that renameat_noreplace works on DOS_NT, would it make sense to 
> apply the attached further patch as well? If we can get 
> renameat_noreplace to work on Cygwin the we could simplify the fileio.c 
> code even further.

I'm in the process of writing an implementation of something like 
'renameat2', which I'll submit to the Cygwin developers.  Even if it's 
accepted, however, I think we'll still need to retain the 
case-insensitivity test for users of old versions of Cygwin, unless 
there's a decision to remove that because of the security concerns 
currently being discussed.

Ken

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Stefan Monnier]

>> This should be quite rare.  The only scenario I see matching your
>> concern is if the source is a directory, the destination is not
>> a directory name but is an empty directory and is not a symlink, and
>> the destination is not a descendant of  the source.  Although not
>> impossible, this will happen so rarely that it doesn't invalidate
>> the proposed change.

Paul's suggestion makes a lot of sense to me, but I don't quite
understand the above: why does the emptiness of the destination
directory matter?


        Stefan

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

On 08/16/2017 03:31 PM, Stefan Monnier wrote:
>>> This should be quite rare.  The only scenario I see matching your
>>> concern is if the source is a directory, the destination is not
>>> a directory name but is an empty directory and is not a symlink, and
>>> the destination is not a descendant of  the source.  Although not
>>> impossible, this will happen so rarely that it doesn't invalidate
>>> the proposed change.
> Paul's suggestion makes a lot of sense to me, but I don't quite
> understand the above: why does the emptiness of the destination
> directory matter?

It's because the system call rename(A,B) always fails when B is a 
nonempty directory. Hence the proposed (rename-file A B) will fail if B 
is not a directory name but happens to name a nonempty directory, and 
therefore rename-file noisily fails instead of silently behaving 
differently in this case (which was Eli's concern). Conversely, if B is 
an empty directory then rename(A,B) can succeed (and thereby remove the 
old B) if all the other conditions are met.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Stefan Monnier]

> case (which was Eli's concern). Conversely, if B is an empty directory then
> rename(A,B) can succeed (and thereby remove the old B) if all the other
> conditions are met.

Ah, right, if it's an empty dir then it can be
removed/replaced atomically.  Thanks.  It does seem like the odd case
should be rare, then, indeed.


        Stefan

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: Paul Eggert <eggert@cs.ucla.edu>, John Wiegley <johnw@gnu.org>,
>         rms@gnu.org, p.stephani2@gmail.com, 27986@debbugs.gnu.org
> Date: Wed, 16 Aug 2017 18:31:14 -0400
> 
> Paul's suggestion makes a lot of sense to me

John, what's your take on this?

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Ken Brown]

[-- Attachment #1: Type: text/plain, Size: 843 bytes --]

On 8/16/2017 3:33 PM, Ken Brown wrote:
> On 8/14/2017 7:03 PM, Paul Eggert wrote
>> Now that renameat_noreplace works on DOS_NT, would it make sense to 
>> apply the attached further patch as well? If we can get 
>> renameat_noreplace to work on Cygwin the we could simplify the 
>> fileio.c code even further.
> 
> I'm in the process of writing an implementation of something like 
> 'renameat2', which I'll submit to the Cygwin developers.

This is now done.  The implementation will appear in the next Cygwin 
release.  When that release occurs, I'll install something like the 
attached patch.

Question: Is the patch OK as is, or should I make the call to renameat2 
conditional on CYGWIN?  In other words, is it safe to assume that 
renameat2 is defined on any platform on which RENAME_NOREPLACE is 
defined but not SYS_renameat2?

Ken


[-- Attachment #2: 0001-Implement-renameat_noreplace-on-recent-Cygwin.patch --]
[-- Type: text/plain, Size: 1219 bytes --]

From b9ef4ca995350d1a520aba66de1d5e4209f3eb91 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbrown@cornell.edu>
Date: Sat, 19 Aug 2017 17:17:27 -0400
Subject: [PATCH] Implement renameat_noreplace on recent Cygwin

* src/sysdep.c [CYGWIN]: Include cygwin/fs.h.
(renameat_noreplace) [RENAME_NOREPLACE]: Use renameat2.
(Bug#27986)
---
 src/sysdep.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/sysdep.c b/src/sysdep.c
index 12e9c83ee9..77221097db 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -42,6 +42,10 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 # include <sys/syscall.h>
 #endif
 
+#ifdef CYGWIN
+# include <cygwin/fs.h>
+#endif
+
 #if defined DARWIN_OS || defined __FreeBSD__
 # include <sys/sysctl.h>
 #endif
@@ -2685,6 +2689,8 @@ renameat_noreplace (int srcfd, char const *src, int dstfd, char const *dst)
 {
 #if defined SYS_renameat2 && defined RENAME_NOREPLACE
   return syscall (SYS_renameat2, srcfd, src, dstfd, dst, RENAME_NOREPLACE);
+#elif defined RENAME_NOREPLACE	/* Cygwin >= 2.9.0. */
+  return renameat2 (srcfd, src, dstfd, dst, RENAME_NOREPLACE);
 #elif defined RENAME_EXCL
   return renameatx_np (srcfd, src, dstfd, dst, RENAME_EXCL);
 #else
-- 
2.14.1

bug#27986: 26.0.50; 'rename-file' can rename files without

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

I haven't followed the discussion, but I'm told there is a proposal to
make this change:

  > Btw, in case it isn't clear: the issue at hand is an incompatible
  > change to rename-file (and probably also other functions, like
  > copy-file).  Where previously (rename-file A B) with B a directory
  > will move A into B/A, under the proposed change it will only do so if
  > B actually ended in a slash; otherwise it will move A to B, deleting B
  > if it exists.  The incompatibility will manifest itself if some old
  > code expects to get B/A, but instead gets either an error (if B is a
  > non-empty directory) or B silently removed (if it is empty).

Assuming this applies only when directory B is empty, so that this
won't delete non-empty directories, then I don't have any objection.
I would object to deleting non-empty directories here.

Another option that might be good is to make this operation always
signal an error in the case where B is a directory and does not end
with a slash.

I don't have an opinion about which of those two is better.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

Ken Brown wrote:
> This is now done.  The implementation will appear in the next Cygwin release.  
> When that release occurs, I'll install something like the attached patch.

Thanks.

> Question: Is the patch OK as is, or should I make the call to renameat2 
> conditional on CYGWIN?  In other words, is it safe to assume that renameat2 is 
> defined on any platform on which RENAME_NOREPLACE is defined but not SYS_renameat2?

It should be OK, for the same reason the RENAME_EXCL branch is OK (it assumes 
renameatx_np). If we run across some future platform where it doesn't work, we 
can port it then. As far as I know, Cygwin will be the first platform with 
renameat2 in its C library.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Ken Brown]

On 8/19/2017 5:37 PM, Paul Eggert wrote:
> Ken Brown wrote:
>> This is now done.  The implementation will appear in the next Cygwin 
>> release. When that release occurs, I'll install something like the 
>> attached patch.
> 
> Thanks.
> 
>> Question: Is the patch OK as is, or should I make the call to 
>> renameat2 conditional on CYGWIN?  In other words, is it safe to assume 
>> that renameat2 is defined on any platform on which RENAME_NOREPLACE is 
>> defined but not SYS_renameat2?
> 
> It should be OK, for the same reason the RENAME_EXCL branch is OK (it 
> assumes renameatx_np). If we run across some future platform where it 
> doesn't work, we can port it then. As far as I know, Cygwin will be the 
> first platform with renameat2 in its C library.

Is there any (good) reason that glibc doesn't provide a wrapper for it? 
It seems that this would be pretty trivial to do.

Ken

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

On 08/19/2017 03:04 PM, Ken Brown wrote:
>
> Is there any (good) reason that glibc doesn't provide a wrapper for 
> it? It seems that this would be pretty trivial to do. 

On the glibc side there has been a concern that it support a good GNU 
libc interface that does not assume a Linux kernel, as opposed to merely 
being a thin wrapper around whatever system calls Linux happens to 
provide. I don't know what the specific objections to renameat2 are, though.

bug#27986: 26.0.50; 'rename-file' can rename files without

[Eli Zaretskii]

> From: Richard Stallman <rms@gnu.org>
> Date: Sat, 19 Aug 2017 17:33:56 -0400
> 
>   > Btw, in case it isn't clear: the issue at hand is an incompatible
>   > change to rename-file (and probably also other functions, like
>   > copy-file).  Where previously (rename-file A B) with B a directory
>   > will move A into B/A, under the proposed change it will only do so if
>   > B actually ended in a slash; otherwise it will move A to B, deleting B
>   > if it exists.  The incompatibility will manifest itself if some old
>   > code expects to get B/A, but instead gets either an error (if B is a
>   > non-empty directory) or B silently removed (if it is empty).
> 
> Assuming this applies only when directory B is empty, so that this
> won't delete non-empty directories, then I don't have any objection.
> I would object to deleting non-empty directories here.
> 
> Another option that might be good is to make this operation always
> signal an error in the case where B is a directory and does not end
> with a slash.
> 
> I don't have an opinion about which of those two is better.

Thanks.  I do indeed think that making this an error even if B is an
empty directory would be better, as it will reveal the cases that need
to be fixed more prominently.

bug#27986: 26.0.50; 'rename-file' can rename files without

[John Wiegley]

>>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:

EZ> Thanks. I do indeed think that making this an error even if B is an empty
EZ> directory would be better, as it will reveal the cases that need to be
EZ> fixed more prominently.

I prefer that to renaming a directory when the target is a directory.  That's
not how /bin/mv behaves, so I'd be surprised if (rename-file) did differently.
Making the Emacs version more restrictive, however, makes sense.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#27986: 26.0.50; 'rename-file' can rename files without

[Eli Zaretskii]

> From: John Wiegley <jwiegley@gmail.com>
> Cc: rms@gnu.org,  27986@debbugs.gnu.org
> Date: Fri, 25 Aug 2017 13:33:46 -0700
> 
> >>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> EZ> Thanks. I do indeed think that making this an error even if B is an empty
> EZ> directory would be better, as it will reveal the cases that need to be
> EZ> fixed more prominently.
> 
> I prefer that to renaming a directory when the target is a directory.  That's
> not how /bin/mv behaves, so I'd be surprised if (rename-file) did differently.
> Making the Emacs version more restrictive, however, makes sense.

Thanks.

Paul, it sounds like we have a consensus on this, so please go ahead
with the changes.  At least Richard, John, and myself prefer to signal
an error even when the target is an empty directory, so I hope the
non-interactive rename-file could do that after your changes.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

OK, I installed the patch into the master branch. Also, I reviewed all calls to 
the affected functions and have a few followup patches that I plan to install 
separately soon, and mention here as I do.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

Paul Eggert wrote:
> I reviewed all calls to the affected functions and have a few followup patches 
> that I plan to install separately soon, and mention here as I do.

I installed the patches (attached). Closing the bug report.

I'll CC: this to Michael, as the last patch is to the Tramp tests, and he may 
want Tramp to become consistent with the new behavior for rename-file etc.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-copy-directory-act-like-copy-file-etc.patch --]
[-- Type: text/x-patch; name="0001-Make-copy-directory-act-like-copy-file-etc.patch", Size: 5285 bytes --]

From e22794867d878d53675fcc91d2ef1ad2494a2ff2 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 10 Sep 2017 22:07:30 -0700
Subject: [PATCH 1/6] Make copy-directory act like copy-file etc.

Do the special dance with the destination only if it is a
directory name, for consistency with copy-file etc. (Bug#27986).
* doc/emacs/files.texi (Copying and Naming):
* doc/lispref/files.texi (Create/Delete Dirs):
* etc/NEWS: Document this.
* lisp/files.el (copy-directory): Treat NEWNAME as special
only if it is a directory name.
---
 doc/emacs/files.texi   |  8 ++++----
 doc/lispref/files.texi |  5 +++--
 etc/NEWS               |  4 ++--
 lisp/files.el          | 22 ++++++++++------------
 4 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 0cf46b6..ca4f223 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1572,10 +1572,10 @@ Copying and Naming
 
 @findex copy-directory
   @kbd{M-x copy-directory} copies directories, similar to the
-@command{cp -r} shell command.  If @var{new} is an existing directory,
-it creates a copy of the @var{old} directory and puts it in @var{new}.
-If @var{new} is not an existing directory, it copies all the contents
-of @var{old} into a new directory named @var{new}.
+@command{cp -r} shell command.  If @var{new} is a directory name, it
+creates a copy of the @var{old} directory and puts it in @var{new}.
+Otherwise it copies all the contents of @var{old} into a new directory
+named @var{new}.
 
 @cindex renaming files
 @findex rename-file
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index eacaf04..901382f 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -2976,8 +2976,9 @@ Create/Delete Dirs
 
 @deffn Command copy-directory dirname newname &optional keep-time parents copy-contents
 This command copies the directory named @var{dirname} to
-@var{newname}.  If @var{newname} names an existing directory,
+@var{newname}.  If @var{newname} is a directory name,
 @var{dirname} will be copied to a subdirectory there.
+@xref{Directory Names}.
 
 It always sets the file modes of the copied files to match the
 corresponding original file.
@@ -2992,7 +2993,7 @@ Create/Delete Dirs
 
 The fifth argument @var{copy-contents}, if non-@code{nil}, means to
 copy the contents of @var{dirname} directly into @var{newname} if the
-latter is an existing directory, instead of copying @var{dirname} into
+latter is a directory name, instead of copying @var{dirname} into
 it as a subdirectory.
 @end deffn
 
diff --git a/etc/NEWS b/etc/NEWS
index 4187dd8..136d458 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1285,8 +1285,8 @@ documentation and had inherent races that led to security holes.  A
 call like (rename-file C D) that used the old, undocumented behavior
 can be written as (rename-file C (file-name-as-directory D)), a
 formulation portable to both older and newer versions of Emacs.
-Affected functions include add-name-to-file, copy-file,
-make-symbolic-link, and rename-file.
+Affected functions include add-name-to-file, copy-directory,
+copy-file, make-symbolic-link, and rename-file.
 
 \f
 * Lisp Changes in Emacs 26.1
diff --git a/lisp/files.el b/lisp/files.el
index 85e649f..7ab6f76 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5501,10 +5501,10 @@ copy-directory
 create parent directories if they don't exist.  Interactively,
 this happens by default.
 
-If NEWNAME names an existing directory, copy DIRECTORY as a
-subdirectory there.  However, if called from Lisp with a non-nil
-optional argument COPY-CONTENTS, copy the contents of DIRECTORY
-directly into NEWNAME instead."
+If NEWNAME is a directory name, copy DIRECTORY as a subdirectory
+there.  However, if called from Lisp with a non-nil optional
+argument COPY-CONTENTS, copy the contents of DIRECTORY directly
+into NEWNAME instead."
   (interactive
    (let ((dir (read-directory-name
 	       "Copy directory: " default-directory default-directory t nil)))
@@ -5526,19 +5526,17 @@ copy-directory
 
       ;; Compute target name.
       (setq directory (directory-file-name (expand-file-name directory))
-	    newname   (directory-file-name (expand-file-name newname)))
+	    newname (expand-file-name newname))
 
-      (cond ((not (file-directory-p newname))
-	     ;; If NEWNAME is not an existing directory, create it;
+      (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 an existing directory and COPY-CONTENTS
+	    ;; If NEWNAME is a directory name and COPY-CONTENTS
 	    ;; is nil, copy into NEWNAME/[DIRECTORY-BASENAME].
 	    ((not copy-contents)
-	     (setq newname (concat
-			    (file-name-as-directory newname)
-			    (file-name-nondirectory
-			     (directory-file-name directory))))
+	     (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"
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Make-write-file-act-like-copy-file-etc.patch --]
[-- Type: text/x-patch; name="0002-Make-write-file-act-like-copy-file-etc.patch", Size: 1741 bytes --]

From 61946d991b663c9d35a50b758d0108c3cbf8027b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 10 Sep 2017 22:19:01 -0700
Subject: [PATCH 2/6] Make write-file act like copy-file etc.

Change write-file to be consistent with the new behavior
of copy-file, etc.
* etc/NEWS: Mention this.
* lisp/files.el (write-file): Treat the destination as special
only if it is a directory name.
---
 etc/NEWS      | 3 ++-
 lisp/files.el | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 136d458..4da4c37 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1286,7 +1286,8 @@ call like (rename-file C D) that used the old, undocumented behavior
 can be written as (rename-file C (file-name-as-directory D)), a
 formulation portable to both older and newer versions of Emacs.
 Affected functions include add-name-to-file, copy-directory,
-copy-file, make-symbolic-link, and rename-file.
+copy-file, format-write-file, make-symbolic-link, rename-file, and
+write-file.
 
 \f
 * Lisp Changes in Emacs 26.1
diff --git a/lisp/files.el b/lisp/files.el
index 7ab6f76..611a4c5 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -4212,10 +4212,10 @@ write-file
 	 (not current-prefix-arg)))
   (or (null filename) (string-equal filename "")
       (progn
-	;; If arg is just a directory,
+	;; If arg is a directory name,
 	;; use the default file name, but in that directory.
-	(if (file-directory-p filename)
-	    (setq filename (concat (file-name-as-directory filename)
+	(if (directory-name-p filename)
+	    (setq filename (concat filename
 				   (file-name-nondirectory
 				    (or buffer-file-name (buffer-name))))))
 	(and confirm
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Make-gnus-copy-file-act-like-copy-file-etc.patch --]
[-- Type: text/x-patch; name="0003-Make-gnus-copy-file-act-like-copy-file-etc.patch", Size: 1626 bytes --]

From 739593d68742f45e4e35dfc99573c47a5031b646 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 10 Sep 2017 22:21:20 -0700
Subject: [PATCH 3/6] Make gnus-copy-file act like copy-file etc.

* etc/NEWS: Mention this.
* lisp/gnus/gnus-util.el (gnus-copy-file): Treat the destination
as special only if it is a directory name.
---
 etc/NEWS               | 4 ++--
 lisp/gnus/gnus-util.el | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 4da4c37..fc40a3a 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1286,8 +1286,8 @@ call like (rename-file C D) that used the old, undocumented behavior
 can be written as (rename-file C (file-name-as-directory D)), a
 formulation portable to both older and newer versions of Emacs.
 Affected functions include add-name-to-file, copy-directory,
-copy-file, format-write-file, make-symbolic-link, rename-file, and
-write-file.
+copy-file, format-write-file, gnus-copy-file, make-symbolic-link,
+rename-file, and write-file.
 
 \f
 * Lisp Changes in Emacs 26.1
diff --git a/lisp/gnus/gnus-util.el b/lisp/gnus/gnus-util.el
index b509d8a..93541f0 100644
--- a/lisp/gnus/gnus-util.el
+++ b/lisp/gnus/gnus-util.el
@@ -594,9 +594,6 @@ gnus-copy-file
 	 (read-file-name "Copy file to: " default-directory)))
   (unless to
     (setq to (read-file-name "Copy file to: " default-directory)))
-  (when (file-directory-p to)
-    (setq to (concat (file-name-as-directory to)
-		     (file-name-nondirectory file))))
   (copy-file file to))
 
 (defvar gnus-work-buffer " *gnus work*")
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-Adjust-ob-tangle-to-new-copy-file-behavior.patch --]
[-- Type: text/x-patch; name="0004-Adjust-ob-tangle-to-new-copy-file-behavior.patch", Size: 865 bytes --]

From 74b8615fcceba7b92c4938e1bcc92015f10ae899 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 10 Sep 2017 22:22:55 -0700
Subject: [PATCH 4/6] Adjust ob-tangle to new copy-file behavior

* lisp/org/ob-tangle.el (org-babel-tangle-publish):
Port to new copy-file behavior.
---
 lisp/org/ob-tangle.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/org/ob-tangle.el b/lisp/org/ob-tangle.el
index 3b05332..2dc55ca 100644
--- a/lisp/org/ob-tangle.el
+++ b/lisp/org/ob-tangle.el
@@ -197,6 +197,7 @@ org-babel-tangle-publish
   "Tangle FILENAME and place the results in PUB-DIR."
   (unless (file-exists-p pub-dir)
     (make-directory pub-dir t))
+  (setq pub-dir (file-name-as-directory pub-dir))
   (mapc (lambda (el) (copy-file el pub-dir t)) (org-babel-tangle-file filename)))
 
 ;;;###autoload
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0005-Adjust-thumbs-to-new-rename-file-behavior.patch --]
[-- Type: text/x-patch; name="0005-Adjust-thumbs-to-new-rename-file-behavior.patch", Size: 2260 bytes --]

From 2aa028825920207cca2bacb581111ab780e5d9ee Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 10 Sep 2017 22:28:08 -0700
Subject: [PATCH 5/6] Adjust thumbs to new rename-file behavior

* etc/NEWS: Mention this.
* lisp/thumbs.el (thumbs-rename-images): Treat the destination
as special only if it is a directory name.  When there is
a marked list, turn the destination into a directory name
if it is not already.
---
 etc/NEWS       |  2 +-
 lisp/thumbs.el | 15 ++++-----------
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index fc40a3a..3f1df23 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1287,7 +1287,7 @@ can be written as (rename-file C (file-name-as-directory D)), a
 formulation portable to both older and newer versions of Emacs.
 Affected functions include add-name-to-file, copy-directory,
 copy-file, format-write-file, gnus-copy-file, make-symbolic-link,
-rename-file, and write-file.
+rename-file, thumbs-rename-images, and write-file.
 
 \f
 * Lisp Changes in Emacs 26.1
diff --git a/lisp/thumbs.el b/lisp/thumbs.el
index 0665429..d0b5e22 100644
--- a/lisp/thumbs.el
+++ b/lisp/thumbs.el
@@ -523,23 +523,16 @@ thumbs-rename-images
   (interactive "FRename to file or directory: ")
   (let ((files (or thumbs-marked-list (list (thumbs-current-image))))
 	failures)
-    (if (and (not (file-directory-p newfile))
-	     thumbs-marked-list)
-	(if (file-exists-p newfile)
-	    (error "Renaming marked files to file name `%s'" newfile)
-	  (make-directory newfile t)))
+    (when thumbs-marked-list
+      (make-directory newfile t)
+      (setq newfile (file-name-as-directory newfile)))
     (if (yes-or-no-p (format "Really rename %d files? " (length files)))
 	(let ((thumbs-file-list (thumbs-file-alist))
 	      (inhibit-read-only t))
 	  (dolist (file files)
 	    (let (failure)
 	      (condition-case ()
-		  (if (file-directory-p newfile)
-		      (rename-file file
-				   (expand-file-name
-				    (file-name-nondirectory file)
-				    newfile))
-		    (rename-file file newfile))
+		  (rename-file file newfile)
 		(file-error (setq failure t)
 			    (push file failures)))
 	      (unless failure
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #7: 0006-Port-tramp-tests-to-new-copy-directory-behavior.patch --]
[-- Type: text/x-patch; name="0006-Port-tramp-tests-to-new-copy-directory-behavior.patch", Size: 1513 bytes --]

From 29963648dd11d53088f753e4f9b0491a7b981c0f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 10 Sep 2017 23:04:10 -0700
Subject: [PATCH 6/6] Port tramp-tests to new copy-directory behavior

* test/lisp/net/tramp-tests.el (tramp-test15-copy-directory):
Use directory name as arg for copy-directory when we want
the special behavior.
---
 test/lisp/net/tramp-tests.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index 735211c..4139d50 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -2117,7 +2117,7 @@ tramp--test-backtrace
 	    (should (file-directory-p tmp-name2))
 	    (should (file-exists-p tmp-name5))
 	    ;; Target directory does exist already.
-	    (copy-directory tmp-name1 tmp-name2)
+	    (copy-directory tmp-name1 (file-name-as-directory tmp-name2))
 	    (should (file-directory-p tmp-name3))
 	    (should (file-exists-p tmp-name6)))
 
@@ -2140,7 +2140,8 @@ tramp--test-backtrace
 	    ;; Target directory does exist already.
 	    (delete-file tmp-name5)
 	    (should-not (file-exists-p tmp-name5))
-	    (copy-directory tmp-name1 tmp-name2 nil 'parents 'contents)
+	    (copy-directory tmp-name1 (file-name-as-directory tmp-name2)
+			    nil 'parents 'contents)
 	    (should (file-directory-p tmp-name2))
 	    (should (file-exists-p tmp-name5))
 	    (should-not (file-directory-p tmp-name3))
-- 
2.7.4

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: johnw@gnu.org, rms@gnu.org, p.stephani2@gmail.com,
>  27986-done@debbugs.gnu.org, Michael Albinus <michael.albinus@gmx.de>
> Date: Sun, 10 Sep 2017 23:07:34 -0700
> 
> Paul Eggert wrote:
> > I reviewed all calls to the affected functions and have a few followup patches 
> > that I plan to install separately soon, and mention here as I do.
> 
> I installed the patches (attached). Closing the bug report.

Am I missing something, or do these changes modify interactive
behavior in incompatible ways?  I thought we agreed to leave the
interactive behavior intact, and only change the non-interactive uses.

Thanks.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

On 09/11/2017 07:47 AM, Eli Zaretskii wrote:
> do these changes modify interactive
> behavior in incompatible ways?  I thought we agreed to leave the
> interactive behavior intact, and only change the non-interactive uses.
>
That wasn't my understanding. In our last email exchange about 
interactivity I proposed that Emacs prompt the user when the destination 
is not a directory name but happens to be a directory (see 
Bug#27986#97), but you were dubious about that (Bug#27986#100) so I left 
it alone.

I normally use dired to rename files in Emacs, and dired's behavior 
hasn't changed as far as I can tell. Where there is a bit of a change is 
when using M-x rename-file directly. Here, in the typical case with file 
name completion there is still no difference, since names of destination 
directories are completed to have trailing /. However, if one uses "M-x 
rename-file foo RET and then laboriously types out the name of an 
existing directory /tmp/destination-dir without using completion and 
without trailing / before hitting RET, one will notice a difference: 
rename-file will now say "File /tmp/destination-dir already exists; 
rename to it anyway? (yes or no)" and if one types "yes" the rename will 
typically fail. So yes, this is a (noisy) incompatibility with previous 
usage. If you like I can go back and implement the suggestion in 
Bug#27986#97; this would be more-compatible with existing usage. I 
suggest leaving it alone, though, as things are simpler and easier to 
explain the way they are.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Eli Zaretskii]

> Cc: monnier@IRO.UMontreal.CA, johnw@gnu.org, rms@gnu.org,
>  p.stephani2@gmail.com, 27986@debbugs.gnu.org, michael.albinus@gmx.de
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 11 Sep 2017 09:45:48 -0700
> 
> On 09/11/2017 07:47 AM, Eli Zaretskii wrote:
> > do these changes modify interactive
> > behavior in incompatible ways?  I thought we agreed to leave the
> > interactive behavior intact, and only change the non-interactive uses.
> >
> That wasn't my understanding. In our last email exchange about 
> interactivity I proposed that Emacs prompt the user when the destination 
> is not a directory name but happens to be a directory (see 
> Bug#27986#97), but you were dubious about that (Bug#27986#100) so I left 
> it alone.
> 
> I normally use dired to rename files in Emacs, and dired's behavior 
> hasn't changed as far as I can tell. Where there is a bit of a change is 
> when using M-x rename-file directly. Here, in the typical case with file 
> name completion there is still no difference, since names of destination 
> directories are completed to have trailing /. However, if one uses "M-x 
> rename-file foo RET and then laboriously types out the name of an 
> existing directory /tmp/destination-dir without using completion and 
> without trailing / before hitting RET, one will notice a difference: 
> rename-file will now say "File /tmp/destination-dir already exists; 
> rename to it anyway? (yes or no)" and if one types "yes" the rename will 
> typically fail. So yes, this is a (noisy) incompatibility with previous 
> usage. If you like I can go back and implement the suggestion in 
> Bug#27986#97; this would be more-compatible with existing usage. I 
> suggest leaving it alone, though, as things are simpler and easier to 
> explain the way they are.

I think there was a confusion here between interactive and
non-interactive uses of rename-file.  For interactive use, AFAIR we
agreed that the behavior should stay as before, in particular to be
consistent with e.g. invocation of 'mv' from the shell prompt.

For non-interactive use, we agreed to change the behavior wrt
directories, with a slight preference to signaling an error even if
the target is an empty directory.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert]

On 09/11/2017 10:09 AM, Eli Zaretskii wrote:
> I think there was a confusion here between interactive and
> non-interactive uses of rename-file.  For interactive use, AFAIR we
> agreed that the behavior should stay as before, in particular to be
> consistent with e.g. invocation of 'mv' from the shell prompt.

That wasn't my understanding, and the email record is consistent with my 
interpretation. Although we discussed mv, the last comment on that topic 
was from John, who wrote about mv that "Making the Emacs version more 
restrictive, however, makes sense" (Bug#27986#145). This corresponds to 
the patch I installed, whose rename-file is more restrictive than mv 
because it balks at renaming a regular file F to G when G is not a 
directory name but happens to be an existing directory.

As I wrote, I don't think this will matter much in practice because the 
point is moot when file name completion is used. If I'm wrong and it is 
a significant problem, I can still implement the suggestion in 
Bug#27986#97 to ameliorate it. It may be better, though, to try out what 
we have now for a while, to see whether that suggestion would actually help.

bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Michael Albinus]

Paul Eggert <eggert@cs.ucla.edu> writes:

Hi Paul,

> I'll CC: this to Michael, as the last patch is to the Tramp tests, and
> he may want Tramp to become consistent with the new behavior for
> rename-file etc.

I've checked Tramp: it works already proper, thanks to Kai. At least for
the methods handled in tramp-sh.el. Next days, I'll check also
tramp-adb.el, tramp-gvfs.el, and tramp-smb.el.

I've added some additional tests to tramp-tests.el, in order to provoke
an error when a directory as target does not look like a directory name.

Best regards, Michael.