From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains Date: Wed, 07 Jul 2021 21:02:35 +0300 Message-ID: <834kd6f1dw.fsf@gnu.org> References: <87o8bn7bie.fsf@gnus.org> <87zgv6vuon.fsf@gmx.de> <837di9lwbm.fsf@gnu.org> <87a6n5vuu4.fsf@gnus.org> <8735sqnmei.fsf@gnus.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="19436"; mail-complaints-to="usenet@ciao.gmane.io" Cc: michael.albinus@gmx.de, ncaprisunfan@gmail.com, 49261@debbugs.gnu.org To: Lars Ingebrigtsen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Jul 07 20:03:09 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1m1Bt7-0004nA-Bv for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 07 Jul 2021 20:03:09 +0200 Original-Received: from localhost ([::1]:54178 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1m1Bt5-0001Wd-W5 for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 07 Jul 2021 14:03:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:54672) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1m1Bt0-0001W3-8r for bug-gnu-emacs@gnu.org; Wed, 07 Jul 2021 14:03:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:41859) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1m1Bt0-0003Ah-1z for bug-gnu-emacs@gnu.org; Wed, 07 Jul 2021 14:03:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1m1Bsz-00065p-VF for bug-gnu-emacs@gnu.org; Wed, 07 Jul 2021 14:03:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 07 Jul 2021 18:03:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 49261 X-GNU-PR-Package: emacs Original-Received: via spool by 49261-submit@debbugs.gnu.org id=B49261.162568095323379 (code B ref 49261); Wed, 07 Jul 2021 18:03:01 +0000 Original-Received: (at 49261) by debbugs.gnu.org; 7 Jul 2021 18:02:33 +0000 Original-Received: from localhost ([127.0.0.1]:53405 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m1BsX-000651-FT for submit@debbugs.gnu.org; Wed, 07 Jul 2021 14:02:33 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:56718) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1m1BsU-00064o-QC for 49261@debbugs.gnu.org; Wed, 07 Jul 2021 14:02:32 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:40556) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1m1BsP-00036T-A3; Wed, 07 Jul 2021 14:02:25 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:4888 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1m1BsO-00056H-Tw; Wed, 07 Jul 2021 14:02:25 -0400 In-Reply-To: <8735sqnmei.fsf@gnus.org> (message from Lars Ingebrigtsen on Wed, 07 Jul 2021 18:01:25 +0200) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list 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-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:209604 Archived-At: > From: Lars Ingebrigtsen > Cc: Michael Albinus , ncaprisunfan@gmail.com, > 49261@debbugs.gnu.org > Date: Wed, 07 Jul 2021 18:01:25 +0200 > > +(defun auto-save--transform-file-name (filename transforms > + prefix suffix) > + "Transform FILENAME according to TRANSFORMS. > +See `auto-save-file-name-transforms' for the format of > +TRANSFORMS. PREFIX is prepended to the non-directory portion of > +the resulting file name, and SUFFIX is appended." > + (let (result uniq) > + ;; Apply user-specified translations > + ;; to the file name. > + (while (and transforms (not result)) > + (if (string-match (car (car transforms)) filename) > + (setq result (replace-match (cadr (car transforms)) t nil > + filename) > + uniq (car (cddr (car transforms))))) If this is going to be called from C, it should probably use save-match-data, because no one will expect that just modifying a file from some Lisp program could clobber the match data. Also, do we ever need this during loadup? Because before files.el is loaded by loadup, this function will not be available, so unconditionally calling it from C without protection, not even safe_call or somesuch, is not safe. > +static Lisp_Object > +make_lock_file_name (Lisp_Object fn) > +{ > + return ENCODE_FILE (call1 (intern ("make-lock-file-name"), fn)); > +} I'd prefer not to have a function return an encoded file name, it's unusual and unexpected. It is better to leave that to the caller. (And if you do that, the rationale for having a separate function for this will all but disappear, I think.) > - fn = Fexpand_file_name (fn, Qnil); > + fn = make_lock_file_name (Fexpand_file_name (fn, Qnil)); In the original code, 'fn' was an un-encoded file name, but your changes made it encoded. Why not keep the code more similar by having a separate variable with the encoded file name? E.g., this would avoid potential trouble here: > if (!NILP (subject_buf) > && NILP (Fverify_visited_file_modtime (subject_buf)) > && !NILP (Ffile_exists_p (fn)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< Ffile_exists_p was passed an un-encoded file name in the original code. It calls file handlers, and encodes local file names by itself, so it is better to pass it un-encoded file names. > - && !(lfname && current_lock_owner (NULL, lfname) == -2)) > + && !(create_lockfiles && current_lock_owner (NULL, lfname) == -2)) > call1 (intern ("userlock--ask-user-about-supersession-threat"), fn); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Likewise here: Lisp function should generally be called with un-encoded file names. > unlock_file_body (Lisp_Object fn) > { > char *lfname; > - USE_SAFE_ALLOCA; > - > - Lisp_Object filename = Fexpand_file_name (fn, Qnil); > - fn = ENCODE_FILE (filename); > > - MAKE_LOCK_NAME (lfname, fn); > + Lisp_Object filename = make_lock_file_name (Fexpand_file_name (fn, Qnil)); > + lfname = SSDATA (filename); > > int err = current_lock_owner (0, lfname); > if (err == -2 && unlink (lfname) != 0 && errno != ENOENT) > @@ -736,7 +714,6 @@ unlock_file_body (Lisp_Object fn) > if (0 < err) > report_file_errno ("Unlocking file", filename, err); Same problem here: 'filename' is now an encoded file name, so you call report_file_errno with an encoded file name, which is a no-no. Last, but not least: do we care that now locking a file will cons strings, even with the default value of lock-file-name-transforms? That sounds like we are punishing the vast majority of users for the benefit of the few who need the opt-in behavior. Should we perhaps avoid consing strings, or maybe even calling Lisp altogether, under the default value of that option? Thanks.