From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.bugs Subject: bug#27986: 26.0.50; 'rename-file' can rename files without confirmation Date: Fri, 11 Aug 2017 01:15:13 -0700 Organization: UCLA Computer Science Department Message-ID: <61980dde-3d68-7200-e7f4-98f62e410060@cs.ucla.edu> References: NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010434A5D07DB431825F2CDF" X-Trace: blaine.gmane.org 1502439390 26814 195.159.176.226 (11 Aug 2017 08:16:30 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 11 Aug 2017 08:16:30 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 Cc: Philipp , 27986@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri Aug 11 10:16:24 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dg578-00067w-3C for geb-bug-gnu-emacs@m.gmane.org; Fri, 11 Aug 2017 10:16:14 +0200 Original-Received: from localhost ([::1]:47327 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dg57E-0000zB-FZ for geb-bug-gnu-emacs@m.gmane.org; Fri, 11 Aug 2017 04:16:20 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44852) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dg575-0000wT-Px for bug-gnu-emacs@gnu.org; Fri, 11 Aug 2017 04:16:14 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dg56w-0008OR-2f for bug-gnu-emacs@gnu.org; Fri, 11 Aug 2017 04:16:07 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:46428) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dg56v-0008ON-UI for bug-gnu-emacs@gnu.org; Fri, 11 Aug 2017 04:16:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dg56v-0006sq-NW for bug-gnu-emacs@gnu.org; Fri, 11 Aug 2017 04:16:01 -0400 X-Loop: help-debbugs@gnu.org In-Reply-To: Resent-From: Paul Eggert Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 11 Aug 2017 08:16:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 27986 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 27986-submit@debbugs.gnu.org id=B27986.150243932824355 (code B ref 27986); Fri, 11 Aug 2017 08:16:01 +0000 Original-Received: (at 27986) by debbugs.gnu.org; 11 Aug 2017 08:15:28 +0000 Original-Received: from localhost ([127.0.0.1]:55109 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dg56M-0006Hp-45 for submit@debbugs.gnu.org; Fri, 11 Aug 2017 04:15:26 -0400 Original-Received: from zimbra.cs.ucla.edu ([131.179.128.68]:35396) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dg56I-00067n-Dy for 27986@debbugs.gnu.org; Fri, 11 Aug 2017 04:15:23 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id DBF1C16081F; Fri, 11 Aug 2017 01:15:15 -0700 (PDT) Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id uDgM-yBAxd5U; Fri, 11 Aug 2017 01:15:14 -0700 (PDT) Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 4E1CA160821; Fri, 11 Aug 2017 01:15:14 -0700 (PDT) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id wRXKSB58gkA8; Fri, 11 Aug 2017 01:15:14 -0700 (PDT) Original-Received: from [192.168.1.9] (unknown [47.153.184.153]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id 232BF16081F; Fri, 11 Aug 2017 01:15:14 -0700 (PDT) Content-Language: en-US X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:135650 Archived-At: This is a multi-part message in MIME format. --------------010434A5D07DB431825F2CDF Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable >> Probably the test for case-insensitive file names should be >> removed altogether >=20 > Which one? there are two of them. There should be just one such test, since it's testing the same thing and= =20 there's no point to doing the same test twice. And I notice the code has = some=20 other duplicate system calls. I installed the attached patch, which prune= s away=20 some of these duplicates. With this patch, (rename-file "a" "b/") now tak= es just=20 one system call on recent GNU/Linux, whereas it used to take four (and wa= s not=20 atomic). However, even with this patch there are races on GNU/Linux which can lead= to=20 potential security problems. Perhaps we can't fix these races on MS-Windo= ws but=20 we should be able to fix them on a GNUish host. However, we will need to = change=20 the semantics of rename-file etc. slightly, since no single system call s= upports=20 the cp-like target rewriting of these functions. I have a fix in mind to = do that=20 in a hopefully compatible-enough way, which I'll try to propose soon. I'l= l keep=20 case-insensitive file systems in mind when I do that. This reminds me of a similar problem in GNU coreutils which I fixed a doz= en=20 years ago by adding the -T option to mv, ln, etc. --------------010434A5D07DB431825F2CDF Content-Type: text/x-patch; name="0001-Improve-performance-for-rename-file-etc.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0001-Improve-performance-for-rename-file-etc.patch" =46rom a56e6e79613779895975b1762c311bf8fe46f551 Mon Sep 17 00:00:00 2001 From: Paul Eggert 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")= ))) =20 -(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 (=3D lastc ?/) - (and (memq system-type '(windows-nt ms-dos)) - (=3D lastc ?\\))))) - (defun directory-files-recursively (dir regexp &optional include-directo= ries) "Return list of all files under DIR that have file names matching REGE= XP. 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. =20 -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; } =20 +DEFUN ("directory-name-p", Fdirectory_name_p, Sdirectory_name_p, 1, 1, 0= , + doc: /* Return non-nil if NAME ends with a directory separator ch= aracter. */) + (Lisp_Object name) +{ + CHECK_STRING (name); + ptrdiff_t namelen =3D SBYTES (name); + unsigned char c =3D 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 (n= ame)); +} + +/* 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 out= put file to match the input file. =20 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.) =20 A prefix arg makes KEEP-TIME non-nil. =20 -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. =20 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 =20 - encoded_file =3D encoded_newname =3D Qnil; - CHECK_STRING (file); - CHECK_STRING (newname); - - if (!NILP (Ffile_directory_p (newname))) - newname =3D Fexpand_file_name (Ffile_name_nondirectory (file), newna= me); - else - newname =3D Fexpand_file_name (newname, Qnil); - file =3D Fexpand_file_name (file, Qnil); + newname =3D expand_cp_target (file, newname); =20 /* 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 exis= ts. +An integer third arg means request confirmation if NEWNAME already exist= s. This is what happens in interactive use with M-x. */) (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exis= ts) { @@ -2314,24 +2338,22 @@ This is what happens in interactive use with M-x.= */) Lisp_Object encoded_file, encoded_newname, symlink_target; int dirp =3D -1; =20 - symlink_target =3D encoded_file =3D encoded_newname =3D Qnil; - CHECK_STRING (file); - CHECK_STRING (newname); file =3D Fexpand_file_name (file, Qnil); =20 - 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 =3D false; + if (!NILP (Ffile_name_case_insensitive_p (file))) { - dirp =3D !NILP (Ffile_directory_p (file)); - Lisp_Object fname =3D dirp ? Fdirectory_file_name (file) : file; - newname =3D Fexpand_file_name (Ffile_name_nondirectory (fname), ne= wname); + newname =3D Fexpand_file_name (newname, Qnil); + case_only_rename =3D !NILP (Fstring_equal (Fdowncase (file), + Fdowncase (newname))); } - else - newname =3D Fexpand_file_name (newname, Qnil); + + if (!case_only_rename) + newname =3D expand_cp_target (Fdirectory_file_name (file), newname);= =20 /* 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 =3D ENCODE_FILE (file); encoded_newname =3D ENCODE_FILE (newname); =20 - /* 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 - =3D ((!NILP (ok_if_already_exists) && !INTEGERP (ok_if_already_exist= s)) - || (file_name_case_insensitive_p (SSDATA (encoded_file)) - && ! NILP (Fstring_equal (Fdowncase (file), Fdowncase (newname)))));= - + bool plain_rename =3D (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 =3D !NILP (Ffile_directory_p (file)); + dirp =3D 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 str= ings. -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 exis= ts. +An integer third arg means request confirmation if NEWNAME already exist= s. This is what happens in interactive use with M-x. */) (Lisp_Object file, Lisp_Object newname, Lisp_Object ok_if_already_exis= ts) { Lisp_Object handler; Lisp_Object encoded_file, encoded_newname; =20 - encoded_file =3D encoded_newname =3D Qnil; - CHECK_STRING (file); - CHECK_STRING (newname); file =3D Fexpand_file_name (file, Qnil); - - if (!NILP (Ffile_directory_p (newname))) - newname =3D Fexpand_file_name (Ffile_name_nondirectory (file), newna= me); - else - newname =3D Fexpand_file_name (newname, Qnil); + newname =3D expand_cp_target (file, newname); =20 /* 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 exi= sts. +An integer third arg means request confirmation if LINKNAME already exis= ts. This happens for interactive use with M-x. */) (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_e= xists) { Lisp_Object handler; Lisp_Object encoded_target, encoded_linkname; =20 - encoded_target =3D encoded_linkname =3D 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) =3D=3D '~') target =3D Fexpand_file_name (target, Qnil); =20 - if (!NILP (Ffile_directory_p (linkname))) - linkname =3D Fexpand_file_name (Ffile_name_nondirectory (target), li= nkname); - else - linkname =3D Fexpand_file_name (linkname, Qnil); + linkname =3D expand_cp_target (target, linkname); =20 /* 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. =20 A non-nil NO-MESSAGE argument means do not print any message if successf= ul. A non-nil CURRENT-ONLY argument means save only current buffer. */) @@ -6111,7 +6115,7 @@ This applies only to the operation `inhibit-file-na= me-operation'. */); Vinhibit_file_name_operation =3D Qnil; =20 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-pre= fix' 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' an= d 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); --=20 2.7.4 --------------010434A5D07DB431825F2CDF--