From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Jens Schmidt via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#66546: 30.0.50; save-buffer to write-protected file without backup fails Date: Thu, 19 Oct 2023 23:12:59 +0200 Message-ID: <87zg0ez57o.fsf@sappc2.fritz.box> References: <83h6mtq9t7.fsf@gnu.org> <8734ydx7x3.fsf@sappc2.fritz.box> <83cyxgqwjm.fsf@gnu.org> <87lec4cjqe.fsf@sappc2.fritz.box> <83ttqsp5x1.fsf@gnu.org> <87il78cdyf.fsf@sappc2.fritz.box> <83pm1gozi6.fsf@gnu.org> <87edhvd84h.fsf@sappc2.fritz.box> <838r82q0gi.fsf@gnu.org> <87wmvmfi68.fsf@sappc2.fritz.box> <83o7gxo771.fsf@gnu.org> <87ttqpgg8l.fsf@sappc2.fritz.box> <83cyxcnp2f.fsf@gnu.org> <87sf671xe3.fsf@sappc2.fritz.box> <83y1fzmdhm.fsf@gnu.org> Reply-To: Jens Schmidt Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8130"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) Cc: 66546@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Oct 19 23:14:13 2023 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 1qtaLM-0001qh-QH for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 19 Oct 2023 23:14:12 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qtaKo-0007Gt-Db; Thu, 19 Oct 2023 17:13:38 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qtaKm-0007Gh-1T for bug-gnu-emacs@gnu.org; Thu, 19 Oct 2023 17:13:36 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qtaKl-00030C-Ez for bug-gnu-emacs@gnu.org; Thu, 19 Oct 2023 17:13:35 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qtaLB-0004gM-Ui for bug-gnu-emacs@gnu.org; Thu, 19 Oct 2023 17:14:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Jens Schmidt Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 19 Oct 2023 21:14:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 66546 X-GNU-PR-Package: emacs Original-Received: via spool by 66546-submit@debbugs.gnu.org id=B66546.169775002817968 (code B ref 66546); Thu, 19 Oct 2023 21:14:01 +0000 Original-Received: (at 66546) by debbugs.gnu.org; 19 Oct 2023 21:13:48 +0000 Original-Received: from localhost ([127.0.0.1]:37996 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qtaKx-0004fk-HL for submit@debbugs.gnu.org; Thu, 19 Oct 2023 17:13:48 -0400 Original-Received: from mr5.vodafonemail.de ([145.253.228.165]:59852) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qtaKs-0004fS-Tc for 66546@debbugs.gnu.org; Thu, 19 Oct 2023 17:13:46 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vodafonemail.de; s=vfde-mb-mr2-23sep; t=1697749990; bh=ok2VK5G8LCw/rCXMNuh0lpvZ7rcrZja8CAUp0I+W0HY=; h=From:To:Subject:References:Date:In-Reply-To:Message-ID:User-Agent: Content-Type:From; b=ZUUqiErxnlNFflv8T5dYzghdnIqv4ArK8AoC3tCflJ793kUmSClCvrefFZs/Nbecp LBShxapSjUb+jChTrgp+2OLSjYoKVrjw4RHVNS1RNsiHiaugVyJ/YuXH4pw3q4otVD C/2eePNohT/UzsHVOo4D5MWOuia05hB3cJxPC/2c= Original-Received: from smtp.vodafone.de (unknown [10.0.0.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by mr5.vodafonemail.de (Postfix) with ESMTPS id 4SBL6G4r2Nz1yQw; Thu, 19 Oct 2023 21:13:10 +0000 (UTC) Original-Received: from sappc2 (port-92-194-49-48.dynamic.as20676.net [92.194.49.48]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp.vodafone.de (Postfix) with ESMTPSA id 4SBL665T8LzHnHf; Thu, 19 Oct 2023 21:12:59 +0000 (UTC) In-Reply-To: <83y1fzmdhm.fsf@gnu.org> (Eli Zaretskii's message of "Thu, 19 Oct 2023 07:40:21 +0300") X-purgate-type: clean X-purgate: clean X-purgate-size: 11096 X-purgate-ID: 155817::1697749986-1F7FB94E-D867F86D/0/0 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-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:272760 Archived-At: --=-=-= Content-Type: text/plain Eli Zaretskii writes: >> From: Jens Schmidt >> Cc: 66546@debbugs.gnu.org >> Date: Wed, 18 Oct 2023 22:36:04 +0200 >> >> Why exactly do we need that >> >> (set-file-extended-attributes >> buffer-file-name >> (file-extended-attributes buffer-file-name)) >> >> incantation or the equivalent one >> >> (setq setmodes >> (list (file-modes buffer-file-name) >> (with-demoted-errors >> "Error getting extended attributes: %s" >> (file-extended-attributes buffer-file-name)) >> buffer-file-name)) >> (with-demoted-errors "Error setting attributes: %s" >> (set-file-extended-attributes buffer-file-name >> (nth 1 setmodes))) >> >> in function `basic-save-buffer-2'? > > Because it was added there long ago, and because we have no reason to > believe it does any harm. I see. The only drawback to such harmless but potentially confusing code that I see is that it attracts future questions and protracted discussions like this one. Of course, one could check the commit (hopefully the right one), find this bug, and read it until your message above. But probably it would be helpful to shortcut that look-up process, like this: @@ -5946,7 +5949,9 @@ basic-save-buffer-2 (file-extended-attributes buffer-file-name)) buffer-file-name)) ;; If set-file-extended-attributes fails to make the - ;; file writable, fall back on set-file-modes. + ;; file writable, fall back on set-file-modes. Calling + ;; set-file-extended-attributes here may or may not be + ;; actually necessary, for details see Bug#66546. (with-demoted-errors "Error setting attributes: %s" (set-file-extended-attributes buffer-file-name (nth 1 setmodes))) Anyway, there is still issue B left. Since you have fixed issue A already, the reproducer has slightly changed: rm -f foo echo foo > foo chmod 0400 foo ./src/emacs -Q Define the following advice on `write-region' to simulate a write error during buffer save: ------------------------- snip ------------------------- (advice-add 'write-region :override (lambda (&rest _) (error "No space left on device"))) ------------------------- snip ------------------------- Then continue C-x C-f foo RET C-x C-q bar C-0 C-x C-s => File foo is write-protected; try to save anyway? yes RET => No space left on device So far everything is as expected: The buffer is in status "unsaved", the file unchanged. Now check the permissions of file foo: [emacs-master]$ ls -al foo -rw------- 1 jschmidt jschmidt 7 Oct 14 20:33 foo So the temporary write permissions of file foo did not get removed again. Which they should have been, in my opinion. Here comes my analysis, hopefully more explicit and detailed than for the first issue. I'm aware that this bug is *really* old, but I think this scenario, though rarely met, should be handled properly as well. At least I'm not attempting to remove old code ... The issue is located again in function `basic-save-buffer-2'. There it is limited to the "else" branch of the following `if': (if (or (and file-precious-flag dir-writable) (and break-hardlink-on-save ...)) so I'll focus only on that "else" branch plus the code before that `if'. Let's call the file being saved TARGET-FILE. In the focused part of the function, variable `setmodes' gets set to A) the result of function `(backup-buffer)': The value is non-nil after a backup was made by renaming. It has the form (MODES EXTENDED-ATTRIBUTES BACKUPNAME). B) or an analogous triple (MODES EXTENDED-ATTRIBUTES TARGET-FILE) if TARGET-FILE is not writable and no backup should be created: (setq setmodes (list (file-modes buffer-file-name) (with-demoted-errors "Error getting extended attributes: %s" (file-extended-attributes buffer-file-name)) buffer-file-name)) In this case, TARGET-FILE is made writable after the triple has been assigned to `setmodes'. In any case, if non-nil, the resulting value of `setmodes' is intended to restore the state (extended attributes and mode) of TARGET-FILE as good as the OS permits after the save operation. This holds both for successful and unsuccessful ("no space left on device") save operations. However, the handling of successful and unsuccessful operations differs: - After a successful save and if `setmodes' is non-nil, the state recorded in `setmodes' should be applied to TARGET-FILE, which in case A) has been newly created or in case B) has been overwritten with new contents. - After an unsuccessful save and if `setmodes' is non-nil, in case A) the backup file of TARGET-FILE should be renamed to TARGET-FILE. In case B), however, I think that the original permissions of TARGET-FILE should be restored. As far as the OS permits. The save operation is implemented by the call to function `write-region' in the following `unwind-protect': (unwind-protect (progn ;; Pass in nil&nil rather than point-min&max to indicate ;; we're saving the buffer rather than just a region. ;; write-region-annotate-functions may make use of it. (write-region nil nil buffer-file-name nil t buffer-file-truename) (when save-silently (message nil)) (setq success t)) ;; If we get an error writing the new file, and we made ;; the backup by renaming, undo the backing-up. (and setmodes (not success) (progn (rename-file (nth 2 setmodes) buffer-file-name t) (setq buffer-backed-up nil)))) The problem here is that the UNWINDFORMS do not distinguish whether the value of `setmodes' is non-nil because a backup file has been created (case A above) or because TARGET-FILE has been made temporarily writable (case B above). As a result, in case B) function `rename-file' is called with both arguments FILE and NEWNAME equalling TARGET-FILE. While this is not the bug per se, it does at least not restore the permissions on TARGET-FILE. My patch fixes this issue by distinguishing cases A) and B) in the UNWDINFORMS. Please review. I also attached a slightly unrelated, minor doc fix I came across when working on this bug. --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-Fix-argument-name-for-function-copy-file.patch >From 4e959868b6fecd9399454a81877f151dfca218ac Mon Sep 17 00:00:00 2001 From: Jens Schmidt Date: Wed, 18 Oct 2023 22:43:37 +0200 Subject: [PATCH 1/2] ; Fix argument name for function copy-file * doc/lispref/files.texi (Changing Files): Change name of last argument of function `copy-file' from `preserve-extended-attributes' to `preserve-permissions', as used in the function's description and doc string. (Bug#66546) --- doc/lispref/files.texi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi index afedf776c86..dc66ea8bc9c 100644 --- a/doc/lispref/files.texi +++ b/doc/lispref/files.texi @@ -1803,7 +1803,7 @@ Changing Files @var{oldname} is a directory and a non-directory otherwise. @end deffn -@deffn Command copy-file oldname newname &optional ok-if-already-exists time preserve-uid-gid preserve-extended-attributes +@deffn Command copy-file oldname newname &optional ok-if-already-exists time preserve-uid-gid preserve-permissions This command copies the file @var{oldname} to @var{newname}. An error is signaled if @var{oldname} is not a regular file. If @var{newname} names a directory, it copies @var{oldname} into that directory, -- 2.30.2 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0002-Better-handle-errors-when-writing-r-o-files-without-.patch >From 223284c964164cd5e07dbbfb763925922fbcb598 Mon Sep 17 00:00:00 2001 From: Jens Schmidt Date: Thu, 19 Oct 2023 23:00:32 +0200 Subject: [PATCH 2/2] Better handle errors when writing r-o files without backup * lisp/files.el (basic-save-buffer-2): Restore file permissions when writing read-only files without backup fails. (Bug#66546) --- lisp/files.el | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/lisp/files.el b/lisp/files.el index adfe8bd44b9..9fc2f5f376a 100644 --- a/lisp/files.el +++ b/lisp/files.el @@ -5934,10 +5934,13 @@ basic-save-buffer-2 t)) ;; If file not writable, see if we can make it writable ;; temporarily while we write it (its original modes will be - ;; restored in 'basic-save-buffer'). But no need to do so if - ;; we have just backed it up (setmodes is set) because that - ;; says we're superseding. + ;; restored in 'basic-save-buffer' or, in case of an error, in + ;; the `unwind-protect' below). But no need to do so if we + ;; have just backed it up (setmodes is set) because that says + ;; we're superseding. (cond ((and tempsetmodes (not setmodes)) + ;; Remember we made the file writable. + (setq tempsetmodes 'u+w) ;; Change the mode back, after writing. (setq setmodes (list (file-modes buffer-file-name) @@ -5963,12 +5966,23 @@ basic-save-buffer-2 buffer-file-name nil t buffer-file-truename) (when save-silently (message nil)) (setq success t)) - ;; If we get an error writing the new file, and we made - ;; the backup by renaming, undo the backing-up. - (and setmodes (not success) - (progn - (rename-file (nth 2 setmodes) buffer-file-name t) - (setq buffer-backed-up nil))))))) + (cond + ;; If we get an error writing the file which we + ;; previously made writable, attempt to undo the + ;; write-access. + ((and (eq tempsetmodes 'u+w) (not success)) + (condition-case () + (unless + (with-demoted-errors "Error setting file modes: %S" + (set-file-modes buffer-file-name (car setmodes))) + (set-file-extended-attributes buffer-file-name + (nth 1 setmodes))) + (error nil))) + ;; If we get an error writing the new file, and we made + ;; the backup by renaming, undo the backing-up. + ((and setmodes (not success)) + (rename-file (nth 2 setmodes) buffer-file-name t) + (setq buffer-backed-up nil))))))) setmodes)) (declare-function diff-no-select "diff" -- 2.30.2 --=-=-=--