bug#66546: 30.0.50; save-buffer to write-protected file without backup fails

[Jens Schmidt]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
>> 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.


>From 4e959868b6fecd9399454a81877f151dfca218ac Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
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

>From 223284c964164cd5e07dbbfb763925922fbcb598 Mon Sep 17 00:00:00 2001
From: Jens Schmidt <jschmidt4gnu@vodafonemail.de>
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